From 8f1611d7a45f7cbd0a4574beac789c7e22d36aa3 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Thu, 5 Nov 2026 16:02:22 +0000 Subject: [PATCH 1/3] Fix import option as option_dependency --- experimental/ir/ir_imports.go | 50 ++++++-- experimental/ir/ir_symbol.go | 2 +- experimental/ir/ir_test.go | 4 +- experimental/ir/lower_resolve.go | 5 +- .../imports/option.proto.yaml.symtab.yaml | 12 ++ .../testdata/imports/option_public.proto.yaml | 80 +++++++++++++ .../imports/option_public.proto.yaml.fds.yaml | 75 ++++++++++++ .../option_public.proto.yaml.stderr.txt | 39 +++++++ .../option_public.proto.yaml.symtab.yaml | 107 ++++++++++++++++++ 9 files changed, 359 insertions(+), 15 deletions(-) create mode 100644 experimental/ir/testdata/imports/option_public.proto.yaml create mode 100644 experimental/ir/testdata/imports/option_public.proto.yaml.fds.yaml create mode 100644 experimental/ir/testdata/imports/option_public.proto.yaml.stderr.txt create mode 100644 experimental/ir/testdata/imports/option_public.proto.yaml.symtab.yaml diff --git a/experimental/ir/ir_imports.go b/experimental/ir/ir_imports.go index 964a26f5..d479af1b 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 is reachable through an import that is not an option + // import, either directly or because that import re-exports it. Visible + // files that are not dependencies can only be used in options. + // + // This is not the same as option, which records how the import was + // declared. An option import can still be a dependency if a regular + // import also re-exports it. + dependency 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].dependency = 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..0c8f29ec 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.dependency) { 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/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..b6a0ab6b --- /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:8 + | + 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:15 + | + 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 } } From add69786da184f6c6e93ea0794a0604c041500bd Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Tue, 10 Nov 2026 10:27:07 +0100 Subject: [PATCH 2/3] Set optionOnly flag --- experimental/ir/ir_imports.go | 14 +++++++------- experimental/ir/ir_symbol.go | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/experimental/ir/ir_imports.go b/experimental/ir/ir_imports.go index d479af1b..bb88e146 100644 --- a/experimental/ir/ir_imports.go +++ b/experimental/ir/ir_imports.go @@ -126,14 +126,14 @@ type imported struct { // still re-exported. public bool - // Whether this file is reachable through an import that is not an option - // import, either directly or because that import re-exports it. Visible - // files that are not dependencies can only be used in options. + // 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 can still be a dependency if a regular - // import also re-exports it. - dependency bool + // declared. An option import is not option-only if a regular import also + // re-exports it. + optionOnly bool visible, used bool } @@ -191,7 +191,7 @@ func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) { } for n := range i.files { - i.files[n].dependency = dependencies.ContainsID(i.files[n].file.InternedPath()) + i.files[n].optionOnly = !dependencies.ContainsID(i.files[n].file.InternedPath()) } // Now, build the path and causes maps. diff --git a/experimental/ir/ir_symbol.go b/experimental/ir/ir_symbol.go index 0c8f29ec..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.dependency) { + if !imp.visible || (!allowOptions && imp.optionOnly) { return false } From 4db6499080f08885a0299e7b32ce1649538f40d9 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Tue, 10 Nov 2026 10:33:25 +0100 Subject: [PATCH 3/3] Update import delete for whole line --- experimental/ir/lower_validate.go | 85 +++++++++++++------ .../option_public.proto.yaml.stderr.txt | 8 +- .../imports/unused.proto.yaml.stderr.txt | 24 +++--- .../optimize_lite.proto.yaml.stderr.txt | 16 ++-- 4 files changed, 84 insertions(+), 49 deletions(-) diff --git a/experimental/ir/lower_validate.go b/experimental/ir/lower_validate.go index 9f7cb41f..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(), }), @@ -193,9 +193,9 @@ func validateEnum(ty Type, r *report.Report) { validateEnumValueJSON(ty, r) } -// enumValueJSON returns the (pb.enumvalue.json) option on an enum value, and -// its string field. ok is false if the option is not set at all. -func enumValueJSON(m Member) (option, str Value, ok bool) { +// enumValueJSON returns the (pb.enumvalue.json).string option on an enum +// value. An empty (pb.enumvalue.json) = {} is treated as unset. +func enumValueJSON(m Member) Value { ids := &m.Context().session.builtins for option := range m.Options().Fields() { if option.Field().InternedFullName() != ids.EnumValueJSON { @@ -203,12 +203,11 @@ func enumValueJSON(m Member) (option, str Value, ok bool) { } for str := range option.AsMessage().Fields() { if str.Field().InternedFullName() == ids.EnumValueJSONString { - return option, str, true + return str } } - return option, Value{}, true } - return Value{}, Value{}, false + return Value{} } // validateEnumValueJSON validates custom JSON strings set with @@ -216,7 +215,7 @@ func enumValueJSON(m Member) (option, str Value, ok bool) { func validateEnumValueJSON(ty Type, r *report.Report) { custom := false for member := range seq.Values(ty.Members()) { - if _, _, ok := enumValueJSON(member); ok { + if !enumValueJSON(member).IsZero() { custom = true break } @@ -248,16 +247,12 @@ func validateEnumValueJSON(ty Type, r *report.Report) { } for member := range seq.Values(ty.Members()) { - option, str, custom := enumValueJSON(member) - name, hasString := str.AsString() + str := enumValueJSON(member) + name, custom := str.AsString() var span source.Spanner if custom { - span = option.OptionSpan() - if hasString { - span = str.ValueAST() - } - + span = str.ValueAST() if strings.ContainsRune(name, 0) { r.Errorf("custom JSON name cannot contain NUL bytes").Apply( report.Snippet(span), @@ -279,11 +274,10 @@ func validateEnumValueJSON(ty Type, r *report.Report) { if primary, ok := primaries[member.Number()]; !ok { primaries[member.Number()] = member } else { - _, primaryStr, _ := enumValueJSON(primary) - primaryName, primaryHas := primaryStr.AsString() - if primaryHas != hasString || primaryName != name { + primaryName, primaryHas := enumValueJSON(primary).AsString() + if primaryHas != custom || primaryName != name { r.Errorf("aliases must have the same custom JSON name").Apply( - report.Snippetf(member.AST().Name(), "this has %s", describeJSONName(name, hasString)), + report.Snippetf(member.AST().Name(), "this has %s", describeJSONName(name, custom)), report.Snippetf(primary.AST().Name(), "but this has %s", describeJSONName(primaryName, primaryHas)), ) } @@ -608,17 +602,25 @@ func validateExtensionDeclarations(ty Type, r *report.Report) { // First, walk through all of the extension ranges to get their associated // option objects. - options := make(map[MessageValue][]ReservedRange) + rangesByOptions := make(map[MessageValue][]ReservedRange) + var order []MessageValue // For deterministic diagnostics. for r := range seq.Values(ty.ExtensionRanges()) { if r.Options().IsZero() { continue } - mapsx.Append(options, r.Options(), r) + if _, ok := rangesByOptions[r.Options()]; !ok { + order = append(order, r.Options()) + } + mapsx.Append(rangesByOptions, r.Options(), r) } + // Declared names must be unique across all ranges of the message. + declaredNames := make(map[string]Value) + // Now, walk through each grouping of extensions and match up their // declarations. - for options, ranges := range options { + for _, options := range order { + ranges := rangesByOptions[options] rangeSpan := func() source.Span { return source.JoinSeq(iterx.Map(slices.Values(ranges), func(r ReservedRange) source.Span { return r.AST().Span() @@ -657,11 +659,21 @@ func validateExtensionDeclarations(ty Type, r *report.Report) { var haveMissingField bool numbers := make(map[int32]struct{}) + declaredNumbers := make(map[int64]Value) for elem := range seq.Values(decls.Elements()) { decl := elem.AsMessage() number := decl.Field(builtins.ExtnDeclNumber) if n, ok := number.AsInt(); ok { + if prev, dup := declaredNumbers[n]; dup { + r.Errorf("extension declaration number %d is declared multiple times", n).Apply( + report.Snippet(number.ValueAST()), + report.Snippetf(prev.ValueAST(), "previously declared here"), + ) + } else { + declaredNumbers[n] = number + } + // Find the range that contains n. var found bool for _, r := range ranges { @@ -725,8 +737,6 @@ func validateExtensionDeclarations(ty Type, r *report.Report) { return true } - // NOTE: name deduplication needs to wait until global linking, - // similar to extension number deduplication. name := decl.Field(builtins.ExtnDeclName) tyName := decl.Field(builtins.ExtnDeclType) @@ -768,6 +778,17 @@ func validateExtensionDeclarations(ty Type, r *report.Report) { ) haveMissingField = true } + + if s, ok := name.AsString(); ok && !tyName.IsZero() { + if prev, dup := declaredNames[s]; dup { + r.Errorf("extension name `%s` is declared multiple times", s).Apply( + report.Snippet(name.ValueAST()), + report.Snippetf(prev.ValueAST(), "previously declared here"), + ) + } else { + declaredNames[s] = name + } + } } // Generate warnings for each range that is missing at least one value. @@ -797,9 +818,11 @@ func validateDeclaredExtension(m Member, r *report.Report) { extendee := m.Container() var decl MessageValue var elem Element + var rng ReservedRange declSearch: for r := range extendee.Ranges(m.Number()) { - decls := r.AsReserved().Options().Field(builtins.ExtnDecls) + rng = r.AsReserved() + decls := rng.Options().Field(builtins.ExtnDecls) for v := range seq.Values(decls.Elements()) { msg := v.AsMessage() number := msg.Field(builtins.ExtnDeclNumber) @@ -811,6 +834,18 @@ declSearch: } } if decl.IsZero() { + // An extension needs a matching declaration if its range has any + // declarations or is explicitly marked as DECLARATION. + decls := rng.Options().Field(builtins.ExtnDecls) + verification := rng.Options().Field(builtins.ExtnVerification) + if v, _ := verification.AsInt(); !decls.IsZero() || (!verification.IsZero() && v == 0) { + r.Errorf("missing extension declaration for `%s`", m.FullName()).Apply( + report.Snippet(m.AST().Value()), + report.PageBreak, + report.Snippetf(rng.AST(), "requires declarations"), + report.Helpf("an extension range must declare all of its extensions if it is marked `DECLARATION` or declares any extension; otherwise, consider splitting up the range"), + ) + } return // Not a declared extension. } diff --git a/experimental/ir/testdata/imports/option_public.proto.yaml.stderr.txt b/experimental/ir/testdata/imports/option_public.proto.yaml.stderr.txt index b6a0ab6b..e5cf2605 100644 --- a/experimental/ir/testdata/imports/option_public.proto.yaml.stderr.txt +++ b/experimental/ir/testdata/imports/option_public.proto.yaml.stderr.txt @@ -1,8 +1,8 @@ warning: unused import "other.proto" - --> main.proto:4:8 + --> main.proto:4:1 | 4 | import "other.proto"; - | ^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^ help: delete it | 4 | - import "other.proto"; @@ -26,10 +26,10 @@ error: `c.C` is only imported for use in options | warning: unused import "b.proto" - --> other.proto:4:15 + --> other.proto:4:1 | 4 | import option "b.proto"; - | ^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: delete it | 4 | - import option "b.proto"; 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";