Skip to content

Fix extension validation for proto2 required fields#746

Merged
doriable merged 1 commit into
mainfrom
BSRT-5-required-field-extension
Jul 20, 2026
Merged

Fix extension validation for proto2 required fields#746
doriable merged 1 commit into
mainfrom
BSRT-5-required-field-extension

Conversation

@doriable

Copy link
Copy Markdown
Member

This PR adds validation for non-editions features required
fields on extensions.

@doriable
doriable requested a review from emcfarlane July 20, 2026 19:33
} else {
d.Apply(report.Snippet(m.AST()))
}
d.Apply(report.Helpf("extensions cannot use the `required` label"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

label the right term here, what about a test for LEGACY_REQUIRED? extensions can't be required. would maybe fit both patterns better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LEGACY_REQUIRED goes through the features, so the output you get is:

expected singular field, found extension

Which is basically the unexpected error... I had considered making them as similar as possible, in terms of the diagnostics, but I wasn't sure if that made sense in this case? So I just kind of kept them as separate diagnostics...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nicer to make it explicit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And as for the test case with LEGACY_REQUIRED -- I can add a test case, but proto2 files cannot set features, so it's not really a problem right now:

            error: `features` cannot be set in "proto2"
              --> testdata/extend/required.proto:16:33
               |
             1 | syntax = "proto2";
               |          -------- syntax specified here
            ...
            16 |     required string meow = 103 [features.field_presence = LEGACY_REQUIRED];
               |                                 ^^^^^^^^

            error: `field_presence` is not supported in "proto2"
              --> testdata/extend/required.proto:16:33
               |
             1 | syntax = "proto2";
               |          -------- syntax specified here
            ...
            16 |     required string meow = 103 [features.field_presence = LEGACY_REQUIRED];
               |                                 ^^^^^^^^^^^^^^^^^^^^^^^
               |
               = help: `field_presence` requires at least Edition 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nicer to make it explicit.

So the full diagnostic is explicit, it has help statement that says field_present cannot be set on extensions:

error: expected singular field, found extension
--> testdata/options/validate/presence.proto:26:5
|
26 | int32 e = 4 [features.field_presence = EXPLICIT];
| ^^^^^^^^^^^^^-----------------------^^^^^^^^^^^^^
| |
| `field_presence` set here
|
= help: `field_presence` cannot be set on extensions
= help: all singular extensions have explicit presence

The output I provided is from buf, which currently doesn't use the full diagnostic report, so it doesn't appear to be explicit... but yeah...

Comment on lines +771 to +773
d.Apply(report.Snippet(required.PrefixToken()))
} else {
d.Apply(report.Snippet(m.AST()))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
d.Apply(report.Snippet(required.PrefixToken()))
} else {
d.Apply(report.Snippet(m.AST()))
}); !required.IsZero() {
d.Apply(report.Snippet(required.PrefixToken()))
d.Apply(report.Helpf("extensions cannot use the `required` label"))
} else {
d.Apply(report.Snippet(m.AST()))
d.Apply(report.Helpf("extensions cannot set `field_presence` to `LEGACY_REQUIRED`"))
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ re: conversation in comment above, this is only checking for required field, and the features.field_presence is checked below with different diagnostics, so we shouldn't mix the diagnostics here.

@doriable
doriable merged commit 9cf9183 into main Jul 20, 2026
11 checks passed
@doriable
doriable deleted the BSRT-5-required-field-extension branch July 20, 2026 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants