diff --git a/experimental/ir/ir_imports.go b/experimental/ir/ir_imports.go index 964a26f5..bb88e146 100644 --- a/experimental/ir/ir_imports.go +++ b/experimental/ir/ir_imports.go @@ -126,6 +126,15 @@ type imported struct { // still re-exported. public bool + // Whether this file's symbols can only be used in options, because it is + // not reachable through any import that is not an option import, either + // directly or because that import re-exports it. + // + // This is not the same as option, which records how the import was + // declared. An option import is not option-only if a regular import also + // re-exports it. + optionOnly bool + visible, used bool } @@ -152,16 +161,11 @@ func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) { // import cannot be classified by looking only at whichever direct import // happens to pull it in first. Instead, classify every transitive import // up front, quantifying over all direct imports. - public, visible := i.classifyTransitive() + public, visible, dependencies := i.classifyTransitive() - for k, file := range seq.All(i.Directs()) { + for file := range seq.Values(i.Directs()) { for imp := range seq.Values(file.TransitiveImports()) { if !mapsx.AddZero(dedup, imp.InternedPath()) { - // If imp is public, but file is already present, we need to - // treat this import as non-option, because this overrides it. - if imp.Public { - i.files[k].option = false - } continue } @@ -186,6 +190,10 @@ func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) { } } + for n := range i.files { + i.files[n].optionOnly = !dependencies.ContainsID(i.files[n].file.InternedPath()) + } + // Now, build the path and causes maps. i.byPath = make(intern.Map[uint32]) i.causes = make(intern.Map[uint32]) @@ -194,11 +202,18 @@ func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) { i.byPath[imp.file.InternedPath()] = uint32(n) } + // Direct imports take precedence over transitive imports, and a direct + // import that re-exports a file takes precedence over one that does not. for k, file := range seq.All(i.Directs()) { - // Direct imports take precedence over transitive imports. i.causes[file.InternedPath()] = uint32(k) - for imp := range seq.Values(file.TransitiveImports()) { - mapsx.Add(i.causes, imp.InternedPath(), uint32(k)) + } + for _, public := range []bool{true, false} { + for k, file := range seq.All(i.Directs()) { + for imp := range seq.Values(file.TransitiveImports()) { + if imp.Public == public { + mapsx.Add(i.causes, imp.InternedPath(), uint32(k)) + } + } } } } @@ -216,10 +231,18 @@ func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) { // Neither set includes the direct imports themselves, which are always // visible, nor files reached only through non-public imports, which are // neither. -func (i *imports) classifyTransitive() (public, visible intern.Set) { +// +// dependencies is the set of direct imports that are not option imports, +// together with the files they re-export. Unlike public and visible, it +// includes direct imports. +func (i *imports) classifyTransitive() (public, visible, dependencies intern.Set) { public = make(intern.Set) visible = make(intern.Set) + dependencies = make(intern.Set) for file := range seq.Values(i.Directs()) { + if !file.Option { + dependencies.AddID(file.InternedPath()) + } for imp := range seq.Values(file.TransitiveImports()) { if !imp.Public { continue @@ -229,9 +252,12 @@ func (i *imports) classifyTransitive() (public, visible intern.Set) { if file.Public { public.AddID(imp.InternedPath()) } + if !file.Option { + dependencies.AddID(imp.InternedPath()) + } } } - return public, visible + return public, visible, dependencies } // Insert inserts a new import at the given position. It also builds up the path map for lookups. diff --git a/experimental/ir/ir_symbol.go b/experimental/ir/ir_symbol.go index ad3cfb41..6f310b65 100644 --- a/experimental/ir/ir_symbol.go +++ b/experimental/ir/ir_symbol.go @@ -166,7 +166,7 @@ func (s Symbol) Visible(in *File, allowOptions bool) bool { } imp := in.imports.files[idx] - if !imp.visible || !(allowOptions || !imp.option) { + if !imp.visible || (!allowOptions && imp.optionOnly) { return false } diff --git a/experimental/ir/ir_test.go b/experimental/ir/ir_test.go index 91aab7e8..6d95a162 100644 --- a/experimental/ir/ir_test.go +++ b/experimental/ir/ir_test.go @@ -555,13 +555,15 @@ func symtabProto(files []*ir.File) *compilerpb.SymbolSet { options = sym.AsOneof().Options() } + // OptionOnly is set for symbols that are visible in options, but not + // elsewhere. symtab.Symbols = append(symtab.Symbols, &compilerpb.Symbol{ Fqn: string(sym.FullName()), Kind: compilerpb.Symbol_Kind(sym.Kind()), File: sym.Context().Path(), Index: uint32(sym.RawData()), Visible: sym.Kind() != ir.SymbolKindPackage && sym.Visible(file, false), - OptionOnly: sym.Kind() != ir.SymbolKindPackage && !sym.Visible(file, true) && sym.Visible(file, true), + OptionOnly: sym.Kind() != ir.SymbolKindPackage && !sym.Visible(file, false) && sym.Visible(file, true), Options: new(optionWalker).message(options), Features: dumpFeatures(sym.FeatureSet(), sym.Kind().OptionTarget()), }) diff --git a/experimental/ir/lower_resolve.go b/experimental/ir/lower_resolve.go index 17aef93c..18a20220 100644 --- a/experimental/ir/lower_resolve.go +++ b/experimental/ir/lower_resolve.go @@ -372,7 +372,10 @@ func (r symbolRef) diagnoseLookup(sym Symbol, expectedName FullName) *report.Dia ) case !sym.Visible(r.File, r.allowOption): if !r.allowOption && sym.Visible(r.File, true) { - decl := sym.Import(r.File).Decl + // The symbol may come from a file the option import re-exports, so + // point at the direct import instead. + imports := &r.File.imports + decl := imports.files[imports.causes[sym.Context().InternedPath()]].decl var option token.Token for m := range seq.Values(decl.ModifierTokens()) { if m.Keyword() == keyword.Option { diff --git a/experimental/ir/lower_validate.go b/experimental/ir/lower_validate.go index 2e42499d..6419e032 100644 --- a/experimental/ir/lower_validate.go +++ b/experimental/ir/lower_validate.go @@ -57,7 +57,7 @@ func diagnoseUnusedImports(f *File, r *report.Report) { } r.Warnf("unused import %s", imp.Decl.ImportPath().AsLiteral().Text()).Apply( - report.Snippet(imp.Decl.ImportPath()), + report.Snippet(imp.Decl), report.SuggestEdits(imp.Decl, "delete it", report.Edit{ Start: 0, End: imp.Decl.Span().Len(), }), diff --git a/experimental/ir/testdata/imports/option.proto.yaml.symtab.yaml b/experimental/ir/testdata/imports/option.proto.yaml.symtab.yaml index b9bbc2ba..bb5ae91b 100644 --- a/experimental/ir/testdata/imports/option.proto.yaml.symtab.yaml +++ b/experimental/ir/testdata/imports/option.proto.yaml.symtab.yaml @@ -23,18 +23,22 @@ tables: kind: KIND_MESSAGE file: "option.proto" index: 1 + option_only: true - fqn: "buf.test.Foo.x" kind: KIND_FIELD file: "option.proto" index: 1 + option_only: true - fqn: "buf.test.e" kind: KIND_EXTENSION file: "option.proto" index: 2 + option_only: true - fqn: "buf.test.any_opt" kind: KIND_EXTENSION file: "option.proto" index: 3 + option_only: true "main2.proto": imports: - { path: "google/protobuf/any.proto", transitive: true } @@ -60,18 +64,22 @@ tables: kind: KIND_MESSAGE file: "option.proto" index: 1 + visible: true - fqn: "buf.test.Foo.x" kind: KIND_FIELD file: "option.proto" index: 1 + visible: true - fqn: "buf.test.e" kind: KIND_EXTENSION file: "option.proto" index: 2 + visible: true - fqn: "buf.test.any_opt" kind: KIND_EXTENSION file: "option.proto" index: 3 + visible: true "main3.proto": imports: - { path: "google/protobuf/any.proto", transitive: true } @@ -94,15 +102,19 @@ tables: kind: KIND_MESSAGE file: "option.proto" index: 1 + option_only: true - fqn: "buf.test.Foo.x" kind: KIND_FIELD file: "option.proto" index: 1 + option_only: true - fqn: "buf.test.e" kind: KIND_EXTENSION file: "option.proto" index: 2 + option_only: true - fqn: "buf.test.any_opt" kind: KIND_EXTENSION file: "option.proto" index: 3 + option_only: true diff --git a/experimental/ir/testdata/imports/option_public.proto.yaml b/experimental/ir/testdata/imports/option_public.proto.yaml new file mode 100644 index 00000000..51724f24 --- /dev/null +++ b/experimental/ir/testdata/imports/option_public.proto.yaml @@ -0,0 +1,80 @@ +descriptor: true +symtab: true +files: +# An option import must stay an option import, even when its public import is +# also reached through another import. +- path: "main.proto" + text: | + edition = "2024"; + package buf.test; + + import "other.proto"; + import option "b.proto"; + + option (c.copt) = 1; + +# An option import that is also re-exported by a regular import is fully +# visible. +- path: "main2.proto" + text: | + edition = "2024"; + package buf.test; + + import "x.proto"; + import option "b.proto"; + + message M2 { + b.B f = 1; + c.C g = 2; + } + +# A file re-exported only by an option import is only visible in options. +- path: "main3.proto" + text: | + edition = "2024"; + package buf.test; + + import option "b.proto"; + + option (c.copt) = 1; + message M3 { c.C f = 1; } + +- path: "other.proto" + import: true + text: | + edition = "2024"; + package other; + + import option "b.proto"; + +- path: "x.proto" + import: true + text: | + edition = "2024"; + package x; + + import public "b.proto"; + +- path: "b.proto" + import: true + text: | + edition = "2024"; + package b; + + import public "c.proto"; + + message B {} + +- path: "c.proto" + import: true + text: | + edition = "2024"; + package c; + + import "google/protobuf/descriptor.proto"; + + message C {} + + extend google.protobuf.FileOptions { + int32 copt = 50000; + } diff --git a/experimental/ir/testdata/imports/option_public.proto.yaml.fds.yaml b/experimental/ir/testdata/imports/option_public.proto.yaml.fds.yaml new file mode 100644 index 00000000..a5967172 --- /dev/null +++ b/experimental/ir/testdata/imports/option_public.proto.yaml.fds.yaml @@ -0,0 +1,75 @@ +file: +- name: "c.proto" + package: "c" + dependency: ["google/protobuf/descriptor.proto"] + message_type: [{ name: "C" }] + extension: + - name: "copt" + number: 50000 + label: LABEL_OPTIONAL + type: TYPE_INT32 + extendee: ".google.protobuf.FileOptions" + json_name: "copt" + syntax: "editions" + edition: EDITION_2024 +- name: "b.proto" + package: "b" + dependency: ["c.proto"] + public_dependency: [0] + message_type: [{ name: "B" }] + syntax: "editions" + edition: EDITION_2024 +- name: "other.proto" + package: "other" + option_dependency: ["b.proto"] + syntax: "editions" + edition: EDITION_2024 +- name: "main.proto" + package: "buf.test" + dependency: ["other.proto"] + option_dependency: ["b.proto"] + options.$unknown: "50000: 1" + syntax: "editions" + edition: EDITION_2024 +- name: "x.proto" + package: "x" + dependency: ["b.proto"] + public_dependency: [0] + syntax: "editions" + edition: EDITION_2024 +- name: "main2.proto" + package: "buf.test" + dependency: ["x.proto"] + option_dependency: ["b.proto"] + message_type: + - name: "M2" + field: + - name: "f" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".b.B" + json_name: "f" + - name: "g" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".c.C" + json_name: "g" + syntax: "editions" + edition: EDITION_2024 +- name: "main3.proto" + package: "buf.test" + option_dependency: ["b.proto"] + message_type: + - name: "M3" + field: + - name: "f" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".c.C" + json_name: "f" + options.$unknown: "50000: 1" + syntax: "editions" + edition: EDITION_2024 diff --git a/experimental/ir/testdata/imports/option_public.proto.yaml.stderr.txt b/experimental/ir/testdata/imports/option_public.proto.yaml.stderr.txt new file mode 100644 index 00000000..e5cf2605 --- /dev/null +++ b/experimental/ir/testdata/imports/option_public.proto.yaml.stderr.txt @@ -0,0 +1,39 @@ +warning: unused import "other.proto" + --> main.proto:4:1 + | + 4 | import "other.proto"; + | ^^^^^^^^^^^^^^^^^^^^^ + help: delete it + | + 4 | - import "other.proto"; + | + = help: no symbols from this file are referenced + +error: `c.C` is only imported for use in options + --> main3.proto:7:14 + | + 4 | import option "b.proto"; + | ------------------------ imported as `option` here + 5 | + 6 | option (c.copt) = 1; + 7 | message M3 { c.C f = 1; } + | ^^^ requires non-`option` import + | + help: delete `option` + | + 4 | - import option "b.proto"; + 4 | + import "b.proto"; + | + +warning: unused import "b.proto" + --> other.proto:4:1 + | + 4 | import option "b.proto"; + | ^^^^^^^^^^^^^^^^^^^^^^^^ + help: delete it + | + 4 | - import option "b.proto"; + | + = help: no symbols from this file are referenced + +encountered 1 error and 2 warnings diff --git a/experimental/ir/testdata/imports/option_public.proto.yaml.symtab.yaml b/experimental/ir/testdata/imports/option_public.proto.yaml.symtab.yaml new file mode 100644 index 00000000..f8abfee6 --- /dev/null +++ b/experimental/ir/testdata/imports/option_public.proto.yaml.symtab.yaml @@ -0,0 +1,107 @@ +tables: + "main.proto": + imports: + - { path: "b.proto", visible: true } + - { path: "c.proto", transitive: true, visible: true } + - { path: "google/protobuf/descriptor.proto", transitive: true } + - { path: "other.proto", visible: true } + symbols: + - { fqn: "b", kind: KIND_PACKAGE, file: "b.proto" } + - fqn: "b.B" + kind: KIND_MESSAGE + file: "b.proto" + index: 1 + option_only: true + - { fqn: "c", kind: KIND_PACKAGE, file: "c.proto" } + - fqn: "c.C" + kind: KIND_MESSAGE + file: "c.proto" + index: 1 + option_only: true + - fqn: "c.copt" + kind: KIND_EXTENSION + file: "c.proto" + index: 1 + option_only: true + - { fqn: "buf", kind: KIND_PACKAGE, file: "main.proto" } + - { fqn: "buf.test", kind: KIND_PACKAGE, file: "main.proto" } + - { fqn: "other", kind: KIND_PACKAGE, file: "other.proto" } + options.message.extns: { "c.copt": { i32: 1 } } + "main2.proto": + imports: + - { path: "b.proto", visible: true } + - { path: "c.proto", transitive: true, visible: true } + - { path: "google/protobuf/descriptor.proto", transitive: true } + - { path: "x.proto", visible: true } + symbols: + - { fqn: "b", kind: KIND_PACKAGE, file: "b.proto" } + - fqn: "b.B" + kind: KIND_MESSAGE + file: "b.proto" + index: 1 + visible: true + - { fqn: "c", kind: KIND_PACKAGE, file: "c.proto" } + - fqn: "c.C" + kind: KIND_MESSAGE + file: "c.proto" + index: 1 + visible: true + - fqn: "c.copt" + kind: KIND_EXTENSION + file: "c.proto" + index: 1 + visible: true + - { fqn: "buf", kind: KIND_PACKAGE, file: "main2.proto" } + - { fqn: "buf.test", kind: KIND_PACKAGE, file: "main2.proto" } + - fqn: "buf.test.M2" + kind: KIND_MESSAGE + file: "main2.proto" + index: 1 + visible: true + - fqn: "buf.test.M2.f" + kind: KIND_FIELD + file: "main2.proto" + index: 1 + visible: true + - fqn: "buf.test.M2.g" + kind: KIND_FIELD + file: "main2.proto" + index: 2 + visible: true + - { fqn: "x", kind: KIND_PACKAGE, file: "x.proto" } + "main3.proto": + imports: + - { path: "b.proto", visible: true } + - { path: "c.proto", transitive: true, visible: true } + - { path: "google/protobuf/descriptor.proto", transitive: true } + symbols: + - { fqn: "b", kind: KIND_PACKAGE, file: "b.proto" } + - fqn: "b.B" + kind: KIND_MESSAGE + file: "b.proto" + index: 1 + option_only: true + - { fqn: "c", kind: KIND_PACKAGE, file: "c.proto" } + - fqn: "c.C" + kind: KIND_MESSAGE + file: "c.proto" + index: 1 + option_only: true + - fqn: "c.copt" + kind: KIND_EXTENSION + file: "c.proto" + index: 1 + option_only: true + - { fqn: "buf", kind: KIND_PACKAGE, file: "main3.proto" } + - { fqn: "buf.test", kind: KIND_PACKAGE, file: "main3.proto" } + - fqn: "buf.test.M3" + kind: KIND_MESSAGE + file: "main3.proto" + index: 1 + visible: true + - fqn: "buf.test.M3.f" + kind: KIND_FIELD + file: "main3.proto" + index: 1 + visible: true + options.message.extns: { "c.copt": { i32: 1 } } diff --git a/experimental/ir/testdata/imports/unused.proto.yaml.stderr.txt b/experimental/ir/testdata/imports/unused.proto.yaml.stderr.txt index 603d644e..e28c8be3 100644 --- a/experimental/ir/testdata/imports/unused.proto.yaml.stderr.txt +++ b/experimental/ir/testdata/imports/unused.proto.yaml.stderr.txt @@ -1,8 +1,8 @@ warning: unused import "b.proto" - --> a2.proto:4:8 + --> a2.proto:4:1 | 4 | import "b.proto"; - | ^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^ help: delete it | 4 | - import "b.proto"; @@ -10,10 +10,10 @@ warning: unused import "b.proto" = help: no symbols from this file are referenced warning: unused import "c.proto" - --> a2.proto:5:8 + --> a2.proto:5:1 | 5 | import "c.proto"; - | ^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^ help: delete it | 5 | - import "c.proto"; @@ -21,10 +21,10 @@ warning: unused import "c.proto" = help: no symbols from this file are referenced warning: unused import "d.proto" - --> a2.proto:6:8 + --> a2.proto:6:1 | 6 | import "d.proto"; - | ^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^ help: delete it | 6 | - import "d.proto"; @@ -32,10 +32,10 @@ warning: unused import "d.proto" = help: no symbols from this file are referenced warning: unused import "c.proto" - --> a3.proto:5:8 + --> a3.proto:5:1 | 5 | import "c.proto"; - | ^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^ help: delete it | 5 | - import "c.proto"; @@ -43,10 +43,10 @@ warning: unused import "c.proto" = help: no symbols from this file are referenced warning: unused import "c.proto" - --> a4.proto:5:8 + --> a4.proto:5:1 | 5 | import "c.proto"; - | ^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^ help: delete it | 5 | - import "c.proto"; @@ -54,10 +54,10 @@ warning: unused import "c.proto" = help: no symbols from this file are referenced warning: unused import "c.proto" - --> a8.proto:5:8 + --> a8.proto:5:1 | 5 | import "c.proto"; - | ^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^ help: delete it | 5 | - import "c.proto"; diff --git a/experimental/ir/testdata/options/validate/optimize_lite.proto.yaml.stderr.txt b/experimental/ir/testdata/options/validate/optimize_lite.proto.yaml.stderr.txt index 89001861..6810a0fc 100644 --- a/experimental/ir/testdata/options/validate/optimize_lite.proto.yaml.stderr.txt +++ b/experimental/ir/testdata/options/validate/optimize_lite.proto.yaml.stderr.txt @@ -1,8 +1,8 @@ warning: unused import "heavy1.proto" - --> heavy2.proto:4:8 + --> heavy2.proto:4:1 | 4 | import "heavy1.proto"; - | ^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^ help: delete it | 4 | - import "heavy1.proto"; @@ -10,10 +10,10 @@ warning: unused import "heavy1.proto" = help: no symbols from this file are referenced warning: unused import "lite1.proto" - --> heavy2.proto:5:8 + --> heavy2.proto:5:1 | 5 | import "lite1.proto"; - | ^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^ help: delete it | 5 | - import "lite1.proto"; @@ -21,10 +21,10 @@ warning: unused import "lite1.proto" = help: no symbols from this file are referenced warning: unused import "heavy1.proto" - --> lite1.proto:6:8 + --> lite1.proto:6:1 | 6 | import "heavy1.proto"; - | ^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^ help: delete it | 6 | - import "heavy1.proto"; @@ -32,10 +32,10 @@ warning: unused import "heavy1.proto" = help: no symbols from this file are referenced warning: unused import "lite1.proto" - --> lite2.proto:6:8 + --> lite2.proto:6:1 | 6 | import "lite1.proto"; - | ^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^ help: delete it | 6 | - import "lite1.proto";