protoc: validate reserved names are identifiers #10586
Conversation
|
ping @mkruskal-google, mind taking a quick pass? |
There was a problem hiding this comment.
Hey Joshua, sorry for the delay and thanks for the PR! This LGTM, but would you mind filling out a change proposal? https://docs.google.com/document/d/1RPgh6engDi319RhZ9cKmLQYLgyF7xscBHpqfHTpkQ9Q/edit#heading=h.f4bilzmezze2
|
@mkruskal-google, I've filled out the proposal for a small language change: cc @fowles It's a very minor change -- especially since it requires no changes in the descriptor, which means it has no impact on anything but the parser. But do you also want a proposal for this PR? That seems a little heavy-weight -- what this PR is doing is trivial. Is the issue that you expect many existing files in google3 to have invalid reserved names, so this change need to a more elaborate flagging/migration process? If so, then it sounds like maybe we should just close this PR and possibly add the check in a future "edition" (where there would be a standard way of controlling this logic via message option). |
|
Thanks Joshua! Please add protobuf-team@google.com as editors too. I think it would be reasonable to fold this PR into your proposal. The most important purpose of these change proposals is just recording the motivation for and discussion around new changes. However, this PR is also a breaking change so we need to be careful about rollout. I suspect there will be some pre-existing mistakes that this will now break. For the google3-side that shouldn't be too hard to clean up, and I'm more worried about how we release this to OSS |
@mkruskal-google, FWIW, the grammars on the protobuf developer site already state that this must be an identifier. So I think this is just fixing a bug -- the compiler should have never accepted the bad input in the first place. If there are users that have bad sources, they will have to fix their sources before they can upgrade to the new version of Is there something about this particular issue that makes that approach unreasonable? I'll update the proposal doc to describe this validation step. But I'm not sure what I should say about the migration: as stated above, for open-source land, I think it's reasonable to simply release this as a bug-fix and require users to fix any invalid sources before they upgrade. But I think the google3/internal migration would be something I can't really write about. Can you just change the compiler, let Tap (or whatever current CI system is called) turn red for some projects, and expect teams to fix their sources to get green again? Or do you use a global presubmit queue to identify all of the problematic sources first, before integrating this change into google3? |
|
No there's nothing unique about this issue, and I agree that this could be viewed as just a bug fix. But it is a breaking bug fix, so we might want to be careful about how it's released. I'm not sure what the best procedure is and need to discuss it with the team. As far as google3 goes, I don't think you need to worry about it at all. I can handle fixing any downstream breakages when we import this. I expect it to be fairly minimal and not require that much work |
|
To unblock this PR, what do you think about downgrading this error to a warning? That way it's not a breaking change and is just a harmless new validation check. Once we meet to decide on your tier 1 proposal we can then just flip it to an error (assuming we decide to) |
@mkruskal-google, done. |
…ved-names protoc: validate reserved names are identifiers
Fixes #6335 -- just the validation of strings part, not the ambiguity issues (which likely calls for a separate issue since it really requires a minor language change).