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

protoc: reserved names should be identifiers in the grammar, not strings #13440

Closed
jhump opened this issue Aug 2, 2023 · 5 comments
Closed
Assignees
Labels
inactive Denotes the issue/PR has not seen activity in the last 90 days. protoc

Comments

@jhump
Copy link
Contributor

jhump commented Aug 2, 2023

As a follow on from #6335, the grammar for reserved names should be updated to support identifiers as values, not just strings:

- reserved = "reserved" ( ranges | strFieldNames ) ";"
+ reserved = "reserved" ( ranges | strFieldNames | fieldNames ) ";"
  ranges = range { "," range }
  range =  intLit [ "to" ( intLit | "max" ) ]
  strFieldNames = strFieldName { "," strFieldName }
  strFieldName = "'" fieldName "'" | '"' fieldName '"'
+ fieldNames = fieldName { "," fieldName }

Bare identifiers aren't as error-prone since there's no risk of forgetting a comma causing the parser to concatenate adjacent tokens into an unintended reserved name.

Eventually, it would be nice to deprecate or remove support for using string values, but removal would be a backwards-incompatible change.

@jhump jhump added the untriaged auto added to all issues by default when created. label Aug 2, 2023
@zhangskz zhangskz added the protoc label Aug 2, 2023
@fowles fowles removed the untriaged auto added to all issues by default when created. label Aug 7, 2023
@fowles
Copy link
Member

fowles commented Aug 7, 2023

@Logofile for the doc part of this

@jhump
Copy link
Contributor Author

jhump commented Aug 7, 2023

@fowles, @Logofile, just to be clear, this isn't a doc bug for the grammar, but a proposal to change the actual parser to accept identifiers (instead of string literals).

This was discussed in a tier 1 review on October 7th, 2022. IIUC, this part of the proposal (the grammar/parser change) was to be delayed until "edition zero". I opened this issue since it looks like "edition zero" is getting close and also for posterity, so this proposal is documented in GitHub.

@Logofile
Copy link
Member

Logofile commented Aug 7, 2023

Ah! That's the piece I was missing. I will add this to the list of things that need to be documented for Editions.

copybara-service bot pushed a commit that referenced this issue Aug 10, 2023
…iterals (only in editions) (#13471)

This addresses #13440.

@mkruskal-google, I saw you are assigned the above issue. I hope it's okay that I took a stab at it.

Closes #13471

COPYBARA_INTEGRATE_REVIEW=#13471 from jhump:jh/reserved-names-support-identifiers a23f18e
FUTURE_COPYBARA_INTEGRATE_REVIEW=#13471 from jhump:jh/reserved-names-support-identifiers a23f18e
PiperOrigin-RevId: 555559034
copybara-service bot pushed a commit that referenced this issue Aug 10, 2023
…iterals (only in editions) (#13471)

This addresses #13440.

@mkruskal-google, I saw you are assigned the above issue. I hope it's okay that I took a stab at it.

Closes #13471

COPYBARA_INTEGRATE_REVIEW=#13471 from jhump:jh/reserved-names-support-identifiers a23f18e
FUTURE_COPYBARA_INTEGRATE_REVIEW=#13471 from jhump:jh/reserved-names-support-identifiers a23f18e
PiperOrigin-RevId: 555559034
copybara-service bot pushed a commit that referenced this issue Aug 10, 2023
…iterals (only in editions) (#13471)

This addresses #13440.

@mkruskal-google, I saw you are assigned the above issue. I hope it's okay that I took a stab at it.

Closes #13471

COPYBARA_INTEGRATE_REVIEW=#13471 from jhump:jh/reserved-names-support-identifiers a23f18e
FUTURE_COPYBARA_INTEGRATE_REVIEW=#13471 from jhump:jh/reserved-names-support-identifiers a23f18e
PiperOrigin-RevId: 555559034
copybara-service bot pushed a commit that referenced this issue Aug 10, 2023
…iterals (only in editions) (#13471)

This addresses #13440.

@mkruskal-google, I saw you are assigned the above issue. I hope it's okay that I took a stab at it.

Closes #13471

COPYBARA_INTEGRATE_REVIEW=#13471 from jhump:jh/reserved-names-support-identifiers a23f18e
PiperOrigin-RevId: 555610263
Copy link

github-actions bot commented Nov 7, 2023

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Nov 7, 2023
@mkruskal-google
Copy link
Member

This has been bundled into edition 2023, where we will enforce this new grammar and use prototiller to migrate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Denotes the issue/PR has not seen activity in the last 90 days. protoc
Projects
None yet
Development

No branches or pull requests

5 participants