From 639eea28204a106de4eec3c4f394349ed280f590 Mon Sep 17 00:00:00 2001 From: Miguel Young de la Sota Date: Fri, 17 Oct 2025 13:58:13 -0700 Subject: [PATCH 1/4] update wkts --- .../testdata/editions/proto2.proto.stderr.txt | 4 +-- .../extend/message_set.proto.stderr.txt | 8 ++--- .../testdata/options/builtin.proto.stderr.txt | 4 +-- .../testdata/options/missing.proto.stderr.txt | 20 ++++++------- .../options/redundant_fqn.proto.stderr.txt | 4 +-- .../google/protobuf/descriptor.proto | 7 +++++ .../google/protobuf/go_features.proto | 29 +++++++++++++++++++ .../google/protobuf/timestamp.proto | 13 +++++---- wellknownimports/update.sh | 20 +++++++++++++ 9 files changed, 83 insertions(+), 26 deletions(-) create mode 100755 wellknownimports/update.sh diff --git a/experimental/ir/testdata/editions/proto2.proto.stderr.txt b/experimental/ir/testdata/editions/proto2.proto.stderr.txt index a629111e0..580f2944c 100644 --- a/experimental/ir/testdata/editions/proto2.proto.stderr.txt +++ b/experimental/ir/testdata/editions/proto2.proto.stderr.txt @@ -64,9 +64,9 @@ warning: redundant custom option setting syntax | | | this field is not a message extension | - ::: google/protobuf/descriptor.proto:801:23 + ::: google/protobuf/descriptor.proto:804:23 | -801 | optional FeatureSet features = 21; +804 | optional FeatureSet features = 21; | -------- | | | field declared inside of `google.protobuf.FieldOptions` here diff --git a/experimental/ir/testdata/extend/message_set.proto.stderr.txt b/experimental/ir/testdata/extend/message_set.proto.stderr.txt index 6a5f0bc4f..cbee5f87f 100644 --- a/experimental/ir/testdata/extend/message_set.proto.stderr.txt +++ b/experimental/ir/testdata/extend/message_set.proto.stderr.txt @@ -24,9 +24,9 @@ warning: redundant custom option setting syntax | | | this field is not a message extension | - ::: google/protobuf/descriptor.proto:600:17 + ::: google/protobuf/descriptor.proto:603:17 | -600 | optional bool message_set_wire_format = 1 [default = false]; +603 | optional bool message_set_wire_format = 1 [default = false]; | ----------------------- | | | field declared inside of `google.protobuf.MessageOptions` here @@ -78,9 +78,9 @@ warning: redundant custom option setting syntax | | | this field is not a message extension | - ::: google/protobuf/descriptor.proto:600:17 + ::: google/protobuf/descriptor.proto:603:17 | -600 | optional bool message_set_wire_format = 1 [default = false]; +603 | optional bool message_set_wire_format = 1 [default = false]; | ----------------------- | | | field declared inside of `google.protobuf.MessageOptions` here diff --git a/experimental/ir/testdata/options/builtin.proto.stderr.txt b/experimental/ir/testdata/options/builtin.proto.stderr.txt index 15ed874da..bd50a4514 100644 --- a/experimental/ir/testdata/options/builtin.proto.stderr.txt +++ b/experimental/ir/testdata/options/builtin.proto.stderr.txt @@ -6,9 +6,9 @@ error: option `deprecated` set multiple times 25 | option deprecated = true; | ^^^^^^^^^^ ... also set here | - ::: google/protobuf/descriptor.proto:611:17 + ::: google/protobuf/descriptor.proto:614:17 | -611 | optional bool deprecated = 3 [default = false]; +614 | optional bool deprecated = 3 [default = false]; | ---------- not a repeated field | = note: a non-`repeated` option may be set at most once diff --git a/experimental/ir/testdata/options/missing.proto.stderr.txt b/experimental/ir/testdata/options/missing.proto.stderr.txt index 894c50aff..f6581b781 100644 --- a/experimental/ir/testdata/options/missing.proto.stderr.txt +++ b/experimental/ir/testdata/options/missing.proto.stderr.txt @@ -12,9 +12,9 @@ error: expected singular message, found scalar type `string` | | | found scalar type `string` | - ::: google/protobuf/descriptor.proto:504:12 + ::: google/protobuf/descriptor.proto:507:12 | -504 | optional string go_package = 11; +507 | optional string go_package = 11; | ------ type specified here error: cannot find message field `unknown` in `google.protobuf.MessageOptions` @@ -31,9 +31,9 @@ error: expected singular message, found scalar type `bool` | | | found scalar type `bool` | - ::: google/protobuf/descriptor.proto:611:12 + ::: google/protobuf/descriptor.proto:614:12 | -611 | optional bool deprecated = 3 [default = false]; +614 | optional bool deprecated = 3 [default = false]; | ---- type specified here error: cannot find message field `unknown` in `google.protobuf.FieldOptions` @@ -52,9 +52,9 @@ error: expected singular message, found scalar type `bool` | | | field selector requires singular message | - ::: google/protobuf/descriptor.proto:743:12 + ::: google/protobuf/descriptor.proto:746:12 | -743 | optional bool lazy = 5 [default = false]; +746 | optional bool lazy = 5 [default = false]; | ---- type specified here error: expected singular message, found scalar type `int32` @@ -87,9 +87,9 @@ error: expected singular message, found scalar type `bool` | | | found scalar type `bool` | - ::: google/protobuf/descriptor.proto:859:12 + ::: google/protobuf/descriptor.proto:866:12 | -859 | optional bool deprecated = 3 [default = false]; +866 | optional bool deprecated = 3 [default = false]; | ---- type specified here error: cannot find message field `unknown` in `google.protobuf.EnumValueOptions` @@ -108,9 +108,9 @@ error: expected singular message, found scalar type `bool` | | | field selector requires singular message | - ::: google/protobuf/descriptor.proto:889:12 + ::: google/protobuf/descriptor.proto:896:12 | -889 | optional bool deprecated = 1 [default = false]; +896 | optional bool deprecated = 1 [default = false]; | ---- type specified here encountered 12 errors diff --git a/experimental/ir/testdata/options/redundant_fqn.proto.stderr.txt b/experimental/ir/testdata/options/redundant_fqn.proto.stderr.txt index 96d8e2fd2..629db3b13 100644 --- a/experimental/ir/testdata/options/redundant_fqn.proto.stderr.txt +++ b/experimental/ir/testdata/options/redundant_fqn.proto.stderr.txt @@ -6,9 +6,9 @@ warning: redundant custom option setting syntax | | | this field is not a message extension | - ::: google/protobuf/descriptor.proto:611:17 + ::: google/protobuf/descriptor.proto:614:17 | -611 | optional bool deprecated = 3 [default = false]; +614 | optional bool deprecated = 3 [default = false]; | ---------- | | | field declared inside of `google.protobuf.MessageOptions` here diff --git a/wellknownimports/google/protobuf/descriptor.proto b/wellknownimports/google/protobuf/descriptor.proto index 8849ae761..620c707ca 100644 --- a/wellknownimports/google/protobuf/descriptor.proto +++ b/wellknownimports/google/protobuf/descriptor.proto @@ -398,6 +398,9 @@ message ServiceDescriptorProto { repeated MethodDescriptorProto method = 2; optional ServiceOptions options = 3; + + reserved 4; + reserved "stream"; } // Describes a method of a service. @@ -819,6 +822,10 @@ message FieldOptions { // this one, the last default assigned will be used, and proto files will // not be able to override it. optional Edition edition_removed = 4; + + // The removal error text if this feature is used after the edition it was + // removed in. + optional string removal_error = 5; } optional FeatureSupport feature_support = 22; diff --git a/wellknownimports/google/protobuf/go_features.proto b/wellknownimports/google/protobuf/go_features.proto index 4d13a4131..13819acd3 100644 --- a/wellknownimports/google/protobuf/go_features.proto +++ b/wellknownimports/google/protobuf/go_features.proto @@ -80,4 +80,33 @@ message GoFeatures { value: "STRIP_ENUM_PREFIX_KEEP" } ]; + + // Wrap the OptimizeMode enum in a message for scoping: + // This way, users can type shorter names (SPEED, CODE_SIZE). + message OptimizeModeFeature { + // The name of this enum matches OptimizeMode in descriptor.proto. + enum OptimizeMode { + // OPTIMIZE_MODE_UNSPECIFIED results in falling back to the default + // (optimize for code size), but needs to be a separate value to distinguish + // between an explicitly set optimize mode or a missing optimize mode. + OPTIMIZE_MODE_UNSPECIFIED = 0; + SPEED = 1; + CODE_SIZE = 2; + // There is no enum entry for LITE_RUNTIME (descriptor.proto), + // because Go Protobuf does not have the concept of a lite runtime. + } + } + + optional OptimizeModeFeature.OptimizeMode optimize_mode = 4 [ + retention = RETENTION_RUNTIME, + targets = TARGET_TYPE_MESSAGE, + targets = TARGET_TYPE_FILE, + feature_support = { + edition_introduced: EDITION_2024, + }, + edition_defaults = { + edition: EDITION_LEGACY, + value: "OPTIMIZE_MODE_UNSPECIFIED" + } + ]; } diff --git a/wellknownimports/google/protobuf/timestamp.proto b/wellknownimports/google/protobuf/timestamp.proto index fd0bc07dc..17039620f 100644 --- a/wellknownimports/google/protobuf/timestamp.proto +++ b/wellknownimports/google/protobuf/timestamp.proto @@ -131,14 +131,15 @@ option csharp_namespace = "Google.Protobuf.WellKnownTypes"; // ) to obtain a formatter capable of generating timestamps in this format. // message Timestamp { - // Represents seconds of UTC time since Unix epoch - // 1970-01-01T00:00:00Z. Must be from 0001-01-01T00:00:00Z to - // 9999-12-31T23:59:59Z inclusive. + // Represents seconds of UTC time since Unix epoch 1970-01-01T00:00:00Z. Must + // be between -62135596800 and 253402300799 inclusive (which corresponds to + // 0001-01-01T00:00:00Z to 9999-12-31T23:59:59Z). int64 seconds = 1; - // Non-negative fractions of a second at nanosecond resolution. Negative - // second values with fractions must still have non-negative nanos values - // that count forward in time. Must be from 0 to 999,999,999 + // Non-negative fractions of a second at nanosecond resolution. This field is + // the nanosecond portion of the duration, not an alternative to seconds. + // Negative second values with fractions must still have non-negative nanos + // values that count forward in time. Must be between 0 and 999,999,999 // inclusive. int32 nanos = 2; } diff --git a/wellknownimports/update.sh b/wellknownimports/update.sh new file mode 100755 index 000000000..80a9cd8d6 --- /dev/null +++ b/wellknownimports/update.sh @@ -0,0 +1,20 @@ +#!/bin/bash + +# Updates the WKT files from the GitHub source of truth. + +set -e + +RAW_URL="https://raw.githubusercontent.com/protocolbuffers/protobuf/main" + +cd "$(dirname "$0")" +for WKT in $(find . -name '*.proto'); do + WKT="${WKT#"./"}" + case $WKT in + "google/protobuf/go_features.proto") SRC="go/$WKT" ;; + "google/protobuf/java_features.proto") SRC="java/core/src/main/resources/$WKT" ;; + *) SRC="src/$WKT" ;; + esac + + echo "$RAW_URL/$SRC -> $WKT" + curl "$RAW_URL/$SRC" > "$WKT" -f +done From 2bcfd8bcb181ebfada0011bcd15cd596ee1bfdd0 Mon Sep 17 00:00:00 2001 From: Miguel Young de la Sota Date: Fri, 17 Oct 2025 15:47:31 -0700 Subject: [PATCH 2/4] implement import option --- experimental/ast/syntax/name.go | 27 ++++ .../internal/erredition/erredition.go | 99 +++++++++++++++ experimental/internal/erredition/normalize.go | 43 +++++++ .../internal/erredition/normalize_test.go | 16 +++ experimental/ir/fdp.go | 16 ++- experimental/ir/ir_imports.go | 42 ++++--- experimental/ir/ir_symbol.go | 28 ++++- experimental/ir/ir_test.go | 15 +-- experimental/ir/lower_features.go | 118 +++--------------- experimental/ir/lower_imports.go | 1 + experimental/ir/lower_options.go | 5 +- experimental/ir/lower_resolve.go | 28 ++++- experimental/ir/lower_symbols.go | 2 +- experimental/ir/lower_validate.go | 46 +++---- .../editions/inheritance.proto.stderr.txt | 5 +- .../editions/lifetime.proto.stderr.txt | 2 +- .../testdata/imports/ok.proto.yaml.fds.yaml | 2 +- .../testdata/imports/ok.proto.yaml.stderr.txt | 12 +- .../ir/testdata/imports/option.proto.yaml | 66 ++++++++++ .../imports/option.proto.yaml.fds.yaml | 58 +++++++++ .../imports/option.proto.yaml.stderr.txt | 55 ++++++++ .../imports/option.proto.yaml.symtab.yaml | 62 +++++++++ experimental/parser/legalize_file.go | 44 +++++-- experimental/parser/parse_state.go | 7 +- .../parser/import/modifiers.proto.stderr.txt | 52 ++++++-- .../parser/import/ok.proto.stderr.txt | 22 +++- .../parser/import/options.proto.stderr.txt | 11 +- .../parser/testdata/parser/import/order.proto | 24 ++++ .../parser/import/order.proto.stderr.txt | 29 +++++ .../testdata/parser/import/order.proto.yaml | 18 +++ .../gen/buf/compiler/v1alpha1/symtab.pb.go | 14 ++- .../proto/buf/compiler/v1alpha1/symtab.proto | 1 + 32 files changed, 765 insertions(+), 205 deletions(-) create mode 100644 experimental/ast/syntax/name.go create mode 100644 experimental/internal/erredition/erredition.go create mode 100644 experimental/internal/erredition/normalize.go create mode 100644 experimental/internal/erredition/normalize_test.go create mode 100644 experimental/ir/testdata/imports/option.proto.yaml create mode 100644 experimental/ir/testdata/imports/option.proto.yaml.fds.yaml create mode 100644 experimental/ir/testdata/imports/option.proto.yaml.stderr.txt create mode 100644 experimental/ir/testdata/imports/option.proto.yaml.symtab.yaml create mode 100644 experimental/parser/testdata/parser/import/order.proto create mode 100644 experimental/parser/testdata/parser/import/order.proto.stderr.txt create mode 100644 experimental/parser/testdata/parser/import/order.proto.yaml diff --git a/experimental/ast/syntax/name.go b/experimental/ast/syntax/name.go new file mode 100644 index 000000000..c2705e5bb --- /dev/null +++ b/experimental/ast/syntax/name.go @@ -0,0 +1,27 @@ +package syntax + +import ( + "fmt" + "strconv" +) + +var names = func() map[Syntax]string { + names := make(map[Syntax]string) + for syntax := range All() { + if syntax.IsEdition() { + names[syntax] = fmt.Sprintf("Edition %s", syntax) + } else { + names[syntax] = strconv.Quote(syntax.String()) + } + } + return names +}() + +// Name returns the name of this syntax as it should appear in diagnostics. +func (s Syntax) Name() string { + name, ok := names[s] + if !ok { + return "Edition " + } + return name +} diff --git a/experimental/internal/erredition/erredition.go b/experimental/internal/erredition/erredition.go new file mode 100644 index 000000000..6ffe82214 --- /dev/null +++ b/experimental/internal/erredition/erredition.go @@ -0,0 +1,99 @@ +// Package erredition defines common diagnostics for issuing errors about +// the wrong edition being used. +package erredition + +import ( + "github.com/bufbuild/protocompile/experimental/ast" + "github.com/bufbuild/protocompile/experimental/ast/syntax" + "github.com/bufbuild/protocompile/experimental/report" +) + +// TooNew diagnoses an edition that is too old for the feature used. +type TooOld struct { + Current syntax.Syntax + Decl ast.DeclSyntax + Intro syntax.Syntax + + What any + Where report.Spanner +} + +// Diagnose implements [report.Diagnoser]. +func (e TooOld) Diagnose(d *report.Diagnostic) { + kind := "syntax" + if e.Current.IsEdition() { + kind = "edition" + } + + d.Apply( + report.Message("`%s` is not supported in %s", e.What, e.Current.Name()), + report.Snippet(e.Where), + report.Snippetf(e.Decl.Value(), "%s specified here", kind), + ) + + if e.Intro != syntax.Unknown { + d.Apply(report.Helpf("`%s` requires at least %s", e.What, e.Intro.Name())) + } +} + +// TooNew diagnoses an edition that is too new for the feature used. +type TooNew struct { + Current syntax.Syntax + Decl ast.DeclSyntax + + Deprecated, Removed syntax.Syntax + DeprecatedReason, RemovedReason string + + What any + Where report.Spanner +} + +// Diagnose implements [report.Diagnoser]. +func (e TooNew) Diagnose(d *report.Diagnostic) { + kind := "syntax" + if e.Current.IsEdition() { + kind = "edition" + } + + err := "not supported" + if !e.isRemoved() { + err = "deprecated" + } + + d.Apply( + report.Message("`%s` is %s in %s", e.What, err, e.Current.Name()), + report.Snippet(e.Where), + report.Snippetf(e.Decl.Value(), "%s specified here", kind), + ) + + if e.isRemoved() { + if e.isDeprecated() { + d.Apply(report.Helpf("deprecated since %s, removed in %s", e.Deprecated.Name(), e.Removed.Name())) + } else { + d.Apply(report.Helpf("removed in %s", e.Removed.Name())) + } + + if e.RemovedReason != "" { + d.Apply(report.Helpf("%s", normalizeReason(e.RemovedReason))) + return + } + } else if e.isDeprecated() { + if e.Removed != syntax.Unknown { + d.Apply(report.Helpf("deprecated since %s, to be removed in %s", e.Deprecated.Name(), e.Removed.Name())) + } else { + d.Apply(report.Helpf("deprecated since %s", e.Deprecated.Name())) + } + } + + if e.DeprecatedReason != "" { + d.Apply(report.Helpf("%s", normalizeReason(e.DeprecatedReason))) + } +} + +func (e TooNew) isDeprecated() bool { + return e.Deprecated != syntax.Unknown && e.Deprecated <= e.Current +} + +func (e TooNew) isRemoved() bool { + return e.Removed != syntax.Unknown && e.Removed <= e.Current +} diff --git a/experimental/internal/erredition/normalize.go b/experimental/internal/erredition/normalize.go new file mode 100644 index 000000000..4d80c8d5c --- /dev/null +++ b/experimental/internal/erredition/normalize.go @@ -0,0 +1,43 @@ +package erredition + +import ( + "regexp" + "strings" + "unicode" + "unicode/utf8" +) + +var ( + whitespacePattern = regexp.MustCompile(`[ \t\r\n]+`) + protoDevPattern = regexp.MustCompile(` See http:\/\/protobuf\.dev\/[^ ]+ for more information\.?`) + periodPattern = regexp.MustCompile(`\.( [A-Z]|$)`) + editionPattern = regexp.MustCompile(`edition [0-9]+`) +) + +// normalizeReason canonicalizes the appearance of deprecation reasons. +// Some built-in deprecation warnings have double spaces after periods. +func normalizeReason(text string) string { + // First, normalize all whitespace. + text = whitespacePattern.ReplaceAllString(text, " ") + + // Delete protobuf.dev links; these should ideally use our specialized link + // formatting instead. + text = protoDevPattern.ReplaceAllString(text, "") + + // Replace all sentence-ending periods with semicolons. + text = periodPattern.ReplaceAllStringFunc(text, func(match string) string { + if match == "." { + return "" + } + return ";" + strings.ToLower(match[1:]) + }) + + // Capitalize "Edition" when followed by an edition number. + text = editionPattern.ReplaceAllStringFunc(text, func(s string) string { + return "E" + s[1:] + }) + + // Finally, de-capitalize the first letter. + r, n := utf8.DecodeRuneInString(text) + return string(unicode.ToLower(r)) + text[n:] +} diff --git a/experimental/internal/erredition/normalize_test.go b/experimental/internal/erredition/normalize_test.go new file mode 100644 index 000000000..223dc17da --- /dev/null +++ b/experimental/internal/erredition/normalize_test.go @@ -0,0 +1,16 @@ +package erredition + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNormalize(t *testing.T) { + t.Parallel() + + assert.Equal(t, + "the legacy closed enum behavior in Java is deprecated and is scheduled to be removed in Edition 2025", + normalizeReason("The legacy closed enum behavior in Java is deprecated and is scheduled to be removed in edition 2025. See http://protobuf.dev/programming-guides/enum/#java for more information."), + ) +} diff --git a/experimental/ir/fdp.go b/experimental/ir/fdp.go index bd4735db3..b0f8387fd 100644 --- a/experimental/ir/fdp.go +++ b/experimental/ir/fdp.go @@ -141,12 +141,16 @@ func (dg *descGenerator) file(file File, fdp *descriptorpb.FileDescriptorProto) return imp.Decl.KeywordToken().Span().Start })) for i, imp := range imports { - fdp.Dependency = append(fdp.Dependency, imp.Path()) - if imp.Public { - fdp.PublicDependency = append(fdp.PublicDependency, int32(i)) - } - if imp.Weak { - fdp.WeakDependency = append(fdp.WeakDependency, int32(i)) + if !imp.Option { + fdp.Dependency = append(fdp.Dependency, imp.Path()) + if imp.Public { + fdp.PublicDependency = append(fdp.PublicDependency, int32(i)) + } + if imp.Weak { + fdp.WeakDependency = append(fdp.WeakDependency, int32(i)) + } + } else if imp.Option { + fdp.OptionDependency = append(fdp.OptionDependency, imp.Path()) } if dg.sourceCodeInfoExtn != nil && !imp.Used { diff --git a/experimental/ir/ir_imports.go b/experimental/ir/ir_imports.go index 83f4e6d6b..0e8762164 100644 --- a/experimental/ir/ir_imports.go +++ b/experimental/ir/ir_imports.go @@ -26,12 +26,15 @@ import ( // Import is an import in a [File]. type Import struct { - File // The file that is imported. - Public, Weak bool // The kind of import this is. - Direct bool // Whether this is a direct or transitive import. - Visible bool // Whether this import's symbols are visible in the current file. - Used bool // Whether this import has been marked as used. - Decl ast.DeclImport // The import declaration. + File // The file that is imported. + + // The kind of import this is. + Public, Weak, Option bool + + Direct bool // Whether this is a direct or transitive import. + Visible bool // Whether this import's symbols are visible in the current file. + Used bool // Whether this import has been marked as used. + Decl ast.DeclImport // The import declaration. } // imports is a data structure for compactly classifying the transitive imports @@ -100,27 +103,23 @@ type imports struct { // // There is a test in ir_imports_test.go that validates this behavior. So // much pain for a little-used feature... - publicEnd, weakEnd, importEnd, transPublicEnd uint32 + publicEnd, importEnd, transPublicEnd uint32 } // imported wraps an imported [File] and the import statement declaration [ast.DeclImport]. type imported struct { file File decl ast.DeclImport + weak, option bool visible, used bool } // Append appends a direct import to this imports table. func (i *imports) AddDirect(imp Import) { - switch { - case imp.Public: + if imp.Public { i.Insert(imp, int(i.publicEnd), true) i.publicEnd++ - i.weakEnd++ - case imp.Weak: - i.Insert(imp, int(i.weakEnd), true) - i.weakEnd++ - default: + } else { i.Insert(imp, int(i.importEnd), true) } @@ -133,9 +132,15 @@ func (i *imports) AddDirect(imp Import) { // // Must only be called once, after all direct imports are added. func (i *imports) Recurse(dedup intern.Map[ast.DeclImport]) { - for _, file := range seq.All(i.Directs()) { + for k, file := range seq.All(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 } @@ -179,6 +184,8 @@ func (i *imports) Insert(imp Import, pos int, visible bool) { i.files = slices.Insert(i.files, pos, imported{ file: imp.File, decl: imp.Decl, + weak: imp.Weak, + option: imp.Option, visible: visible, }) } @@ -207,7 +214,8 @@ func (i *imports) Directs() seq.Indexer[Import] { return Import{ File: imported.file, Public: public, - Weak: !public && n < i.weakEnd, + Weak: imported.weak, + Option: imported.option, Direct: true, Visible: true, Decl: imported.decl, @@ -221,7 +229,7 @@ func (i *imports) Directs() seq.Indexer[Import] { // Transitive returns an indexer over the Transitive imports. // -// This function does not report whether those imports are weak or used. +// This function does not report whether those imports are weak, option, or used. func (i *imports) Transitive() seq.Indexer[Import] { return seq.NewFixedSlice( i.files[:max(0, len(i.files)-1)], // Exclude the implicit descriptor.proto. diff --git a/experimental/ir/ir_symbol.go b/experimental/ir/ir_symbol.go index f3ab5079a..153ae10a5 100644 --- a/experimental/ir/ir_symbol.go +++ b/experimental/ir/ir_symbol.go @@ -20,6 +20,7 @@ import ( "slices" "sync" + "github.com/bufbuild/protocompile/experimental/ast" "github.com/bufbuild/protocompile/experimental/internal" "github.com/bufbuild/protocompile/experimental/internal/taxa" "github.com/bufbuild/protocompile/experimental/report" @@ -180,10 +181,15 @@ func (s Symbol) Deprecated() Value { // Visible returns whether or not this symbol is visible according to Protobuf's // import semantics, within s.Context().File(). -func (s Symbol) Visible() bool { - return s.ref.file <= 0 || - s.Kind() == SymbolKindPackage || // Packages don't get visibility checks. - s.Context().imports.files[uint(s.ref.file)-1].visible +// +// If allowOptions is true, symbols that were pulled in via import option are +// accepted. +func (s Symbol) Visible(allowOptions bool) bool { + if s.ref.file <= 0 || s.Kind() == SymbolKindPackage { + return true + } + file := s.Context().imports.files[uint(s.ref.file)-1] + return file.visible && (allowOptions || !file.option) } // Definition returns a span for the definition site of this symbol; @@ -212,6 +218,18 @@ func (s Symbol) Definition() report.Span { return report.Span{} } +// Import returns the import declaration that brought this symbol into scope. +// +// Returns zero of s is defined in the current file. +func (s Symbol) Import() ast.DeclImport { + if s.ref.file <= 0 { + return ast.DeclImport{} + } + + file := s.Context().imports.files[uint(s.ref.file)-1] + return file.decl +} + // noun returns a [taxa.Noun] for diagnostic use. func (s Symbol) noun() taxa.Noun { return s.Kind().noun() @@ -500,7 +518,7 @@ again: if !r.ptr.Nil() { found = r sym := wrapSymbol(c, r) - if sym.Visible() && accept(sym.Kind()) { + if sym.Visible(true) && accept(sym.Kind()) { // If the symbol is not visible, keep looking; we may find // another match that is actually visible. break diff --git a/experimental/ir/ir_test.go b/experimental/ir/ir_test.go index 845c7ca48..156aabfc0 100644 --- a/experimental/ir/ir_test.go +++ b/experimental/ir/ir_test.go @@ -352,13 +352,14 @@ func symtabProto(files []ir.File) *compilerpb.SymbolSet { } symtab.Symbols = append(symtab.Symbols, &compilerpb.Symbol{ - Fqn: string(sym.FullName()), - Kind: compilerpb.Symbol_Kind(sym.Kind()), - File: sym.File().Path(), - Index: uint32(sym.RawData()), - Visible: sym.Kind() != ir.SymbolKindPackage && sym.Visible(), - Options: new(optionWalker).message(options), - Features: dumpFeatures(sym.FeatureSet(), sym.Kind().OptionTarget()), + Fqn: string(sym.FullName()), + Kind: compilerpb.Symbol_Kind(sym.Kind()), + File: sym.File().Path(), + Index: uint32(sym.RawData()), + Visible: sym.Kind() != ir.SymbolKindPackage && sym.Visible(false), + OptionOnly: sym.Kind() != ir.SymbolKindPackage && !sym.Visible(true) && sym.Visible(true), + Options: new(optionWalker).message(options), + Features: dumpFeatures(sym.FeatureSet(), sym.Kind().OptionTarget()), }) } slices.SortFunc(symtab.Symbols, diff --git a/experimental/ir/lower_features.go b/experimental/ir/lower_features.go index 305ddfec0..cef287321 100644 --- a/experimental/ir/lower_features.go +++ b/experimental/ir/lower_features.go @@ -16,19 +16,16 @@ package ir import ( "cmp" - "fmt" - "regexp" "slices" "github.com/bufbuild/protocompile/experimental/ast/predeclared" "github.com/bufbuild/protocompile/experimental/ast/syntax" + "github.com/bufbuild/protocompile/experimental/internal/erredition" "github.com/bufbuild/protocompile/experimental/report" "github.com/bufbuild/protocompile/experimental/seq" "github.com/bufbuild/protocompile/internal/ext/slicesx" ) -var whitespacePattern = regexp.MustCompile(`[ \t\r\n]+`) - func buildAllFeatureInfo(f File, r *report.Report) { for m := range f.AllMembers() { if !m.IsEnumValue() { @@ -376,109 +373,24 @@ func validateFeatures(features MessageValue, r *report.Report) { // any relationship between these. switch { case info.IsRemoved(edition), info.IsDeprecated(edition): - r.SoftError(info.IsRemoved(edition), errEditionTooNew{ - file: features.Context().File(), - removed: info.Removed(), - deprecated: info.Deprecated(), - warning: info.DeprecationWarning(), - what: feature.Field().Name(), - where: feature.KeyAST(), + r.SoftError(info.IsRemoved(edition), erredition.TooNew{ + Current: features.Context().File().Syntax(), + Decl: features.Context().File().AST().Syntax(), + Removed: info.Removed(), + Deprecated: info.Deprecated(), + DeprecatedReason: info.DeprecationWarning(), + What: feature.Field().Name(), + Where: feature.KeyAST(), }) case !info.IsIntroduced(edition): - r.Error(errEditionTooOld{ - file: features.Context().File(), - intro: info.Introduced(), - what: feature.Field().Name(), - where: feature.KeyAST(), + r.Error(erredition.TooOld{ + Current: features.Context().File().Syntax(), + Decl: features.Context().File().AST().Syntax(), + Intro: info.Introduced(), + What: feature.Field().Name(), + Where: feature.KeyAST(), }) } } } - -func prettyEdition(s syntax.Syntax) string { - if !s.IsValid() || !s.IsEdition() { - return fmt.Sprintf("\"%s\"", s) - } - return fmt.Sprintf("Edition %s", s) -} - -type errEditionTooOld struct { - file File - intro syntax.Syntax - - what any - where report.Spanner -} - -func (e errEditionTooOld) Diagnose(d *report.Diagnostic) { - kind := "syntax" - if e.file.Syntax().IsEdition() { - kind = "edition" - } - - d.Apply( - report.Message("`%s` is not supported in %s", e.what, prettyEdition(e.file.Syntax())), - report.Snippet(e.where), - report.Snippetf(e.file.AST().Syntax().Value(), "%s specified here", kind), - ) - - if e.intro != syntax.Unknown { - d.Apply(report.Helpf("`%s` requires at least %s", e.what, prettyEdition(e.intro))) - } -} - -type errEditionTooNew struct { - file File - deprecated, removed syntax.Syntax - warning string - - what any - where report.Spanner -} - -func (e errEditionTooNew) Diagnose(d *report.Diagnostic) { - kind := "syntax" - if e.file.Syntax().IsEdition() { - kind = "edition" - } - - err := "not supported" - if !e.isRemoved() { - err = "deprecated" - } - - d.Apply( - report.Message("`%s` is %s in %s", e.what, err, prettyEdition(e.file.Syntax())), - report.Snippet(e.where), - report.Snippetf(e.file.AST().Syntax().Value(), "%s specified here", kind), - ) - - if e.isRemoved() { - if e.isDeprecated() { - d.Apply(report.Helpf("deprecated since %s, removed in %s", prettyEdition(e.deprecated), prettyEdition(e.removed))) - } else { - d.Apply(report.Helpf("removed in %s", prettyEdition(e.removed))) - } - } else if e.isDeprecated() { - if e.removed != syntax.Unknown { - d.Apply(report.Helpf("deprecated since %s, to be removed in %s", prettyEdition(e.deprecated), prettyEdition(e.removed))) - } else { - d.Apply(report.Helpf("deprecated since %s", prettyEdition(e.deprecated))) - } - } - - if e.warning != "" { - // Canonicalize whitespace. Some built-in deprecation warnings have - // double spaces after periods. - text := whitespacePattern.ReplaceAllString(e.warning, " ") - d.Apply(report.Helpf("deprecated: %s", text)) - } -} - -func (e errEditionTooNew) isDeprecated() bool { - return e.deprecated != syntax.Unknown && e.deprecated <= e.file.Syntax() -} -func (e errEditionTooNew) isRemoved() bool { - return e.removed != syntax.Unknown && e.removed <= e.file.Syntax() -} diff --git a/experimental/ir/lower_imports.go b/experimental/ir/lower_imports.go index c93c74b56..d8e77ec12 100644 --- a/experimental/ir/lower_imports.go +++ b/experimental/ir/lower_imports.go @@ -100,6 +100,7 @@ func buildImports(f File, r *report.Report, importer Importer) { File: file, Public: imp.IsPublic(), Weak: imp.IsWeak(), + Option: imp.IsOption(), Decl: imp, }) } diff --git a/experimental/ir/lower_options.go b/experimental/ir/lower_options.go index 7c4547beb..afa7c3c9b 100644 --- a/experimental/ir/lower_options.go +++ b/experimental/ir/lower_options.go @@ -364,7 +364,7 @@ func validateOptionTargetsInValue(m MessageValue, decl report.Span, target Optio } } -// symbolRef is all of the information necessary to resolve an option reference. +// optionRef is all of the information necessary to resolve an option reference. type optionRef struct { *Context *report.Report @@ -431,6 +431,7 @@ func (r optionRef) resolve() { want: taxa.Extension, allowScalars: false, + allowOption: true, suggestImport: true, }.resolve() @@ -504,7 +505,7 @@ func (r optionRef) resolve() { ids.MessageFeatures, ids.FieldFeatures, ids.OneofFeatures, ids.EnumFeatures, ids.EnumValueFeatures: if syn := r.File().Syntax(); !syn.IsEdition() { - r.Errorf("`features` cannot be set in %s", prettyEdition(syn)).Apply( + r.Errorf("`features` cannot be set in %s", syn.Name()).Apply( report.Snippet(pc), report.Snippetf(r.File().AST().Syntax().Value(), "syntax specified here"), ) diff --git a/experimental/ir/lower_resolve.go b/experimental/ir/lower_resolve.go index 95454fb57..866f07f5d 100644 --- a/experimental/ir/lower_resolve.go +++ b/experimental/ir/lower_resolve.go @@ -24,6 +24,7 @@ import ( "github.com/bufbuild/protocompile/experimental/ir/presence" "github.com/bufbuild/protocompile/experimental/report" "github.com/bufbuild/protocompile/experimental/seq" + "github.com/bufbuild/protocompile/experimental/token" "github.com/bufbuild/protocompile/experimental/token/keyword" "github.com/bufbuild/protocompile/internal/arena" "github.com/bufbuild/protocompile/internal/ext/iterx" @@ -246,6 +247,9 @@ type symbolRef struct { // If true, diagnostics will not suggest adding an import. suggestImport bool + + // Allow pulling in symbols via import option. + allowOption bool } // resolve performs symbol resolution. @@ -328,7 +332,29 @@ func (r symbolRef) diagnoseLookup(sym Symbol, expectedName FullName) *report.Dia "rather than the one we found", expectedName), ) - case !sym.Visible(): + case !sym.Visible(r.allowOption): + if !r.allowOption && sym.Visible(true) { + decl := sym.Import() + var option token.Token + for m := range seq.Values(decl.ModifierTokens()) { + if m.Keyword() == keyword.Option { + option = m + } + } + span := report.Join(decl.KeywordToken(), option) + + // This symbol is only visible in option position. + r.Errorf("`%s` is only imported for use in options", r.name).Apply( + report.Snippetf(r.span, "requires non-`option` import"), + report.Snippetf(decl, "imported as `option` here"), + report.SuggestEdits(span, "delete `option`", report.Edit{ + Start: 0, End: span.Len(), + Replace: "import", + }), + ) + break + } + // Complain that we need to import a symbol. d := r.Errorf("cannot find `%s` in this scope", r.name).Apply( report.Snippetf(r.span, "not visible in this scope"), diff --git a/experimental/ir/lower_symbols.go b/experimental/ir/lower_symbols.go index eea4ba4e3..163d55149 100644 --- a/experimental/ir/lower_symbols.go +++ b/experimental/ir/lower_symbols.go @@ -343,7 +343,7 @@ func (e errDuplicates) Diagnose(d *report.Diagnostic) { // that symbol names are global! for i := range e.refs { s := e.symbol(i) - if s.Visible() { + if s.Visible(true) { continue } diff --git a/experimental/ir/lower_validate.go b/experimental/ir/lower_validate.go index 89e5a77e4..414d91871 100644 --- a/experimental/ir/lower_validate.go +++ b/experimental/ir/lower_validate.go @@ -27,6 +27,7 @@ import ( "github.com/bufbuild/protocompile/experimental/ast/predeclared" "github.com/bufbuild/protocompile/experimental/ast/syntax" "github.com/bufbuild/protocompile/experimental/internal" + "github.com/bufbuild/protocompile/experimental/internal/erredition" "github.com/bufbuild/protocompile/experimental/internal/taxa" "github.com/bufbuild/protocompile/experimental/ir/presence" "github.com/bufbuild/protocompile/experimental/report" @@ -688,12 +689,13 @@ func validatePacked(m Member, r *report.Report) { if !packed { want = "EXPANDED" } - r.Error(errEditionTooNew{ - file: m.Context().File(), - removed: syntax.Edition2023, + r.Error(erredition.TooNew{ + Current: m.Context().File().Syntax(), + Decl: m.Context().File().AST().Syntax(), + Removed: syntax.Edition2023, - what: option.Field().Name(), - where: option.KeyAST(), + What: option.Field().Name(), + Where: option.KeyAST(), }).Apply(option.suggestEdit( builtins.FeaturePacked.Name(), want, "replace with `%s`", builtins.FeaturePacked.Name(), @@ -806,13 +808,14 @@ func validateCType(m Member, r *report.Report) { is2023 := f.Syntax() == syntax.Edition2023 switch { case f.Syntax() > syntax.Edition2023: - r.Error(errEditionTooNew{ - file: f, - deprecated: syntax.Edition2023, - removed: syntax.Edition2024, - - what: ctype.Field().Name(), - where: ctype.KeyAST(), + r.Error(erredition.TooNew{ + Current: m.Context().File().Syntax(), + Decl: m.Context().File().AST().Syntax(), + Deprecated: syntax.Edition2023, + Removed: syntax.Edition2024, + + What: ctype.Field().Name(), + Where: ctype.KeyAST(), }).Apply(ctype.suggestEdit( "features.(pb.cpp).string_type", want, "replace with `features.(pb.cpp).string_type`", @@ -828,7 +831,7 @@ func validateCType(m Member, r *report.Report) { ) if !is2023 { - d.Apply(report.Helpf("this becomes a hard error in %s", prettyEdition(syntax.Edition2023))) + d.Apply(report.Helpf("this becomes a hard error in %s", syntax.Edition2023.Name())) } case m.IsExtension() && ctypeValue == 1: // google.protobuf.FieldOptions.CORD @@ -838,17 +841,18 @@ func validateCType(m Member, r *report.Report) { ) if !is2023 { - d.Apply(report.Helpf("this becomes a hard error in %s", prettyEdition(syntax.Edition2023))) + d.Apply(report.Helpf("this becomes a hard error in %s", syntax.Edition2023.Name())) } case is2023: - r.Warn(errEditionTooNew{ - file: f, - deprecated: syntax.Edition2023, - removed: syntax.Edition2024, - - what: ctype.Field().Name(), - where: ctype.KeyAST(), + r.Warn(erredition.TooNew{ + Current: m.Context().File().Syntax(), + Decl: m.Context().File().AST().Syntax(), + Deprecated: syntax.Edition2023, + Removed: syntax.Edition2024, + + What: ctype.Field().Name(), + Where: ctype.KeyAST(), }).Apply(ctype.suggestEdit( "features.(pb.cpp).string_type", want, "replace with `features.(pb.cpp).string_type`", diff --git a/experimental/ir/testdata/editions/inheritance.proto.stderr.txt b/experimental/ir/testdata/editions/inheritance.proto.stderr.txt index 245533acc..9fe464c14 100644 --- a/experimental/ir/testdata/editions/inheritance.proto.stderr.txt +++ b/experimental/ir/testdata/editions/inheritance.proto.stderr.txt @@ -8,9 +8,8 @@ warning: `legacy_closed_enum` is deprecated in Edition 2023 24 | option features.(pb.cpp).legacy_closed_enum = false; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = help: deprecated since Edition 2023 - = help: deprecated: The legacy closed enum behavior in C++ is deprecated and - is scheduled to be removed in edition 2025. See - http://protobuf.dev/programming-guides/enum/#cpp for more information + = help: the legacy closed enum behavior in C++ is deprecated and is scheduled + to be removed in Edition 2025 warning: required fields are deprecated --> testdata/editions/inheritance.proto:28:44 diff --git a/experimental/ir/testdata/editions/lifetime.proto.stderr.txt b/experimental/ir/testdata/editions/lifetime.proto.stderr.txt index 199d7bd78..ee9d6de2b 100644 --- a/experimental/ir/testdata/editions/lifetime.proto.stderr.txt +++ b/experimental/ir/testdata/editions/lifetime.proto.stderr.txt @@ -7,7 +7,7 @@ warning: `b` is deprecated in Edition 2023 23 | option features.(x) = {a: true, b: true, c: true, d: true, e: true, f: true}; | ^ = help: deprecated since Edition 2023 - = help: deprecated: This is a bad feature. Don't use it. + = help: this is a bad feature; don't use it error: `c` is not supported in Edition 2023 --> testdata/editions/lifetime.proto:23:42 diff --git a/experimental/ir/testdata/imports/ok.proto.yaml.fds.yaml b/experimental/ir/testdata/imports/ok.proto.yaml.fds.yaml index 48c9f97b9..3d2a1ddae 100644 --- a/experimental/ir/testdata/imports/ok.proto.yaml.fds.yaml +++ b/experimental/ir/testdata/imports/ok.proto.yaml.fds.yaml @@ -1,6 +1,6 @@ file: -- { name: "a.proto", package: "buf.test", syntax: "proto2" } - { name: "c.proto", package: "buf.test", syntax: "proto2" } +- { name: "a.proto", package: "buf.test", syntax: "proto2" } - { name: "b.proto", package: "buf.test", syntax: "proto2" } - name: "main.proto" package: "buf.test" diff --git a/experimental/ir/testdata/imports/ok.proto.yaml.stderr.txt b/experimental/ir/testdata/imports/ok.proto.yaml.stderr.txt index 4ce5128eb..b830395f4 100644 --- a/experimental/ir/testdata/imports/ok.proto.yaml.stderr.txt +++ b/experimental/ir/testdata/imports/ok.proto.yaml.stderr.txt @@ -1,8 +1,14 @@ -warning: `import weak` is deprecated - --> main.proto:6:1 +warning: `import weak` is deprecated in "proto2" + --> main.proto:6:8 | + 1 | syntax = "proto2"; + | -------- syntax specified here + 2 | package buf.test; +... + 5 | import public "b.proto"; 6 | import weak "c.proto"; - | ^^^^^^^^^^^ + | ^^^^ + = help: deprecated since "proto2", to be removed in Edition 2024 = help: `import weak` is not implemented correctly in most Protobuf implementations diff --git a/experimental/ir/testdata/imports/option.proto.yaml b/experimental/ir/testdata/imports/option.proto.yaml new file mode 100644 index 000000000..84f06bab4 --- /dev/null +++ b/experimental/ir/testdata/imports/option.proto.yaml @@ -0,0 +1,66 @@ +# Copyright 2020-2025 Buf Technologies, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +descriptor: true +symtab: true +files: +- path: "main.proto" + text: | + edition = "2024"; + package buf.test; + + import option "option.proto"; + + message Bar { + option (e).x = 1; + Foo y = 2; + } + +- path: "main2.proto" + text: | + edition = "2024"; + package buf.test; + + import "public.proto"; + import option "option.proto"; + + message Bar { + option (e).x = 1; + int32 y = 2; + } + +- path: "public.proto" + import: true + text: | + edition = "2024"; + package buf.test; + + import public "option.proto"; + +- path: "option.proto" + import: true + text: | + edition = "2024"; + package buf.test; + + import "google/protobuf/descriptor.proto"; + + message Foo { + int32 x = 1; + } + + extend google.protobuf.MessageOptions { + Foo e = 10001; + } + \ No newline at end of file diff --git a/experimental/ir/testdata/imports/option.proto.yaml.fds.yaml b/experimental/ir/testdata/imports/option.proto.yaml.fds.yaml new file mode 100644 index 000000000..171a7c284 --- /dev/null +++ b/experimental/ir/testdata/imports/option.proto.yaml.fds.yaml @@ -0,0 +1,58 @@ +file: +- name: "option.proto" + package: "buf.test" + dependency: ["google/protobuf/descriptor.proto"] + message_type: + - name: "Foo" + field: + - name: "x" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_INT32 + json_name: "x" + extension: + - name: "e" + number: 10001 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".buf.test.Foo" + extendee: ".google.protobuf.MessageOptions" + json_name: "e" + syntax: "editions" + edition: EDITION_2024 +- name: "main.proto" + package: "buf.test" + option_dependency: ["option.proto"] + message_type: + - name: "Bar" + field: + - name: "y" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".buf.test.Foo" + json_name: "y" + options.$unknown: "10001: {1: 1}" + syntax: "editions" + edition: EDITION_2024 +- name: "public.proto" + package: "buf.test" + dependency: ["option.proto"] + public_dependency: [0] + syntax: "editions" + edition: EDITION_2024 +- name: "main2.proto" + package: "buf.test" + dependency: ["public.proto"] + option_dependency: ["option.proto"] + message_type: + - name: "Bar" + field: + - name: "y" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_INT32 + json_name: "y" + options.$unknown: "10001: {1: 1}" + syntax: "editions" + edition: EDITION_2024 diff --git a/experimental/ir/testdata/imports/option.proto.yaml.stderr.txt b/experimental/ir/testdata/imports/option.proto.yaml.stderr.txt new file mode 100644 index 000000000..3ba686e0a --- /dev/null +++ b/experimental/ir/testdata/imports/option.proto.yaml.stderr.txt @@ -0,0 +1,55 @@ +error: sorry, Edition 2024 is not fully implemented + --> main.proto:1:11 + | + 1 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +error: `Foo` is only imported for use in options + --> main.proto:8:3 + | + 4 | import option "option.proto"; + | ----------------------------- imported as `option` here +... + 7 | option (e).x = 1; + 8 | Foo y = 2; + | ^^^ requires non-`option` import + | + help: delete `option` + | + 4 | - import option "option.proto"; + 4 | + import "option.proto"; + +error: sorry, Edition 2024 is not fully implemented + --> main2.proto:1:11 + | + 1 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +warning: unused import "main2.proto" + --> main2.proto:4:8 + | + 4 | import "public.proto"; + | ^^^^^^^^^^^^^^ + help: delete it + | + 4 | - import "public.proto"; + | + = help: no symbols from this file are referenced + +error: sorry, Edition 2024 is not fully implemented + --> option.proto:1:11 + | + 1 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +error: sorry, Edition 2024 is not fully implemented + --> public.proto:1:11 + | + 1 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +encountered 5 errors and 1 warning diff --git a/experimental/ir/testdata/imports/option.proto.yaml.symtab.yaml b/experimental/ir/testdata/imports/option.proto.yaml.symtab.yaml new file mode 100644 index 000000000..475c89cdb --- /dev/null +++ b/experimental/ir/testdata/imports/option.proto.yaml.symtab.yaml @@ -0,0 +1,62 @@ +tables: + "main.proto": + imports: + - { path: "google/protobuf/descriptor.proto", transitive: true } + - { path: "option.proto", visible: true } + symbols: + - { fqn: "buf.test", kind: KIND_PACKAGE, file: "main.proto" } + - fqn: "buf.test.Bar" + kind: KIND_MESSAGE + file: "main.proto" + index: 1 + visible: true + options.message.extns: + "buf.test.e": { message.fields: { "x": { i32: 1 } } } + - fqn: "buf.test.Bar.y" + kind: KIND_FIELD + file: "main.proto" + index: 1 + visible: true + - fqn: "buf.test.Foo" + kind: KIND_MESSAGE + file: "option.proto" + index: 1 + - fqn: "buf.test.Foo.x" + kind: KIND_FIELD + file: "option.proto" + index: 1 + - fqn: "buf.test.e" + kind: KIND_EXTENSION + file: "option.proto" + index: 2 + "main2.proto": + imports: + - { path: "google/protobuf/descriptor.proto", transitive: true } + - { path: "option.proto", visible: true } + - { path: "public.proto", visible: true } + symbols: + - { fqn: "buf.test", kind: KIND_PACKAGE, file: "main2.proto" } + - fqn: "buf.test.Bar" + kind: KIND_MESSAGE + file: "main2.proto" + index: 1 + visible: true + options.message.extns: + "buf.test.e": { message.fields: { "x": { i32: 1 } } } + - fqn: "buf.test.Bar.y" + kind: KIND_FIELD + file: "main2.proto" + index: 1 + visible: true + - fqn: "buf.test.Foo" + kind: KIND_MESSAGE + file: "option.proto" + index: 1 + - fqn: "buf.test.Foo.x" + kind: KIND_FIELD + file: "option.proto" + index: 1 + - fqn: "buf.test.e" + kind: KIND_EXTENSION + file: "option.proto" + index: 2 diff --git a/experimental/parser/legalize_file.go b/experimental/parser/legalize_file.go index 1ff34e512..eff275373 100644 --- a/experimental/parser/legalize_file.go +++ b/experimental/parser/legalize_file.go @@ -20,6 +20,7 @@ import ( "github.com/bufbuild/protocompile/experimental/ast" "github.com/bufbuild/protocompile/experimental/ast/syntax" + "github.com/bufbuild/protocompile/experimental/internal/erredition" "github.com/bufbuild/protocompile/experimental/internal/taxa" "github.com/bufbuild/protocompile/experimental/report" "github.com/bufbuild/protocompile/experimental/seq" @@ -358,6 +359,7 @@ func legalizeImport(p *parser, parent classified, decl ast.DeclImport) { return } + var isOption bool for i, mod := range seq.All(decl.ModifierTokens()) { if i > 0 { p.Errorf("unexpected `%s` modifier in %s", mod.Text(), in).Apply( @@ -374,19 +376,31 @@ func legalizeImport(p *parser, parent classified, decl ast.DeclImport) { case keyword.Public: case keyword.Weak: - p.Warnf("`import weak` is deprecated").Apply( - report.Snippet(report.Join(decl.KeywordToken(), mod)), - report.Helpf("`import weak` is not implemented correctly in most Protobuf implementations"), - ) + p.SoftError(p.syntax >= syntax.Edition2024, erredition.TooNew{ + Current: p.syntax, + Decl: p.syntaxNode, + Deprecated: syntax.Proto2, + DeprecatedReason: "`import weak` is not implemented correctly in most Protobuf implementations", + Removed: syntax.Edition2024, + RemovedReason: "`import weak` has been replaced with `import option`", + + What: "import weak", + Where: mod, + }) case keyword.Option: - p.Error(errRequiresEdition{ - edition: syntax.Edition2024, - node: report.Join(decl.KeywordToken(), mod), - what: "`import option`", - decl: p.syntaxNode, - unimplemented: p.syntax >= syntax.Edition2024, - }) + p.importOptionNode = decl + isOption = true + + if p.syntax < syntax.Edition2024 { + p.Error(erredition.TooOld{ + Current: p.syntax, + Decl: p.syntaxNode, + Intro: syntax.Edition2024, + What: "import option", + Where: mod, + }) + } default: d := p.Error(errUnexpectedMod{ @@ -410,4 +424,12 @@ func legalizeImport(p *parser, parent classified, decl ast.DeclImport) { } } } + + if !isOption && !p.importOptionNode.IsZero() { + p.Errorf("%s after `import option`", taxa.Import).Apply( + report.Snippet(decl), + report.Snippetf(p.importOptionNode, "previous `import option` here"), + report.Helpf("`import option`s must be the last imports in a file"), + ) + } } diff --git a/experimental/parser/parse_state.go b/experimental/parser/parse_state.go index c09f3153f..36bcaeeef 100644 --- a/experimental/parser/parse_state.go +++ b/experimental/parser/parse_state.go @@ -29,9 +29,10 @@ type parser struct { *ast.Nodes *report.Report - syntaxNode ast.DeclSyntax - syntax syntax.Syntax - parseComplete bool + syntaxNode ast.DeclSyntax + importOptionNode ast.DeclImport + syntax syntax.Syntax + parseComplete bool } // classified is a spanner that has been classified by taxa. diff --git a/experimental/parser/testdata/parser/import/modifiers.proto.stderr.txt b/experimental/parser/testdata/parser/import/modifiers.proto.stderr.txt index 3c78374fb..d6c13fa91 100644 --- a/experimental/parser/testdata/parser/import/modifiers.proto.stderr.txt +++ b/experimental/parser/testdata/parser/import/modifiers.proto.stderr.txt @@ -14,11 +14,16 @@ error: unexpected `option` modifier in import | | | already modified here -warning: `import weak` is deprecated - --> testdata/parser/import/modifiers.proto:21:1 +warning: `import weak` is deprecated in "proto2" + --> testdata/parser/import/modifiers.proto:21:8 | +15 | syntax = "proto2"; + | -------- syntax specified here +... +20 | import public option "foo.proto"; 21 | import weak option "foo.proto"; - | ^^^^^^^^^^^ + | ^^^^ + = help: deprecated since "proto2", to be removed in Edition 2024 = help: `import weak` is not implemented correctly in most Protobuf implementations @@ -30,11 +35,16 @@ error: unexpected `option` modifier in import | | | already modified here -error: `import option` requires Edition 2024 or later - --> testdata/parser/import/modifiers.proto:22:1 +error: `import option` is not supported in "proto2" + --> testdata/parser/import/modifiers.proto:22:8 | +15 | syntax = "proto2"; + | -------- syntax specified here +... +21 | import weak option "foo.proto"; 22 | import option weak public "foo.proto"; - | ^^^^^^^^^^^^^ + | ^^^^^^ + = help: `import option` requires at least Edition 2024 error: unexpected `weak` modifier in import --> testdata/parser/import/modifiers.proto:22:15 @@ -52,11 +62,25 @@ error: unexpected `public` modifier in import | | | already modified here -warning: `import weak` is deprecated +error: import after `import option` --> testdata/parser/import/modifiers.proto:23:1 | +22 | import option weak public "foo.proto"; + | -------------------------------------- previous `import option` here 23 | import weak weak "foo.proto"; - | ^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: `import option`s must be the last imports in a file + +warning: `import weak` is deprecated in "proto2" + --> testdata/parser/import/modifiers.proto:23:8 + | +15 | syntax = "proto2"; + | -------- syntax specified here +... +22 | import option weak public "foo.proto"; +23 | import weak weak "foo.proto"; + | ^^^^ + = help: deprecated since "proto2", to be removed in Edition 2024 = help: `import weak` is not implemented correctly in most Protobuf implementations @@ -68,6 +92,16 @@ error: unexpected `weak` modifier in import | | | already modified here +error: import after `import option` + --> testdata/parser/import/modifiers.proto:24:1 + | +22 | import option weak public "foo.proto"; + | -------------------------------------- previous `import option` here +23 | import weak weak "foo.proto"; +24 | import export "foo.proto"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: `import option`s must be the last imports in a file + error: unexpected `export` modifier in import --> testdata/parser/import/modifiers.proto:24:8 | @@ -80,4 +114,4 @@ error: unexpected `export` modifier in import | = help: `export` only applies to a type definition -encountered 8 errors and 2 warnings +encountered 10 errors and 2 warnings diff --git a/experimental/parser/testdata/parser/import/ok.proto.stderr.txt b/experimental/parser/testdata/parser/import/ok.proto.stderr.txt index 54847634a..1111d127a 100644 --- a/experimental/parser/testdata/parser/import/ok.proto.stderr.txt +++ b/experimental/parser/testdata/parser/import/ok.proto.stderr.txt @@ -1,15 +1,25 @@ -warning: `import weak` is deprecated - --> testdata/parser/import/ok.proto:20:1 +warning: `import weak` is deprecated in "proto2" + --> testdata/parser/import/ok.proto:20:8 | +15 | syntax = "proto2"; + | -------- syntax specified here +... +19 | import "foo.proto"; 20 | import weak "weak.proto"; - | ^^^^^^^^^^^ + | ^^^^ + = help: deprecated since "proto2", to be removed in Edition 2024 = help: `import weak` is not implemented correctly in most Protobuf implementations -error: `import option` requires Edition 2024 or later - --> testdata/parser/import/ok.proto:22:1 +error: `import option` is not supported in "proto2" + --> testdata/parser/import/ok.proto:22:8 | +15 | syntax = "proto2"; + | -------- syntax specified here +... +21 | import public "public.proto"; 22 | import option "option.proto"; - | ^^^^^^^^^^^^^ + | ^^^^^^ + = help: `import option` requires at least Edition 2024 encountered 1 error and 1 warning diff --git a/experimental/parser/testdata/parser/import/options.proto.stderr.txt b/experimental/parser/testdata/parser/import/options.proto.stderr.txt index 1ff67c1e2..dbd1a5154 100644 --- a/experimental/parser/testdata/parser/import/options.proto.stderr.txt +++ b/experimental/parser/testdata/parser/import/options.proto.stderr.txt @@ -4,11 +4,16 @@ error: import cannot specify compact options 19 | import "foo.proto" [(not.allowed) = "here"]; | ^^^^^^^^^^^^^^^^^^^^^^^^ help: remove this -warning: `import weak` is deprecated - --> testdata/parser/import/options.proto:20:1 +warning: `import weak` is deprecated in "proto2" + --> testdata/parser/import/options.proto:20:8 | +15 | syntax = "proto2"; + | -------- syntax specified here +... +19 | import "foo.proto" [(not.allowed) = "here"]; 20 | import weak "weak.proto" [(not.allowed) = "here"]; - | ^^^^^^^^^^^ + | ^^^^ + = help: deprecated since "proto2", to be removed in Edition 2024 = help: `import weak` is not implemented correctly in most Protobuf implementations diff --git a/experimental/parser/testdata/parser/import/order.proto b/experimental/parser/testdata/parser/import/order.proto new file mode 100644 index 000000000..76c39f8d5 --- /dev/null +++ b/experimental/parser/testdata/parser/import/order.proto @@ -0,0 +1,24 @@ +// Copyright 2020-2024 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +edition = "2024"; + +package test; + +import "foo.proto"; +import "bar.proto"; +import weak "weak.proto"; +import option "baz.proto"; +import option "bang.proto"; +import "wrong.proto"; \ No newline at end of file diff --git a/experimental/parser/testdata/parser/import/order.proto.stderr.txt b/experimental/parser/testdata/parser/import/order.proto.stderr.txt new file mode 100644 index 000000000..801430ce2 --- /dev/null +++ b/experimental/parser/testdata/parser/import/order.proto.stderr.txt @@ -0,0 +1,29 @@ +error: sorry, Edition 2024 is not fully implemented + --> testdata/parser/import/order.proto:15:11 + | +15 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +error: `import weak` is not supported in Edition 2024 + --> testdata/parser/import/order.proto:21:8 + | +15 | edition = "2024"; + | ------ edition specified here +... +20 | import "bar.proto"; +21 | import weak "weak.proto"; + | ^^^^ + = help: deprecated since "proto2", removed in Edition 2024 + = help: `import weak` has been replaced with `import option` + +error: import after `import option` + --> testdata/parser/import/order.proto:24:1 + | +23 | import option "bang.proto"; + | --------------------------- previous `import option` here +24 | import "wrong.proto"; + | ^^^^^^^^^^^^^^^^^^^^^ + = help: `import option`s must be the last imports in a file + +encountered 3 errors diff --git a/experimental/parser/testdata/parser/import/order.proto.yaml b/experimental/parser/testdata/parser/import/order.proto.yaml new file mode 100644 index 000000000..ddb9313bd --- /dev/null +++ b/experimental/parser/testdata/parser/import/order.proto.yaml @@ -0,0 +1,18 @@ +decls: +- syntax: { kind: KIND_EDITION, value.literal.string_value: "2024" } +- package.path.components: [{ ident: "test" }] +- import.import_path.literal.string_value: "foo.proto" +- import.import_path.literal.string_value: "bar.proto" +- import: + modifier: [MODIFIER_WEAK] + import_path.literal.string_value: "weak.proto" + modifier_span: [{}] +- import: + modifier: [MODIFIER_UNSPECIFIED] + import_path.literal.string_value: "baz.proto" + modifier_span: [{}] +- import: + modifier: [MODIFIER_UNSPECIFIED] + import_path.literal.string_value: "bang.proto" + modifier_span: [{}] +- import.import_path.literal.string_value: "wrong.proto" diff --git a/internal/gen/buf/compiler/v1alpha1/symtab.pb.go b/internal/gen/buf/compiler/v1alpha1/symtab.pb.go index 293f75550..794da13ec 100644 --- a/internal/gen/buf/compiler/v1alpha1/symtab.pb.go +++ b/internal/gen/buf/compiler/v1alpha1/symtab.pb.go @@ -384,6 +384,7 @@ type Symbol struct { Index uint32 `protobuf:"varint,4,opt,name=index,proto3" json:"index,omitempty"` // Whether this symbol can be validly referenced in the current file. Visible bool `protobuf:"varint,5,opt,name=visible,proto3" json:"visible,omitempty"` + OptionOnly bool `protobuf:"varint,8,opt,name=option_only,json=optionOnly,proto3" json:"option_only,omitempty"` Options *Value `protobuf:"bytes,6,opt,name=options,proto3" json:"options,omitempty"` Features []*Feature `protobuf:"bytes,7,rep,name=features,proto3" json:"features,omitempty"` unknownFields protoimpl.UnknownFields @@ -455,6 +456,13 @@ func (x *Symbol) GetVisible() bool { return false } +func (x *Symbol) GetOptionOnly() bool { + if x != nil { + return x.OptionOnly + } + return false +} + func (x *Symbol) GetOptions() *Value { if x != nil { return x.Options @@ -889,13 +897,15 @@ const file_buf_compiler_v1alpha1_symtab_proto_rawDesc = "" + "\x04extn\x18\x01 \x01(\tR\x04extn\x12\x12\n" + "\x04name\x18\x02 \x01(\tR\x04name\x12\x14\n" + "\x05value\x18\x03 \x01(\tR\x05value\x12\x1a\n" + - "\bexplicit\x18\x04 \x01(\bR\bexplicit\"\xb6\x03\n" + + "\bexplicit\x18\x04 \x01(\bR\bexplicit\"\xd7\x03\n" + "\x06Symbol\x12\x10\n" + "\x03fqn\x18\x01 \x01(\tR\x03fqn\x126\n" + "\x04kind\x18\x02 \x01(\x0e2\".buf.compiler.v1alpha1.Symbol.KindR\x04kind\x12\x12\n" + "\x04file\x18\x03 \x01(\tR\x04file\x12\x14\n" + "\x05index\x18\x04 \x01(\rR\x05index\x12\x18\n" + - "\avisible\x18\x05 \x01(\bR\avisible\x126\n" + + "\avisible\x18\x05 \x01(\bR\avisible\x12\x1f\n" + + "\voption_only\x18\b \x01(\bR\n" + + "optionOnly\x126\n" + "\aoptions\x18\x06 \x01(\v2\x1c.buf.compiler.v1alpha1.ValueR\aoptions\x12:\n" + "\bfeatures\x18\a \x03(\v2\x1e.buf.compiler.v1alpha1.FeatureR\bfeatures\"\xa9\x01\n" + "\x04Kind\x12\x14\n" + diff --git a/internal/proto/buf/compiler/v1alpha1/symtab.proto b/internal/proto/buf/compiler/v1alpha1/symtab.proto index 67eed76d5..ae816eb84 100644 --- a/internal/proto/buf/compiler/v1alpha1/symtab.proto +++ b/internal/proto/buf/compiler/v1alpha1/symtab.proto @@ -77,6 +77,7 @@ message Symbol { // Whether this symbol can be validly referenced in the current file. bool visible = 5; + bool option_only = 8; Value options = 6; From 534b4d2cb4af448ef09ba218a22b81232fb4b75e Mon Sep 17 00:00:00 2001 From: Miguel Young de la Sota Date: Fri, 17 Oct 2025 18:57:22 -0700 Subject: [PATCH 3/4] implement export/local --- experimental/ir/builtins.go | 49 ++- experimental/ir/fdp.go | 18 + experimental/ir/ir_symbol.go | 20 +- experimental/ir/ir_type.go | 70 ++++ experimental/ir/lower_resolve.go | 42 +- experimental/ir/lower_validate.go | 142 +++++++ experimental/ir/lower_walk.go | 14 +- .../ir/testdata/imports/option.proto.yaml | 1 + .../imports/option.proto.yaml.stderr.txt | 1 + .../ir/testdata/strict_visibility.proto | 54 +++ .../strict_visibility.proto.stderr.txt | 222 +++++++++++ .../ir/testdata/visibility.proto.yaml | 167 ++++++++ .../testdata/visibility.proto.yaml.fds.yaml | 372 ++++++++++++++++++ .../testdata/visibility.proto.yaml.stderr.txt | 289 ++++++++++++++ experimental/parser/diagnostics_internal.go | 38 -- experimental/parser/legalize_def.go | 17 +- experimental/parser/parse_decl.go | 2 +- .../testdata/parser/def/2024.proto.stderr.txt | 34 +- internal/interval/intersect.go | 40 ++ internal/interval/intersect_test.go | 24 ++ 20 files changed, 1510 insertions(+), 106 deletions(-) create mode 100644 experimental/ir/testdata/strict_visibility.proto create mode 100644 experimental/ir/testdata/strict_visibility.proto.stderr.txt create mode 100644 experimental/ir/testdata/visibility.proto.yaml create mode 100644 experimental/ir/testdata/visibility.proto.yaml.fds.yaml create mode 100644 experimental/ir/testdata/visibility.proto.yaml.stderr.txt diff --git a/experimental/ir/builtins.go b/experimental/ir/builtins.go index 968f15781..47eb53d76 100644 --- a/experimental/ir/builtins.go +++ b/experimental/ir/builtins.go @@ -17,6 +17,7 @@ package ir import ( "fmt" "reflect" + "strings" "github.com/bufbuild/protocompile/internal/arena" "github.com/bufbuild/protocompile/internal/intern" @@ -76,14 +77,15 @@ type builtins struct { EditionSupportWarning Member EditionSupportRemoved Member - FeatureSet Type - FeaturePresence Member - FeatureEnumType Member - FeaturePacked Member - FeatureUTF8 Member - FeatureGroup Member - FeatureEnum Member - FeatureJSON Member + FeatureSet Type + FeaturePresence Member + FeatureEnumType Member + FeaturePacked Member + FeatureUTF8 Member + FeatureGroup Member + FeatureEnum Member + FeatureJSON Member + FeatureVisibility Member `builtin:"optional"` FileFeatures Member MessageFeatures Member @@ -161,14 +163,15 @@ type builtinIDs struct { EditionSupportWarning intern.ID `intern:"google.protobuf.FieldOptions.FeatureSupport.deprecation_warning"` EditionSupportRemoved intern.ID `intern:"google.protobuf.FieldOptions.FeatureSupport.edition_removed"` - FeatureSet intern.ID `intern:"google.protobuf.FeatureSet"` - FeaturePresence intern.ID `intern:"google.protobuf.FeatureSet.field_presence"` - FeatureEnumType intern.ID `intern:"google.protobuf.FeatureSet.enum_type"` - FeaturePacked intern.ID `intern:"google.protobuf.FeatureSet.repeated_field_encoding"` - FeatureUTF8 intern.ID `intern:"google.protobuf.FeatureSet.utf8_validation"` - FeatureGroup intern.ID `intern:"google.protobuf.FeatureSet.message_encoding"` - FeatureEnum intern.ID `intern:"google.protobuf.FeatureSet.enum_type"` - FeatureJSON intern.ID `intern:"google.protobuf.FeatureSet.json_format"` + FeatureSet intern.ID `intern:"google.protobuf.FeatureSet"` + FeaturePresence intern.ID `intern:"google.protobuf.FeatureSet.field_presence"` + FeatureEnumType intern.ID `intern:"google.protobuf.FeatureSet.enum_type"` + FeaturePacked intern.ID `intern:"google.protobuf.FeatureSet.repeated_field_encoding"` + FeatureUTF8 intern.ID `intern:"google.protobuf.FeatureSet.utf8_validation"` + FeatureGroup intern.ID `intern:"google.protobuf.FeatureSet.message_encoding"` + FeatureEnum intern.ID `intern:"google.protobuf.FeatureSet.enum_type"` + FeatureJSON intern.ID `intern:"google.protobuf.FeatureSet.json_format"` + FeatureVisibility intern.ID `intern:"google.protobuf.FeatureSet.default_symbol_visibility"` FileFeatures intern.ID `intern:"google.protobuf.FileOptions.features"` MessageFeatures intern.ID `intern:"google.protobuf.MessageOptions.features"` @@ -206,11 +209,23 @@ func resolveBuiltins(c *Context) { ids := reflect.ValueOf(c.session.builtins) for i := range v.NumField() { field := v.Field(i) - id := ids.FieldByName(v.Type().Field(i).Name).Interface().(intern.ID) //nolint:errcheck + tyField := v.Type().Field(i) + id := ids.FieldByName(tyField.Name).Interface().(intern.ID) //nolint:errcheck kind := kinds[field.Type()] + var optional bool + for option := range strings.SplitSeq(tyField.Tag.Get("builtin"), ",") { + if option == "optional" { + optional = true + } + } + ref := c.exported.lookup(c, id) sym := wrapSymbol(c, ref) + if sym.IsZero() && optional { + continue + } + if sym.Kind() != kind.kind { panic(fmt.Errorf( "missing descriptor.proto symbol: %s `%s`; got kind %s", diff --git a/experimental/ir/fdp.go b/experimental/ir/fdp.go index b0f8387fd..3c16838df 100644 --- a/experimental/ir/fdp.go +++ b/experimental/ir/fdp.go @@ -278,6 +278,15 @@ func (dg *descGenerator) message(ty Type, mdp *descriptorpb.DescriptorProto) { mdp.Options = new(descriptorpb.MessageOptions) dg.options(options, mdp.Options) } + + switch exported, explicit := ty.IsExported(); { + case !explicit: + break + case exported: + mdp.Visibility = descriptorpb.SymbolVisibility_VISIBILITY_EXPORT.Enum() + case !exported: + mdp.Visibility = descriptorpb.SymbolVisibility_VISIBILITY_LOCAL.Enum() + } } var predeclaredToFDPType = []descriptorpb.FieldDescriptorProto_Type{ @@ -406,6 +415,15 @@ func (dg *descGenerator) enum(ty Type, edp *descriptorpb.EnumDescriptorProto) { edp.Options = new(descriptorpb.EnumOptions) dg.options(options, edp.Options) } + + switch exported, explicit := ty.IsExported(); { + case !explicit: + break + case exported: + edp.Visibility = descriptorpb.SymbolVisibility_VISIBILITY_EXPORT.Enum() + case !exported: + edp.Visibility = descriptorpb.SymbolVisibility_VISIBILITY_LOCAL.Enum() + } } func (dg *descGenerator) enumValue(f Member, evdp *descriptorpb.EnumValueDescriptorProto) { diff --git a/experimental/ir/ir_symbol.go b/experimental/ir/ir_symbol.go index 153ae10a5..27ad56ac4 100644 --- a/experimental/ir/ir_symbol.go +++ b/experimental/ir/ir_symbol.go @@ -20,7 +20,6 @@ import ( "slices" "sync" - "github.com/bufbuild/protocompile/experimental/ast" "github.com/bufbuild/protocompile/experimental/internal" "github.com/bufbuild/protocompile/experimental/internal/taxa" "github.com/bufbuild/protocompile/experimental/report" @@ -188,8 +187,18 @@ func (s Symbol) Visible(allowOptions bool) bool { if s.ref.file <= 0 || s.Kind() == SymbolKindPackage { return true } + file := s.Context().imports.files[uint(s.ref.file)-1] - return file.visible && (allowOptions || !file.option) + if !file.visible || !(allowOptions || !file.option) { + return false + } + + if ty := s.AsType(); !ty.IsZero() { + exported, _ := ty.IsExported() + return exported + } + + return true } // Definition returns a span for the definition site of this symbol; @@ -221,13 +230,12 @@ func (s Symbol) Definition() report.Span { // Import returns the import declaration that brought this symbol into scope. // // Returns zero of s is defined in the current file. -func (s Symbol) Import() ast.DeclImport { +func (s Symbol) Import() Import { if s.ref.file <= 0 { - return ast.DeclImport{} + return Import{} } - file := s.Context().imports.files[uint(s.ref.file)-1] - return file.decl + return s.Context().imports.Transitive().At(int(s.ref.file) - 1) } // noun returns a [taxa.Noun] for diagnostic use. diff --git a/experimental/ir/ir_type.go b/experimental/ir/ir_type.go index 3ca3a04af..6e3a91585 100644 --- a/experimental/ir/ir_type.go +++ b/experimental/ir/ir_type.go @@ -17,12 +17,15 @@ package ir import ( "fmt" "iter" + "math" "github.com/bufbuild/protocompile/experimental/ast" "github.com/bufbuild/protocompile/experimental/ast/predeclared" "github.com/bufbuild/protocompile/experimental/internal" "github.com/bufbuild/protocompile/experimental/internal/taxa" "github.com/bufbuild/protocompile/experimental/seq" + "github.com/bufbuild/protocompile/experimental/token" + "github.com/bufbuild/protocompile/experimental/token/keyword" "github.com/bufbuild/protocompile/internal/arena" "github.com/bufbuild/protocompile/internal/ext/iterx" "github.com/bufbuild/protocompile/internal/intern" @@ -88,6 +91,7 @@ type rawType struct { isEnum, isMessageSet bool allowsAlias bool missingRanges bool // See lower_numbers.go. + visibility token.ID } type rawTagRange struct { @@ -212,6 +216,38 @@ func (t Type) IsAny() bool { return t.InternedFullName() == t.Context().session.builtins.AnyPath } +// IsExported returns whether this type is exported for the purposes of +// visibility in other files. +// +// Returns whether this was set explicitly via the export or local keywords. +func (t Type) IsExported() (exported, explicit bool) { + if t.IsZero() { + return false, false + } + + // This is explicitly set via keyword. + if !t.raw.visibility.IsZero() { + return t.raw.visibility.In(t.AST().Context()).Keyword() == keyword.Export, true + } + + // Look up the feature. + if key := t.Context().builtins().FeatureVisibility; !key.IsZero() { + feature := t.FeatureSet().Lookup(key) + switch v, _ := feature.Value().AsInt(); v { + case 0, 1: // DEFAULT_SYMBOL_VISIBILITY_UNKNOWN, EXPORT_ALL + return true, false + case 2: // EXPORT_TOP_LEVEL + return t.Parent().IsZero(), false + case 3, 4: // LOCAL_ALL, STRICT + return false, false + } + } + + // If descriptor.proto is too old to have this feature, assume this + // type is exported. + return true, false +} + // Predeclared returns the predeclared type that this Type corresponds to, if any. // // Returns either [predeclared.Unknown] or a value such that [predeclared.Name.IsScalar] @@ -456,6 +492,40 @@ func (t Type) ReservedNames() seq.Indexer[ReservedName] { ) } +// AbsoluteRange returns the smallest and largest number a member of this type +// can have. +// +// This range is inclusive. +func (t Type) AbsoluteRange() (start, end int32) { + switch { + case t.IsEnum(): + return math.MinInt32, math.MaxInt32 + case t.IsMessageSet(): + return 1, messageSetNumberMax + default: + return 1, fieldNumberMax + } +} + +// OccupiedRanges returns ranges of member numbers currently in use in this +// type. The pairs of numbers are inclusive ranges. +func (t Type) OccupiedRanges() iter.Seq2[[2]int32, seq.Indexer[TagRange]] { + return func(yield func([2]int32, seq.Indexer[TagRange]) bool) { + if t.IsZero() { + return + } + + for e := range t.raw.rangesByNumber.Contiguous(true) { + ranges := seq.NewFixedSlice(e.Value, func(_ int, v rawTagRange) TagRange { + return TagRange{t.withContext, v} + }) + if !yield([2]int32{e.Start, e.End}, ranges) { + return + } + } + } +} + // Options returns the options applied to this type. func (t Type) Oneofs() seq.Indexer[Oneof] { return seq.NewFixedSlice( diff --git a/experimental/ir/lower_resolve.go b/experimental/ir/lower_resolve.go index 866f07f5d..3afbd684f 100644 --- a/experimental/ir/lower_resolve.go +++ b/experimental/ir/lower_resolve.go @@ -334,7 +334,7 @@ func (r symbolRef) diagnoseLookup(sym Symbol, expectedName FullName) *report.Dia ) case !sym.Visible(r.allowOption): if !r.allowOption && sym.Visible(true) { - decl := sym.Import() + decl := sym.Import().Decl var option token.Token for m := range seq.Values(decl.ModifierTokens()) { if m.Keyword() == keyword.Option { @@ -344,7 +344,7 @@ func (r symbolRef) diagnoseLookup(sym Symbol, expectedName FullName) *report.Dia span := report.Join(decl.KeywordToken(), option) // This symbol is only visible in option position. - r.Errorf("`%s` is only imported for use in options", r.name).Apply( + return r.Errorf("`%s` is only imported for use in options", r.name).Apply( report.Snippetf(r.span, "requires non-`option` import"), report.Snippetf(decl, "imported as `option` here"), report.SuggestEdits(span, "delete `option`", report.Edit{ @@ -352,7 +352,43 @@ func (r symbolRef) diagnoseLookup(sym Symbol, expectedName FullName) *report.Dia Replace: "import", }), ) - break + } + + // Check to see if the corresponding import is visible. If it is, that + // means that this is an unexported type. + if imp := sym.Import(); imp.Visible { + if ty := sym.AsType(); !ty.IsZero() { + d := r.Errorf("found unexported %s `%s`", ty.noun(), ty.FullName()).Apply( + report.Snippetf(r.span, "unexported type"), + ) + + // First, see if local was set explicitly. + var local token.Token + for prefix := range ty.AST().Type().Prefixes() { + if prefix.Prefix() == keyword.Local { + local = prefix.PrefixToken() + break + } + } + + if !local.IsZero() { + d.Apply(report.Snippetf(local, "marked as local here")) + } else { + var span report.Span + // Otherwise, see if this was set due to a feature. + if key := ty.Context().builtins().FeatureVisibility; !key.IsZero() { + feature := ty.FeatureSet().Lookup(key) + if !feature.IsDefault() { + span = feature.Value().ValueAST().Span() + } else { + span = ty.Context().File().AST().Syntax().Value().Span() + } + } + + d.Apply(report.Snippetf(span, "this implies `local`")) + } + return d + } } // Complain that we need to import a symbol. diff --git a/experimental/ir/lower_validate.go b/experimental/ir/lower_validate.go index 414d91871..7703cbf1e 100644 --- a/experimental/ir/lower_validate.go +++ b/experimental/ir/lower_validate.go @@ -34,6 +34,7 @@ import ( "github.com/bufbuild/protocompile/experimental/seq" "github.com/bufbuild/protocompile/experimental/token" "github.com/bufbuild/protocompile/experimental/token/keyword" + "github.com/bufbuild/protocompile/internal/ext/cmpx" "github.com/bufbuild/protocompile/internal/ext/iterx" "github.com/bufbuild/protocompile/internal/ext/mapsx" ) @@ -64,6 +65,7 @@ func validateConstraints(f File, r *report.Report) { for ty := range seq.Values(f.AllTypes()) { validateReservedNames(ty, r) + validateVisibility(ty, r) switch { case ty.IsEnum(): validateEnum(ty, r) @@ -980,6 +982,146 @@ func validateUTF8Values(v Value, r *report.Report) { } } +func validateVisibility(ty Type, r *report.Report) { + key := ty.Context().builtins().FeatureVisibility + if key.IsZero() { + return + } + feature := ty.FeatureSet().Lookup(key) + value, _ := feature.Value().AsInt() + strict := value == 4 // STRICT + var impliedExport bool + switch value { + case 0, 1: // DEFAULT_SYMBOL_VISIBILITY_UNKNOWN, EXPORT_ALL + impliedExport = true + case 2: // EXPORT_TOP_LEVEL + impliedExport = ty.Parent().IsZero() + case 3, 4: // LOCAL_ALL, STRICT + impliedExport = false + } + + var why report.Span + if feature.IsDefault() { + why = ty.Context().File().AST().Syntax().Value().Span() + } else { + why = feature.Value().ValueAST().Span() + } + + vis := ty.raw.visibility.In(ty.AST().Context()) + export := vis.Keyword() == keyword.Export + if !ty.raw.visibility.IsZero() && export == impliedExport { + r.Warnf("redundant visibility modifier").Apply( + report.Snippetf(vis, "specified here"), + report.PageBreak, + report.Snippetf(why, "this implies it"), + ) + } + + if !strict || !export { // STRICT + return + } + + // STRICT requires that we check two things: + // + // 1. Nested types are not explicitly exported. + // 2. Unless they are nested within a message that reserves all of its + // field numbers. + parent := ty.Parent() + if ty.Parent().IsZero() { + return + } + + start, end := parent.AbsoluteRange() + + // Find any gaps in the reserved ranges. + gap := int32(start) + ranges := slices.Collect(seq.Values(parent.ReservedRanges())) + if len(ranges) > 0 { + slices.SortFunc(ranges, cmpx.Join( + cmpx.Key(func(r ReservedRange) int32 { + start, _ := r.Range() + return start + }), + cmpx.Key(func(r ReservedRange) int32 { + start, end := r.Range() + return end - start + }), + )) + + // Skip all ranges whose end is less than start. + for len(ranges) > 0 { + _, end := ranges[0].Range() + if end >= start { + break + } + ranges = ranges[1:] + } + + for _, rr := range ranges { + a, b := rr.Range() + if gap < a { + // We're done, gap is not reserved. + break + } + gap = b + 1 + } + } + + if end <= gap { + // If there are multiple reserved ranges, protoc rejects this, because it + // doesn't do the same sophisticated interval sorting we do. + switch { + case parent.ReservedRanges().Len() != 1: + d := r.Errorf("expected exactly one reserved range").Apply( + report.Snippetf(vis, "nested type exported here"), + report.Snippetf(parent.AST(), "... within this type"), + ) + ranges := parent.ReservedRanges() + if ranges.Len() > 0 { + d.Apply( + report.Snippetf(ranges.At(0).AST(), "one here"), + report.Snippetf(ranges.At(1).AST(), "another here"), + ) + } + d.Apply( + report.PageBreak, + report.Snippetf(why, "`STRICT` specified here"), + report.Helpf("in strict mode, nesting an exported type within another type "+ + "requires that that type declare `reserved 1 to max;`, even if all of its field "+ + "numbers are `reserved`"), + report.Helpf("protoc erroneously rejects this, despite being equivalent"), + ) + case ty.IsMessage(): + r.Errorf("nested message type marked as exported").Apply( + report.Snippetf(vis, "nested type exported here"), + report.Snippetf(parent.AST(), "... within this type"), + report.PageBreak, + report.Snippetf(why, "`STRICT` specified here"), + report.Helpf("in strict mode, nested message types cannot be marked as "+ + "exported, even if all the field numbers of its parent are reserved"), + ) + } + + return + } + + // If this is true, the protoc check is bugged and we emit a warning... + bugged := parent.ReservedRanges().Len() == 1 + d := r.SoftErrorf(!bugged, "%s `%s` does not reserve all field numbers", parent.noun(), parent.FullName()).Apply( + report.Snippetf(vis, "nested type exported here"), + report.Snippetf(parent.AST(), "... within this type"), + report.PageBreak, + report.Snippetf(why, "`STRICT` specified here"), + report.Helpf("in strict mode, nesting an exported type within another type "+ + `requires that that type reserve every field number (the "C++ namespace exception"), `+ + "but this type does not reserve the field number %d", gap), + ) + if bugged { + d.Apply(report.Helpf("protoc erroneously accepts this code due to a bug: it only " + + "checks that there is exactly one reserved range")) + } +} + // errNotUTF8 diagnoses a non-UTF8 value. type errNotUTF8 struct { value Element diff --git a/experimental/ir/lower_walk.go b/experimental/ir/lower_walk.go index fd30b6b81..afe531a1e 100644 --- a/experimental/ir/lower_walk.go +++ b/experimental/ir/lower_walk.go @@ -168,6 +168,17 @@ func (w *walker) newType(def ast.DeclDef, parent any) Type { name := def.Name().AsIdent().Name() fqn := w.fullname(parentTy, name) + var visibility token.ID + for prefix := range def.Type().Prefixes() { + switch prefix.Prefix() { + case keyword.Local, keyword.Export: + visibility = prefix.PrefixToken().ID() + default: + continue + } + break + } + isEnum := def.Keyword() == keyword.Enum raw := c.arenas.types.NewCompressed(rawType{ def: def, @@ -175,7 +186,8 @@ func (w *walker) newType(def ast.DeclDef, parent any) Type { fqn: c.session.intern.Intern(fqn), parent: c.arenas.types.Compress(parentTy.raw), - isEnum: isEnum, + isEnum: isEnum, + visibility: visibility, }) ty := Type{internal.NewWith(w.Context()), c.arenas.types.Deref(raw)} diff --git a/experimental/ir/testdata/imports/option.proto.yaml b/experimental/ir/testdata/imports/option.proto.yaml index 84f06bab4..83696ef51 100644 --- a/experimental/ir/testdata/imports/option.proto.yaml +++ b/experimental/ir/testdata/imports/option.proto.yaml @@ -1,3 +1,4 @@ + # Copyright 2020-2025 Buf Technologies, Inc. # # Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/experimental/ir/testdata/imports/option.proto.yaml.stderr.txt b/experimental/ir/testdata/imports/option.proto.yaml.stderr.txt index 3ba686e0a..31803fb52 100644 --- a/experimental/ir/testdata/imports/option.proto.yaml.stderr.txt +++ b/experimental/ir/testdata/imports/option.proto.yaml.stderr.txt @@ -19,6 +19,7 @@ error: `Foo` is only imported for use in options | 4 | - import option "option.proto"; 4 | + import "option.proto"; + | error: sorry, Edition 2024 is not fully implemented --> main2.proto:1:11 diff --git a/experimental/ir/testdata/strict_visibility.proto b/experimental/ir/testdata/strict_visibility.proto new file mode 100644 index 000000000..7de14f56d --- /dev/null +++ b/experimental/ir/testdata/strict_visibility.proto @@ -0,0 +1,54 @@ +// Copyright 2020-2025 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +edition = "2024"; +package buf.test; + +option features.default_symbol_visibility = STRICT; + +export message Complete { + export message M {} + export enum E { Z = 0; } + reserved 1 to max; +} + +export message Complete2 { + export message M {} + export enum E { Z = 0; } + reserved 1 to 536870911; +} + +export message Split { + export message M {} + export enum E { Z = 0; } + reserved 1 to 5, 6 to max; +} + +export message Incomplete1 { + export message M {} + export enum E { Z = 0; } + reserved 1 to 5; +} + +export message Incomplete2 { + export message M {} + export enum E { Z = 0; } + reserved 6 to max; +} + +export message Incomplete3 { + export message M {} + export enum E { Z = 0; } + reserved 1 to 5, 6 to 10; +} \ No newline at end of file diff --git a/experimental/ir/testdata/strict_visibility.proto.stderr.txt b/experimental/ir/testdata/strict_visibility.proto.stderr.txt new file mode 100644 index 000000000..e425f00d1 --- /dev/null +++ b/experimental/ir/testdata/strict_visibility.proto.stderr.txt @@ -0,0 +1,222 @@ +error: sorry, Edition 2024 is not fully implemented + --> testdata/strict_visibility.proto:15:11 + | +15 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +error: nested message type marked as exported + --> testdata/strict_visibility.proto:21:5 + | +20 | / export message Complete { +21 | | export message M {} + | | ^^^^^^ nested type exported here +22 | | export enum E { Z = 0; } +23 | | reserved 1 to max; +24 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nested message types cannot be marked as exported, + even if all the field numbers of its parent are reserved + +error: nested message type marked as exported + --> testdata/strict_visibility.proto:27:5 + | +26 | / export message Complete2 { +27 | | export message M {} + | | ^^^^^^ nested type exported here +28 | | export enum E { Z = 0; } +29 | | reserved 1 to 536870911; +30 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nested message types cannot be marked as exported, + even if all the field numbers of its parent are reserved + +error: expected exactly one reserved range + --> testdata/strict_visibility.proto:33:5 + | +32 | / export message Split { +33 | | export message M {} + | | ^^^^^^ nested type exported here +34 | | export enum E { Z = 0; } +35 | | reserved 1 to 5, 6 to max; + | | ------ -------- another here + | | | + | | one here +36 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nesting an exported type within another type requires + that that type declare `reserved 1 to max;`, even if all of its field + numbers are `reserved` + = help: protoc erroneously rejects this, despite being equivalent + +error: expected exactly one reserved range + --> testdata/strict_visibility.proto:34:5 + | +32 | / export message Split { +33 | | export message M {} +34 | | export enum E { Z = 0; } + | | ^^^^^^ nested type exported here +35 | | reserved 1 to 5, 6 to max; + | | ------ -------- another here + | | | + | | one here +36 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nesting an exported type within another type requires + that that type declare `reserved 1 to max;`, even if all of its field + numbers are `reserved` + = help: protoc erroneously rejects this, despite being equivalent + +warning: message type `buf.test.Incomplete1` does not reserve all field numbers + --> testdata/strict_visibility.proto:39:5 + | +38 | / export message Incomplete1 { +39 | | export message M {} + | | ^^^^^^ nested type exported here +40 | | export enum E { Z = 0; } +41 | | reserved 1 to 5; +42 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nesting an exported type within another type requires + that that type reserve every field number (the "C++ namespace + exception"), but this type does not reserve the field number 6 + = help: protoc erroneously accepts this code due to a bug: it only checks + that there is exactly one reserved range + +warning: message type `buf.test.Incomplete1` does not reserve all field numbers + --> testdata/strict_visibility.proto:40:5 + | +38 | / export message Incomplete1 { +39 | | export message M {} +40 | | export enum E { Z = 0; } + | | ^^^^^^ nested type exported here +41 | | reserved 1 to 5; +42 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nesting an exported type within another type requires + that that type reserve every field number (the "C++ namespace + exception"), but this type does not reserve the field number 6 + = help: protoc erroneously accepts this code due to a bug: it only checks + that there is exactly one reserved range + +warning: message type `buf.test.Incomplete2` does not reserve all field numbers + --> testdata/strict_visibility.proto:45:5 + | +44 | / export message Incomplete2 { +45 | | export message M {} + | | ^^^^^^ nested type exported here +46 | | export enum E { Z = 0; } +47 | | reserved 6 to max; +48 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nesting an exported type within another type requires + that that type reserve every field number (the "C++ namespace + exception"), but this type does not reserve the field number 1 + = help: protoc erroneously accepts this code due to a bug: it only checks + that there is exactly one reserved range + +warning: message type `buf.test.Incomplete2` does not reserve all field numbers + --> testdata/strict_visibility.proto:46:5 + | +44 | / export message Incomplete2 { +45 | | export message M {} +46 | | export enum E { Z = 0; } + | | ^^^^^^ nested type exported here +47 | | reserved 6 to max; +48 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nesting an exported type within another type requires + that that type reserve every field number (the "C++ namespace + exception"), but this type does not reserve the field number 1 + = help: protoc erroneously accepts this code due to a bug: it only checks + that there is exactly one reserved range + +error: message type `buf.test.Incomplete3` does not reserve all field numbers + --> testdata/strict_visibility.proto:51:5 + | +50 | / export message Incomplete3 { +51 | | export message M {} + | | ^^^^^^ nested type exported here +52 | | export enum E { Z = 0; } +53 | | reserved 1 to 5, 6 to 10; +54 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nesting an exported type within another type requires + that that type reserve every field number (the "C++ namespace + exception"), but this type does not reserve the field number 11 + +error: message type `buf.test.Incomplete3` does not reserve all field numbers + --> testdata/strict_visibility.proto:52:5 + | +50 | / export message Incomplete3 { +51 | | export message M {} +52 | | export enum E { Z = 0; } + | | ^^^^^^ nested type exported here +53 | | reserved 1 to 5, 6 to 10; +54 | | } + | \_- ... within this type + | + ::: testdata/strict_visibility.proto:18:45 + | +18 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nesting an exported type within another type requires + that that type reserve every field number (the "C++ namespace + exception"), but this type does not reserve the field number 11 + +encountered 7 errors and 4 warnings diff --git a/experimental/ir/testdata/visibility.proto.yaml b/experimental/ir/testdata/visibility.proto.yaml new file mode 100644 index 000000000..6a94c5b93 --- /dev/null +++ b/experimental/ir/testdata/visibility.proto.yaml @@ -0,0 +1,167 @@ +# Copyright 2020-2025 Buf Technologies, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +exclude: ["missing `package`", "missing `syntax`"] +descriptor: true +source_code_info: true +files: +- path: "main.proto" + text: | + edition = "2024"; + package test; + + import "export.proto"; + import "local.proto"; + import "top_level.proto"; + import "strict.proto"; + + message Export { + export.D d = 1; + export.E e = 2; + export.L l = 3; + export.N.D nd = 4; + export.N.E ne = 5; + export.N.L nl = 6; + } + + message Local { + local.D d = 1; + local.E e = 2; + local.L l = 3; + local.N.D nd = 4; + local.N.E ne = 5; + local.N.L nl = 6; + } + + message TopLevel { + top_level.D d = 1; + top_level.E e = 2; + top_level.L l = 3; + top_level.N.D nd = 4; + top_level.N.E ne = 5; + top_level.N.L nl = 6; + } + + message Strict { + strict.D d = 1; + strict.E e = 2; + strict.L l = 3; + strict.N.D nd = 4; + strict.N.E ne = 5; + strict.N.L nl = 6; + } + +- path: "export.proto" + import: true + text: | + edition = "2024"; + package test.export; + + option features.default_symbol_visibility = EXPORT_ALL; + + message N { + message D {} + export message E {} + local message L {} + } + message D {} + export message E {} + local message L {} + + message U { + D d = 1; + E e = 2; + L l = 3; + N.D nd = 4; + N.E ne = 5; + N.L nl = 6; + } + +- path: "local.proto" + import: true + text: | + edition = "2024"; + package test.local; + + option features.default_symbol_visibility = LOCAL_ALL; + + message N { + message D {} + export message E {} + local message L {} + } + message D {} + export message E {} + local message L {} + + message U { + D d = 1; + E e = 2; + L l = 3; + N.D nd = 4; + N.E ne = 5; + N.L nl = 6; + } + +- path: "top_level.proto" + import: true + text: | + edition = "2024"; + package test.top_level; + + option features.default_symbol_visibility = EXPORT_TOP_LEVEL; + + message N { + message D {} + export message E {} + local message L {} + } + message D {} + export message E {} + local message L {} + + message U { + D d = 1; + E e = 2; + L l = 3; + N.D nd = 4; + N.E ne = 5; + N.L nl = 6; + } + +- path: "strict.proto" + import: true + text: | + edition = "2024"; + package test.strict; + + option features.default_symbol_visibility = STRICT; + + message N { + message D {} + export message E {} + local message L {} + } + message D {} + export message E {} + local message L {} + + message U { + D d = 1; + E e = 2; + L l = 3; + N.D nd = 4; + N.E ne = 5; + N.L nl = 6; + } \ No newline at end of file diff --git a/experimental/ir/testdata/visibility.proto.yaml.fds.yaml b/experimental/ir/testdata/visibility.proto.yaml.fds.yaml new file mode 100644 index 000000000..8a55f9c2b --- /dev/null +++ b/experimental/ir/testdata/visibility.proto.yaml.fds.yaml @@ -0,0 +1,372 @@ +file: +- name: "strict.proto" + package: "test.strict" + message_type: + - name: "N" + nested_type: + - name: "D" + - { name: "E", visibility: VISIBILITY_EXPORT } + - { name: "L", visibility: VISIBILITY_LOCAL } + - name: "D" + - { name: "E", visibility: VISIBILITY_EXPORT } + - { name: "L", visibility: VISIBILITY_LOCAL } + - name: "U" + field: + - name: "d" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.D" + json_name: "d" + - name: "e" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.E" + json_name: "e" + - name: "l" + number: 3 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.L" + json_name: "l" + - name: "nd" + number: 4 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.N.D" + json_name: "nd" + - name: "ne" + number: 5 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.N.E" + json_name: "ne" + - name: "nl" + number: 6 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.N.L" + json_name: "nl" + options.features.default_symbol_visibility: STRICT + source_code_info: {} + syntax: "editions" + edition: EDITION_2024 +- name: "top_level.proto" + package: "test.top_level" + message_type: + - name: "N" + nested_type: + - name: "D" + - { name: "E", visibility: VISIBILITY_EXPORT } + - { name: "L", visibility: VISIBILITY_LOCAL } + - name: "D" + - { name: "E", visibility: VISIBILITY_EXPORT } + - { name: "L", visibility: VISIBILITY_LOCAL } + - name: "U" + field: + - name: "d" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.D" + json_name: "d" + - name: "e" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.E" + json_name: "e" + - name: "l" + number: 3 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.L" + json_name: "l" + - name: "nd" + number: 4 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.N.D" + json_name: "nd" + - name: "ne" + number: 5 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.N.E" + json_name: "ne" + - name: "nl" + number: 6 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.N.L" + json_name: "nl" + options.features.default_symbol_visibility: EXPORT_TOP_LEVEL + source_code_info: {} + syntax: "editions" + edition: EDITION_2024 +- name: "local.proto" + package: "test.local" + message_type: + - name: "N" + nested_type: + - name: "D" + - { name: "E", visibility: VISIBILITY_EXPORT } + - { name: "L", visibility: VISIBILITY_LOCAL } + - name: "D" + - { name: "E", visibility: VISIBILITY_EXPORT } + - { name: "L", visibility: VISIBILITY_LOCAL } + - name: "U" + field: + - name: "d" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.D" + json_name: "d" + - name: "e" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.E" + json_name: "e" + - name: "l" + number: 3 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.L" + json_name: "l" + - name: "nd" + number: 4 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.N.D" + json_name: "nd" + - name: "ne" + number: 5 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.N.E" + json_name: "ne" + - name: "nl" + number: 6 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.N.L" + json_name: "nl" + options.features.default_symbol_visibility: LOCAL_ALL + source_code_info: {} + syntax: "editions" + edition: EDITION_2024 +- name: "export.proto" + package: "test.export" + message_type: + - name: "N" + nested_type: + - name: "D" + - { name: "E", visibility: VISIBILITY_EXPORT } + - { name: "L", visibility: VISIBILITY_LOCAL } + - name: "D" + - { name: "E", visibility: VISIBILITY_EXPORT } + - { name: "L", visibility: VISIBILITY_LOCAL } + - name: "U" + field: + - name: "d" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.D" + json_name: "d" + - name: "e" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.E" + json_name: "e" + - name: "l" + number: 3 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.L" + json_name: "l" + - name: "nd" + number: 4 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.N.D" + json_name: "nd" + - name: "ne" + number: 5 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.N.E" + json_name: "ne" + - name: "nl" + number: 6 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.N.L" + json_name: "nl" + options.features.default_symbol_visibility: EXPORT_ALL + source_code_info: {} + syntax: "editions" + edition: EDITION_2024 +- name: "main.proto" + package: "test" + dependency: ["export.proto", "local.proto", "top_level.proto", "strict.proto"] + message_type: + - name: "Export" + field: + - name: "d" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.D" + json_name: "d" + - name: "e" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.E" + json_name: "e" + - name: "l" + number: 3 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.L" + json_name: "l" + - name: "nd" + number: 4 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.N.D" + json_name: "nd" + - name: "ne" + number: 5 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.N.E" + json_name: "ne" + - name: "nl" + number: 6 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.export.N.L" + json_name: "nl" + - name: "Local" + field: + - name: "d" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.D" + json_name: "d" + - name: "e" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.E" + json_name: "e" + - name: "l" + number: 3 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.L" + json_name: "l" + - name: "nd" + number: 4 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.N.D" + json_name: "nd" + - name: "ne" + number: 5 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.N.E" + json_name: "ne" + - name: "nl" + number: 6 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.local.N.L" + json_name: "nl" + - name: "TopLevel" + field: + - name: "d" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.D" + json_name: "d" + - name: "e" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.E" + json_name: "e" + - name: "l" + number: 3 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.L" + json_name: "l" + - name: "nd" + number: 4 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.N.D" + json_name: "nd" + - name: "ne" + number: 5 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.N.E" + json_name: "ne" + - name: "nl" + number: 6 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.top_level.N.L" + json_name: "nl" + - name: "Strict" + field: + - name: "d" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.D" + json_name: "d" + - name: "e" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.E" + json_name: "e" + - name: "l" + number: 3 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.L" + json_name: "l" + - name: "nd" + number: 4 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.N.D" + json_name: "nd" + - name: "ne" + number: 5 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.N.E" + json_name: "ne" + - name: "nl" + number: 6 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".test.strict.N.L" + json_name: "nl" + source_code_info: {} + syntax: "editions" + edition: EDITION_2024 diff --git a/experimental/ir/testdata/visibility.proto.yaml.stderr.txt b/experimental/ir/testdata/visibility.proto.yaml.stderr.txt new file mode 100644 index 000000000..f83e1758d --- /dev/null +++ b/experimental/ir/testdata/visibility.proto.yaml.stderr.txt @@ -0,0 +1,289 @@ +error: sorry, Edition 2024 is not fully implemented + --> export.proto:1:11 + | + 1 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +warning: redundant visibility modifier + --> export.proto:8:3 + | + 8 | export message E {} + | ^^^^^^ specified here + | + ::: export.proto:4:45 + | + 4 | option features.default_symbol_visibility = EXPORT_ALL; + | ---------- this implies it + +warning: redundant visibility modifier + --> export.proto:12:1 + | +12 | export message E {} + | ^^^^^^ specified here + | + ::: export.proto:4:45 + | + 4 | option features.default_symbol_visibility = EXPORT_ALL; + | ---------- this implies it + +error: sorry, Edition 2024 is not fully implemented + --> local.proto:1:11 + | + 1 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +warning: redundant visibility modifier + --> local.proto:9:3 + | + 9 | local message L {} + | ^^^^^ specified here + | + ::: local.proto:4:45 + | + 4 | option features.default_symbol_visibility = LOCAL_ALL; + | --------- this implies it + +warning: redundant visibility modifier + --> local.proto:13:1 + | +13 | local message L {} + | ^^^^^ specified here + | + ::: local.proto:4:45 + | + 4 | option features.default_symbol_visibility = LOCAL_ALL; + | --------- this implies it + +error: sorry, Edition 2024 is not fully implemented + --> main.proto:1:11 + | + 1 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +error: found unexported message type `test.export.L` + --> main.proto:12:3 + | +12 | export.L l = 3; + | ^^^^^^^^ unexported type + | + ::: export.proto:13:1 + | +13 | local message L {} + | ----- marked as local here + +error: found unexported message type `test.export.N.L` + --> main.proto:15:3 + | +15 | export.N.L nl = 6; + | ^^^^^^^^^^ unexported type + | + ::: export.proto:9:3 + | + 9 | local message L {} + | ----- marked as local here + +error: found unexported message type `test.local.D` + --> main.proto:19:3 + | +19 | local.D d = 1; + | ^^^^^^^ unexported type + | + ::: local.proto:4:45 + | + 4 | option features.default_symbol_visibility = LOCAL_ALL; + | --------- this implies `local` + +error: found unexported message type `test.local.L` + --> main.proto:21:3 + | +21 | local.L l = 3; + | ^^^^^^^ unexported type + | + ::: local.proto:13:1 + | +13 | local message L {} + | ----- marked as local here + +error: found unexported message type `test.local.N.D` + --> main.proto:22:3 + | +22 | local.N.D nd = 4; + | ^^^^^^^^^ unexported type + | + ::: local.proto:4:45 + | + 4 | option features.default_symbol_visibility = LOCAL_ALL; + | --------- this implies `local` + +error: found unexported message type `test.local.N.L` + --> main.proto:24:3 + | +24 | local.N.L nl = 6; + | ^^^^^^^^^ unexported type + | + ::: local.proto:9:3 + | + 9 | local message L {} + | ----- marked as local here + +error: found unexported message type `test.top_level.L` + --> main.proto:30:3 + | +30 | top_level.L l = 3; + | ^^^^^^^^^^^ unexported type + | + ::: top_level.proto:13:1 + | +13 | local message L {} + | ----- marked as local here + +error: found unexported message type `test.top_level.N.D` + --> main.proto:31:3 + | +31 | top_level.N.D nd = 4; + | ^^^^^^^^^^^^^ unexported type + | + ::: top_level.proto:4:45 + | + 4 | option features.default_symbol_visibility = EXPORT_TOP_LEVEL; + | ---------------- + | | + | this implies `local` + +error: found unexported message type `test.top_level.N.L` + --> main.proto:33:3 + | +33 | top_level.N.L nl = 6; + | ^^^^^^^^^^^^^ unexported type + | + ::: top_level.proto:9:3 + | + 9 | local message L {} + | ----- marked as local here + +error: found unexported message type `test.strict.D` + --> main.proto:37:3 + | +37 | strict.D d = 1; + | ^^^^^^^^ unexported type + | + ::: strict.proto:4:45 + | + 4 | option features.default_symbol_visibility = STRICT; + | ------ this implies `local` + +error: found unexported message type `test.strict.L` + --> main.proto:39:3 + | +39 | strict.L l = 3; + | ^^^^^^^^ unexported type + | + ::: strict.proto:13:1 + | +13 | local message L {} + | ----- marked as local here + +error: found unexported message type `test.strict.N.D` + --> main.proto:40:3 + | +40 | strict.N.D nd = 4; + | ^^^^^^^^^^ unexported type + | + ::: strict.proto:4:45 + | + 4 | option features.default_symbol_visibility = STRICT; + | ------ this implies `local` + +error: found unexported message type `test.strict.N.L` + --> main.proto:42:3 + | +42 | strict.N.L nl = 6; + | ^^^^^^^^^^ unexported type + | + ::: strict.proto:9:3 + | + 9 | local message L {} + | ----- marked as local here + +error: sorry, Edition 2024 is not fully implemented + --> strict.proto:1:11 + | + 1 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +error: message type `test.strict.N` does not reserve all field numbers + --> strict.proto:8:3 + | + 6 | / message N { + 7 | | message D {} + 8 | | export message E {} + | | ^^^^^^ nested type exported here + 9 | | local message L {} +10 | | } + | \_- ... within this type + | + ::: strict.proto:4:45 + | + 4 | option features.default_symbol_visibility = STRICT; + | ------ `STRICT` specified here + | + = help: in strict mode, nesting an exported type within another type requires + that that type reserve every field number (the "C++ namespace + exception"), but this type does not reserve the field number 1 + +warning: redundant visibility modifier + --> strict.proto:9:3 + | + 9 | local message L {} + | ^^^^^ specified here + | + ::: strict.proto:4:45 + | + 4 | option features.default_symbol_visibility = STRICT; + | ------ this implies it + +warning: redundant visibility modifier + --> strict.proto:13:1 + | +13 | local message L {} + | ^^^^^ specified here + | + ::: strict.proto:4:45 + | + 4 | option features.default_symbol_visibility = STRICT; + | ------ this implies it + +error: sorry, Edition 2024 is not fully implemented + --> top_level.proto:1:11 + | + 1 | edition = "2024"; + | ^^^^^^ + = help: Edition 2024 will be implemented in a future release + +warning: redundant visibility modifier + --> top_level.proto:9:3 + | + 9 | local message L {} + | ^^^^^ specified here + | + ::: top_level.proto:4:45 + | + 4 | option features.default_symbol_visibility = EXPORT_TOP_LEVEL; + | ---------------- this implies it + +warning: redundant visibility modifier + --> top_level.proto:12:1 + | +12 | export message E {} + | ^^^^^^ specified here + | + ::: top_level.proto:4:45 + | + 4 | option features.default_symbol_visibility = EXPORT_TOP_LEVEL; + | ---------------- this implies it + +encountered 19 errors and 8 warnings diff --git a/experimental/parser/diagnostics_internal.go b/experimental/parser/diagnostics_internal.go index cf98569e4..812ee81fa 100644 --- a/experimental/parser/diagnostics_internal.go +++ b/experimental/parser/diagnostics_internal.go @@ -204,44 +204,6 @@ func (e errBadNest) Diagnose(d *report.Diagnostic) { } } -// errRequiresEdition diagnoses that a certain edition is required for a feature. -// -//nolint:govet // Irrelevant alignment padding lint. -type errRequiresEdition struct { - edition syntax.Syntax - node report.Spanner - what any - decl ast.DeclSyntax - - // If set, this will report that the feature is not implemented instead. - unimplemented bool -} - -func (e errRequiresEdition) Diagnose(d *report.Diagnostic) { - what := e.what - if what == nil { - what = taxa.Classify(e.node) - } - - if e.unimplemented { - d.Apply( - report.Message("sorry, %s is not implemented yet", what), - report.Snippet(e.node), - report.Helpf("%s is part of Edition %s, which will be implemented in a future release", what, e.edition), - ) - return - } - - d.Apply( - report.Message("%s requires Edition %s or later", what, e.edition), - report.Snippet(e.node), - ) - - if !e.decl.IsZero() { - report.Snippetf(e.decl.Value(), "%s specified here", e.decl.Keyword()) - } -} - // errUnexpectedMod diagnoses a modifier placed in the wrong position. type errUnexpectedMod struct { mod token.Token diff --git a/experimental/parser/legalize_def.go b/experimental/parser/legalize_def.go index 7f7f1a43f..103317631 100644 --- a/experimental/parser/legalize_def.go +++ b/experimental/parser/legalize_def.go @@ -19,6 +19,7 @@ import ( "github.com/bufbuild/protocompile/experimental/ast" "github.com/bufbuild/protocompile/experimental/ast/syntax" + "github.com/bufbuild/protocompile/experimental/internal/erredition" "github.com/bufbuild/protocompile/experimental/internal/taxa" "github.com/bufbuild/protocompile/experimental/report" "github.com/bufbuild/protocompile/experimental/token/keyword" @@ -116,13 +117,15 @@ func legalizeTypeDefLike(p *parser, what taxa.Noun, def ast.DeclDef) { isType := what == taxa.Message || what == taxa.Enum if isType && mod.Prefix().IsTypeModifier() { - p.Error(errRequiresEdition{ - edition: syntax.Edition2024, - node: mod.PrefixToken(), - decl: p.syntaxNode, - - unimplemented: p.syntax >= syntax.Edition2024, - }) + if p.syntax < syntax.Edition2024 { + p.Error(erredition.TooOld{ + Current: p.syntax, + Decl: p.syntaxNode, + Intro: syntax.Edition2024, + What: mod.Prefix(), + Where: mod.PrefixToken(), + }) + } continue } diff --git a/experimental/parser/parse_decl.go b/experimental/parser/parse_decl.go index c27b3fd1c..78136a19f 100644 --- a/experimental/parser/parse_decl.go +++ b/experimental/parser/parse_decl.go @@ -101,7 +101,7 @@ func parseDecl(p *parser, c *token.Cursor, in taxa.Noun) ast.DeclAny { // This is used here to disambiguated between a generic DeclDef and one of // the other decl nodes. var kw token.Token - if path := ty.AsPath(); !path.IsZero() { + if path := ty.RemovePrefixes().AsPath(); !path.IsZero() { kw = path.AsIdent() } diff --git a/experimental/parser/testdata/parser/def/2024.proto.stderr.txt b/experimental/parser/testdata/parser/def/2024.proto.stderr.txt index 711051413..fe141323c 100644 --- a/experimental/parser/testdata/parser/def/2024.proto.stderr.txt +++ b/experimental/parser/testdata/parser/def/2024.proto.stderr.txt @@ -5,14 +5,6 @@ error: sorry, Edition 2024 is not fully implemented | ^^^^^^ = help: Edition 2024 will be implemented in a future release -error: sorry, `export` is not implemented yet - --> testdata/parser/def/2024.proto:19:1 - | -19 | export message Foo1 { - | ^^^^^^ - = help: `export` is part of Edition 2024, which will be implemented in a - future release - error: unexpected `export` modifier on message field --> testdata/parser/def/2024.proto:22:5 | @@ -37,30 +29,6 @@ error: unexpected `local` modifier on message field | = help: `local` only applies to a type definition -error: sorry, `local` is not implemented yet - --> testdata/parser/def/2024.proto:25:1 - | -25 | local message Foo2 {} - | ^^^^^ - = help: `local` is part of Edition 2024, which will be implemented in a - future release - -error: sorry, `export` is not implemented yet - --> testdata/parser/def/2024.proto:27:1 - | -27 | export enum Foo3 {} - | ^^^^^^ - = help: `export` is part of Edition 2024, which will be implemented in a - future release - -error: sorry, `local` is not implemented yet - --> testdata/parser/def/2024.proto:28:1 - | -28 | local enum Foo4 {} - | ^^^^^ - = help: `local` is part of Edition 2024, which will be implemented in a - future release - error: unexpected `export` modifier on service definition --> testdata/parser/def/2024.proto:30:1 | @@ -85,4 +53,4 @@ error: unexpected `local` modifier on service definition | = help: `local` only applies to a type definition -encountered 9 errors +encountered 5 errors diff --git a/internal/interval/intersect.go b/internal/interval/intersect.go index 2ddd3bf94..3cdce47af 100644 --- a/internal/interval/intersect.go +++ b/internal/interval/intersect.go @@ -81,6 +81,46 @@ func (m *Intersect[K, V]) Entries() iter.Seq[Entry[K, []V]] { } } +// Ranges returns an iterator over the contiguous ranges in this map. +// +// If values is true, the yielded entries will include all of the values that +// fall within those contiguous ranges. +func (m *Intersect[K, V]) Contiguous(values bool) iter.Seq[Entry[K, []V]] { + return func(yield func(Entry[K, []V]) bool) { + iter := m.tree.Iter() + var current Entry[K, []V] + first := true + for more := iter.First(); more; more = iter.Next() { + entry := iter.Value() + if first { + current = *entry + if !values { + current.Value = nil + } + + first = false + } else if current.End+1 == entry.Start { + current.End = entry.End + if values { + current.Value = append(current.Value, entry.Value...) + } + } else { + if !yield(current) { + return + } + current = *entry + if !values { + current.Value = nil + } + } + } + + if !first { + yield(current) + } + } +} + // Insert inserts a new interval into this map, with the given associated value. // Both endpoints are inclusive. // diff --git a/internal/interval/intersect_test.go b/internal/interval/intersect_test.go index 25435aa44..04482c870 100644 --- a/internal/interval/intersect_test.go +++ b/internal/interval/intersect_test.go @@ -36,6 +36,7 @@ func TestInsert(t *testing.T) { name string ranges []in // Ranges to insert. want []out + join []out }{ { name: "empty-map", @@ -43,6 +44,7 @@ func TestInsert(t *testing.T) { want: []out{ {0, 9, []string{"foo"}}, }, + join: []out{{0, 9, nil}}, }, { name: "new-max", @@ -54,6 +56,7 @@ func TestInsert(t *testing.T) { {0, 9, []string{"foo"}}, {30, 39, []string{"bar"}}, }, + join: []out{{0, 9, nil}, {30, 39, nil}}, }, { name: "new-min", @@ -65,6 +68,7 @@ func TestInsert(t *testing.T) { {0, 9, []string{"foo"}}, {30, 39, []string{"bar"}}, }, + join: []out{{0, 9, nil}, {30, 39, nil}}, }, { @@ -79,6 +83,7 @@ func TestInsert(t *testing.T) { {20, 25, []string{"baz"}}, {30, 39, []string{"bar"}}, }, + join: []out{{0, 9, nil}, {20, 25, nil}, {30, 39, nil}}, }, { name: "case-1", @@ -92,6 +97,7 @@ func TestInsert(t *testing.T) { {20, 29, []string{"baz"}}, {30, 39, []string{"bar"}}, }, + join: []out{{0, 9, nil}, {20, 39, nil}}, }, { name: "case-1", @@ -105,6 +111,7 @@ func TestInsert(t *testing.T) { {10, 19, []string{"baz"}}, {30, 39, []string{"bar"}}, }, + join: []out{{0, 19, nil}, {30, 39, nil}}, }, { name: "case-1", @@ -118,6 +125,7 @@ func TestInsert(t *testing.T) { {10, 29, []string{"baz"}}, {30, 39, []string{"bar"}}, }, + join: []out{{0, 39, nil}}, }, { @@ -131,6 +139,7 @@ func TestInsert(t *testing.T) { {1, 2, []string{"foo", "baz"}}, {3, 9, []string{"foo"}}, }, + join: []out{{0, 9, nil}}, }, { name: "case-2", @@ -142,6 +151,7 @@ func TestInsert(t *testing.T) { {0, 2, []string{"foo", "baz"}}, {3, 9, []string{"foo"}}, }, + join: []out{{0, 9, nil}}, }, { name: "case-2", @@ -152,6 +162,7 @@ func TestInsert(t *testing.T) { want: []out{ {0, 9, []string{"foo", "baz"}}, }, + join: []out{{0, 9, nil}}, }, { @@ -165,6 +176,7 @@ func TestInsert(t *testing.T) { {9, 9, []string{"foo", "baz"}}, {10, 12, []string{"baz"}}, }, + join: []out{{0, 12, nil}}, }, { name: "case-3", @@ -179,6 +191,7 @@ func TestInsert(t *testing.T) { {10, 12, []string{"baz"}}, {30, 39, []string{"bar"}}, }, + join: []out{{0, 12, nil}, {30, 39, nil}}, }, { name: "case-3", @@ -193,6 +206,7 @@ func TestInsert(t *testing.T) { {10, 29, []string{"baz"}}, {30, 39, []string{"bar"}}, }, + join: []out{{0, 39, nil}}, }, { name: "case-3", @@ -208,6 +222,7 @@ func TestInsert(t *testing.T) { {30, 30, []string{"bar", "baz"}}, {31, 39, []string{"bar"}}, }, + join: []out{{0, 39, nil}}, }, { @@ -221,6 +236,7 @@ func TestInsert(t *testing.T) { {0, 0, []string{"foo", "baz"}}, {1, 10, []string{"foo"}}, }, + join: []out{{-2, 10, nil}}, }, { name: "case-4", @@ -235,6 +251,7 @@ func TestInsert(t *testing.T) { {30, 32, []string{"bar", "baz"}}, {33, 39, []string{"bar"}}, }, + join: []out{{0, 9, nil}, {20, 39, nil}}, }, { name: "case-4", @@ -249,6 +266,7 @@ func TestInsert(t *testing.T) { {30, 32, []string{"bar", "baz"}}, {33, 39, []string{"bar"}}, }, + join: []out{{0, 39, nil}}, }, { @@ -262,6 +280,7 @@ func TestInsert(t *testing.T) { {0, 9, []string{"foo", "baz"}}, {10, 12, []string{"baz"}}, }, + join: []out{{-2, 12, nil}}, }, { name: "case-5", @@ -276,6 +295,7 @@ func TestInsert(t *testing.T) { {10, 29, []string{"baz"}}, {30, 39, []string{"bar"}}, }, + join: []out{{-2, 39, nil}}, }, { name: "case-5", @@ -291,6 +311,7 @@ func TestInsert(t *testing.T) { {30, 30, []string{"bar", "baz"}}, {31, 39, []string{"bar"}}, }, + join: []out{{-2, 39, nil}}, }, { name: "case-5", @@ -305,6 +326,7 @@ func TestInsert(t *testing.T) { {30, 39, []string{"bar", "baz"}}, {40, 40, []string{"baz"}}, }, + join: []out{{0, 9, nil}, {29, 40, nil}}, }, { name: "case-5", @@ -319,6 +341,7 @@ func TestInsert(t *testing.T) { {30, 39, []string{"bar", "baz"}}, {40, math.MaxInt, []string{"baz"}}, }, + join: []out{{0, 9, nil}, {29, math.MaxInt, nil}}, }, } @@ -332,6 +355,7 @@ func TestInsert(t *testing.T) { } assert.Equal(t, tt.want, slices.Collect(m.Entries())) + assert.Equal(t, tt.join, slices.Collect(m.Contiguous(false))) }) } } From 1887a31356c4658e15eac37d179ec0de67d4e3be Mon Sep 17 00:00:00 2001 From: Miguel Young de la Sota Date: Wed, 5 Nov 2025 14:19:56 -0800 Subject: [PATCH 4/4] lint --- .protoc_version | 2 +- experimental/ast/syntax/name.go | 14 +++++++++ .../internal/erredition/erredition.go | 21 ++++++++++--- experimental/internal/erredition/normalize.go | 14 +++++++++ .../internal/erredition/normalize_test.go | 14 +++++++++ experimental/ir/lower_features.go | 3 -- experimental/ir/lower_validate.go | 4 ++- .../testdata/options/missing.proto.stderr.txt | 8 ++--- internal/interval/intersect.go | 9 ++++-- internal/testdata/all.protoset | Bin 38056 -> 38070 bytes internal/testdata/desc_test_complex.protoset | Bin 19654 -> 19668 bytes .../desc_test_proto3_optional.protoset | Bin 13507 -> 13521 bytes .../testdata/descriptor_impl_tests.protoset | Bin 27701 -> 27715 bytes .../google/protobuf/descriptor.proto | 4 --- .../google/protobuf/go_features.proto | 29 ------------------ .../google/protobuf/timestamp.proto | 2 +- wellknownimports/update.sh | 20 ------------ 17 files changed, 74 insertions(+), 70 deletions(-) delete mode 100755 wellknownimports/update.sh diff --git a/.protoc_version b/.protoc_version index 593224b3f..244a88ae0 100644 --- a/.protoc_version +++ b/.protoc_version @@ -1 +1 @@ -32.0 +33.0 diff --git a/experimental/ast/syntax/name.go b/experimental/ast/syntax/name.go index c2705e5bb..1b2fa7b25 100644 --- a/experimental/ast/syntax/name.go +++ b/experimental/ast/syntax/name.go @@ -1,3 +1,17 @@ +// Copyright 2020-2025 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package syntax import ( diff --git a/experimental/internal/erredition/erredition.go b/experimental/internal/erredition/erredition.go index 6ffe82214..05056992b 100644 --- a/experimental/internal/erredition/erredition.go +++ b/experimental/internal/erredition/erredition.go @@ -1,3 +1,17 @@ +// Copyright 2020-2025 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + // Package erredition defines common diagnostics for issuing errors about // the wrong edition being used. package erredition @@ -10,12 +24,11 @@ import ( // TooNew diagnoses an edition that is too old for the feature used. type TooOld struct { - Current syntax.Syntax + What any + Where report.Spanner Decl ast.DeclSyntax + Current syntax.Syntax Intro syntax.Syntax - - What any - Where report.Spanner } // Diagnose implements [report.Diagnoser]. diff --git a/experimental/internal/erredition/normalize.go b/experimental/internal/erredition/normalize.go index 4d80c8d5c..f2445627f 100644 --- a/experimental/internal/erredition/normalize.go +++ b/experimental/internal/erredition/normalize.go @@ -1,3 +1,17 @@ +// Copyright 2020-2025 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package erredition import ( diff --git a/experimental/internal/erredition/normalize_test.go b/experimental/internal/erredition/normalize_test.go index 223dc17da..4167cb224 100644 --- a/experimental/internal/erredition/normalize_test.go +++ b/experimental/internal/erredition/normalize_test.go @@ -1,3 +1,17 @@ +// Copyright 2020-2025 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package erredition import ( diff --git a/experimental/ir/lower_features.go b/experimental/ir/lower_features.go index 6f8ec2c0a..9f80ca08c 100644 --- a/experimental/ir/lower_features.go +++ b/experimental/ir/lower_features.go @@ -16,7 +16,6 @@ package ir import ( "cmp" - "regexp" "slices" "github.com/bufbuild/protocompile/experimental/ast/predeclared" @@ -28,8 +27,6 @@ import ( "github.com/bufbuild/protocompile/internal/ext/slicesx" ) -var whitespacePattern = regexp.MustCompile(`[ \t\r\n]+`) - func buildAllFeatureInfo(file *File, r *report.Report) { for m := range file.AllMembers() { if !m.IsEnumValue() { diff --git a/experimental/ir/lower_validate.go b/experimental/ir/lower_validate.go index 636bed089..3b4c8b76a 100644 --- a/experimental/ir/lower_validate.go +++ b/experimental/ir/lower_validate.go @@ -1097,7 +1097,7 @@ func validateVisibility(ty Type, r *report.Report) { start, end := parent.AbsoluteRange() // Find any gaps in the reserved ranges. - gap := int32(start) + gap := start ranges := slices.Collect(seq.Values(parent.ReservedRanges())) if len(ranges) > 0 { slices.SortFunc(ranges, cmpx.Join( @@ -1146,6 +1146,7 @@ func validateVisibility(ty Type, r *report.Report) { report.Snippetf(ranges.At(1).AST(), "another here"), ) } + //nolint:dupword d.Apply( report.PageBreak, report.Snippetf(why, "`STRICT` specified here"), @@ -1170,6 +1171,7 @@ func validateVisibility(ty Type, r *report.Report) { // If this is true, the protoc check is bugged and we emit a warning... bugged := parent.ReservedRanges().Len() == 1 + //nolint:dupword d := r.SoftErrorf(!bugged, "%s `%s` does not reserve all field numbers", parent.noun(), parent.FullName()).Apply( report.Snippetf(vis, "nested type exported here"), report.Snippetf(parent.AST(), "... within this type"), diff --git a/experimental/ir/testdata/options/missing.proto.stderr.txt b/experimental/ir/testdata/options/missing.proto.stderr.txt index f6581b781..876145dfd 100644 --- a/experimental/ir/testdata/options/missing.proto.stderr.txt +++ b/experimental/ir/testdata/options/missing.proto.stderr.txt @@ -87,9 +87,9 @@ error: expected singular message, found scalar type `bool` | | | found scalar type `bool` | - ::: google/protobuf/descriptor.proto:866:12 + ::: google/protobuf/descriptor.proto:862:12 | -866 | optional bool deprecated = 3 [default = false]; +862 | optional bool deprecated = 3 [default = false]; | ---- type specified here error: cannot find message field `unknown` in `google.protobuf.EnumValueOptions` @@ -108,9 +108,9 @@ error: expected singular message, found scalar type `bool` | | | field selector requires singular message | - ::: google/protobuf/descriptor.proto:896:12 + ::: google/protobuf/descriptor.proto:892:12 | -896 | optional bool deprecated = 1 [default = false]; +892 | optional bool deprecated = 1 [default = false]; | ---- type specified here encountered 12 errors diff --git a/internal/interval/intersect.go b/internal/interval/intersect.go index 3cdce47af..bebcdeb88 100644 --- a/internal/interval/intersect.go +++ b/internal/interval/intersect.go @@ -92,19 +92,22 @@ func (m *Intersect[K, V]) Contiguous(values bool) iter.Seq[Entry[K, []V]] { first := true for more := iter.First(); more; more = iter.Next() { entry := iter.Value() - if first { + switch { + case first: current = *entry if !values { current.Value = nil } first = false - } else if current.End+1 == entry.Start { + + case current.End+1 == entry.Start: current.End = entry.End if values { current.Value = append(current.Value, entry.Value...) } - } else { + + default: if !yield(current) { return } diff --git a/internal/testdata/all.protoset b/internal/testdata/all.protoset index 27e7fa8c9520f74d19852502862c4e6976170afc..c4edd2367e133905ae3b0c358665b9abac526a58 100644 GIT binary patch delta 56 zcmV-80LTBRsRFjC00NMYxsRF2}0$lkv(-Mi#E_bgs$Ej1@Pk-sR)o%E%=aoLW?tnVdQ~fmvm;A%7L07YhfA0BaCi PaY<2XV(#YE{L;1nN^lZp delta 39 vcmcaIlkwP0MrN+RX&agU^Kma{=*@u(y@ $WKT" - curl "$RAW_URL/$SRC" > "$WKT" -f -done