Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

editions: compiler disallows extension to have default value when file sets default presence to implicit #16664

Closed
jhump opened this issue Apr 29, 2024 · 4 comments · Fixed by #16714
Assignees
Labels
untriaged auto added to all issues by default when created.

Comments

@jhump
Copy link
Contributor

jhump commented Apr 29, 2024

Extensions always have explicit presence. If an attempt is made to set the field_presence feature to IMPLICIT, the result is an error: "Extensions can't specify field presence".

Unfortunately, if the file default is set to IMPLICIT, protoc is acting as if the extension's field presence inherits this and does not allow setting a default value.

// test.proto
edition = "2023";
option features.field_presence = IMPLICIT;
message Extendee {
  extensions 1 to 2;
}
extend Extendee {
  uint32 ext_with_default = 1 [default = 123];
}
test.proto:8:10: Implicit presence fields can't specify defaults.
@jhump jhump added the untriaged auto added to all issues by default when created. label Apr 29, 2024
@mkruskal-google
Copy link
Member

Nice, thanks for the thoroughness here Josh :)

@mkruskal-google
Copy link
Member

This turned up a related issue: repeated closed enum fields are an error condition when they "inherit" IMPLICIT presence

copybara-service bot pushed a commit that referenced this issue May 2, 2024
Instead of checking the resolved features, we should really be checking the has_presence helper.  Repeated fields, oneofs, and extensions can trigger these conditions when they inherit IMPLICIT, even though it's ignored.

Closes #16664

PiperOrigin-RevId: 629924397
copybara-service bot pushed a commit that referenced this issue May 2, 2024
Instead of checking the resolved features, we should really be checking the has_presence helper.  Repeated fields, oneofs, and extensions can trigger these conditions when they inherit IMPLICIT, even though it's ignored.

Closes #16664

PiperOrigin-RevId: 629924397
@jhump
Copy link
Contributor Author

jhump commented May 2, 2024

This turned up a related issue: repeated closed enum fields are an error condition when they "inherit" IMPLICIT presence

Oh, interesting. I assume the intent is for both of these bugs to be fixed for the v27.0 release? I'll go ahead and test cases to protocompile in the meantime.

@mkruskal-google
Copy link
Member

Yep, I'm changing the validation check to use has_presence (i.e. the actual computed behavior) instead of the resolved feature value

copybara-service bot pushed a commit that referenced this issue May 2, 2024
Instead of checking the resolved features, we should really be checking the has_presence helper.  Repeated fields, oneofs, and extensions can trigger these conditions when they inherit IMPLICIT, even though it's ignored.

Closes #16664

PiperOrigin-RevId: 629924397
mkruskal-google added a commit to mkruskal-google/protobuf that referenced this issue May 2, 2024
Instead of checking the resolved features, we should really be checking the has_presence helper.  Repeated fields, oneofs, and extensions can trigger these conditions when they inherit IMPLICIT, even though it's ignored.

Closes protocolbuffers#16664

PiperOrigin-RevId: 630206208
deannagarcia pushed a commit to deannagarcia/protobuf that referenced this issue Jun 20, 2024
Instead of checking the resolved features, we should really be checking the has_presence helper.  Repeated fields, oneofs, and extensions can trigger these conditions when they inherit IMPLICIT, even though it's ignored.

Closes protocolbuffers#16664

PiperOrigin-RevId: 630206208
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
untriaged auto added to all issues by default when created.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants