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.
Eliminate well known symbols as a concept in the checker and rely on unique symbols #42543
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
Eliminate well known symbols as a concept in the checker and rely on unique symbols #42543
Changes from all commits
31dc466
a3fc74e
bd49b81
4c808c4
ebac382
b954507
e99c05a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll call this out since it's a new bit compared to the older PR. This bit and the following modify our symbol container lookup to prefer, eg,
Symbol.iterator
toSymbolConstructor.iterator
when doing printback for where the specifiedSymbolFlags
say the first is allowable. Now, this isn't specific to symbols orSymbol
in any way - other dynamic names from namespace-ish things should work similarly, too! SoArrayConstructor
,Int8ArrayConsturctor
.... anything with the split namespace pattern the DOM uses a lot.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is pretty much single-handedly responsible for the .js emit changes in this PR.
Symbol.iterator
and it's ilk aren't "simple copyable expressions" since, technically, accessing it could produce side effects (like any other property access). Now... at some point, we addedIdentifier
s toisSimpleCopiableExpression
's domain (which isn't technically safe, since the identifier may get reassigned between the copied references), so maybe property accesses which only consist ofIdentifier
s wouldn't be terribly unreasonable, either. 🤷There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rbuckton is probably the best judge of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact! I didn't actually need to make any of these lib changes to make the PR work, what with the compat code in place! But I've updated them anyway, so they serve as examples of what
unique symbol
s really are to anyone reading the definitions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm - and that won't conflict with a separate "polyfill" definition right? In other words, if someone separately declares it as
and these merge, they won't end up conflicting, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope - and there's already a test to that effect on the diff.