Forbid accessing block-scoped variables on globalThis #30510
Merged
Conversation
This file contains 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
It's just an error; you still get the type of the property.
Also change Array, Function, String, et al from `const` to `var` so that they remain accessible via `globalThis.String`.
Note especially the change in redefineArray, which is now allowed as long as you provide a type that is assignable to ArrayConstructor.
sandersn
changed the title
Unlike forbidding them, this removes the properties entirely. Unfortunately, this means that accessing these properties is only an error with noImplicitAny, and that error is quite confusing. Let's discuss our options. I see 3: 1. Forbid access of block-scoped vars as properties (in all flag settings), but leave them on the type. Simple to implement. 2. Remove block-scoped vars from the globalThis type. Has the bad error/flag behaviour described above, but simple to implement. 3. Remove block-scoped vars from the globalThis type. Also, forbid accessing them by executing another resolveName lookup for failed property accesses on globalThisSymbol. If the second lookup returns a blockscoped var, issue an error instead of falling back to the index signature. This seems too complex to me.
So that value-space references have as clear an error as type-space references.
6 tasks
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.
Fixes #30477
Forbid accessing blockscoped variables on
globalThis. Also change Array, Function, String, et al fromconsttovarso that they remain accessible viaglobalThis. Note that this changes some tests, especially redefineArray, which is now allowed as long as you provide a type that is assignable to ArrayConstructor.I did not change
name, since (1) it's not a global in node (2) it would still be nice to prevent mistaken references to it. In the comparisons below, however, I assume that the other previously-const declarations in es5.d.ts are changed to var.There are four ways to proceed.
Do nothing.
I don't think we should do this. It's bad that your code will work when downlevelling to es5 but then break when downlevelling to es6 and above.
Code: simplest.
Semantics: least correct.
Error on property usage.
Code: two errors in a surprising place: when the property is found, but when the container's symbol is
globalThis.Semantics: Correct errors for value-space usages, but still incorrect for type-space usages.
Remove block-scoped symbols when creating
typeof globalThis.Code: one filter in
resolveAnonymousTypeMembers.Semantics: Technically correct. But globalThis' special noImplicitAny rules mean that the properties are still available. With noImplicitAny, the errors are "property 'x' is implicitly any", not "property 'x' is not found" or "property 'x' is a block-scope variable that is not available on globalThis". Without noImplicitAny, the type is just any.
Both: remove block-scoped symbols, but then provide a special error when attempting to reference properties.
Code: two errors in a non-surprising place, plus the filter when resolving type members.
Semantics: Correct, with "property 'x' is not found" whether noImplicitAny is on or off.
Recommendation
I recommend the fourth option, somewhat reluctantly. That's what this PR implements. The first option, do nothing, is too incorrect. The second option's implementation is about as bad as the fourth's because the code is confusing. The third option's code is not too bad, but the error behaviour is confusing despite being technically correct. The fourth option at least puts errors in the already-junky error-reporting path of property/element access.