feat(parse): enforce edition 2024 local visibility across files - #2401
Open
morgan-coded wants to merge 1 commit into
Open
feat(parse): enforce edition 2024 local visibility across files#2401morgan-coded wants to merge 1 commit into
morgan-coded wants to merge 1 commit into
Conversation
Symbol visibility is parsed and preserved since protobufjs#2415, but nothing rejects a reference to a local symbol from another file. Field#resolve now checks the resolved type or enum and throws when it is local to a different file. References without a known file name on either side (programmatic construction, fromJSON, bundled common types) cannot be proven to cross files and are allowed. Refs protobufjs#2400
morgan-coded
force-pushed
the
edition2024-visibility-2400
branch
from
August 27, 2026 02:31
e2452d2 to
6cabb00
Compare
Contributor
Author
|
Rebased this onto master and it shrank a lot. #2415 landed the parsing and preservation since I opened this, so the parse.js half is gone and what's left is just the resolution check: 21 lines in
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2400.
Rebased onto master and reduced: #2415 landed the parsing and preservation half, so this is now only the resolution check, 21 lines in
src/field.jsplus tests.After a successful lookup,
Field#resolverejects a resolved type or enum that islocalto a different file:If either side has no filename, which covers programmatic construction,
fromJSON, and bundled common types, the reference cannot be proven to cross files and is allowed.Tests sit in
tests/parse_editions.jsbeside the existing export/local parsing cases: same-file references still resolve, cross-filelocalthrows,exportand unmodified symbols are unaffected, and pre-2024 editions plus proto2/proto3 behave exactly as before. That file goes 29 to 39 tests; the full suite 3201 to 3211, all green, with lint and type checks clean.Two things left out on purpose, in case you would rather they were in:
Method#resolveresolves them throughlookupTypeand methods do carry a filename, so the same check would be live there. Sharing it across modules felt like a bigger structural decision than this change should make on its own; it is about 10 lines if you want it here.tryHandleExtensioninroot.jsis deliberately untouched. It is a boolean try inside the deferred-retry loop, so throwing there would break deferred extensions and make errors depend on file load order. Extension fields are still checked through the sister field's ownresolve.The earlier note about
visibilitynot survivingtoJSON/fromJSONno longer applies, since #2415 carries it through the descriptor surface.