Support some late-bound special property assignments #33220
Conversation
src/compiler/utilities.ts
Outdated
| return AssignmentDeclarationKind.None; | ||
| } | ||
|
|
||
| // TODO: Signed numeric literals? |
There was a problem hiding this comment.
Decide whether they're in or out and either remove the TODO or replace it with a warning that they are(n't) supported.
There was a problem hiding this comment.
They should proooobably be in, but some checks are going to get way more complicated to support them.
There was a problem hiding this comment.
replace it with a warning that they are(n't) supported.
That would be a strict degradation from today's behavior where A[+0] is simply an access on A's 0 - no we can't warn here; but I do think recognizing more than this is just an incremental enhancement.
There was a problem hiding this comment.
For reference to a similar area where incremental enhancements could be made, if you write
const x = {
[+(0)]: 12
}
const n = x[1]n is not an error, but if you write
const x = {
[+0]: 12
}
const n = x[1]it is, the only difference being a runtime non-affecting set of parenthesis.
There was a problem hiding this comment.
yeah, a user warning isn't necessary, but a jsdoc warning instead of a TODO in the source is good enough.
weswigham
force-pushed
the
js-this-latebound-property-assignment
branch
from
3c652cf to
37674e0
Compare
src/compiler/utilities.ts
Outdated
| return AssignmentDeclarationKind.None; | ||
| } | ||
|
|
||
| // TODO: Signed numeric literals? |
There was a problem hiding this comment.
yeah, a user warning isn't necessary, but a jsdoc warning instead of a TODO in the source is good enough.
|
|
||
| function addLateBoundAssignmentDeclarationToSymbol(node: BinaryExpression | DynamicNamedDeclaration, symbol: Symbol | undefined) { | ||
| if (symbol) { | ||
| const members = symbol.assignmentDeclarationMembers || (symbol.assignmentDeclarationMembers = createMap()); |
There was a problem hiding this comment.
changing the shape of Symbol makes me think we should run the perf tests as a precaution.
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at a2e3797. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 489a01b. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..33220
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
src/compiler/utilities.ts
Outdated
| return idText(name); | ||
| } | ||
| if (isNumericLiteral(name)) { | ||
| return "" + (+name.text); |
There was a problem hiding this comment.
🤔 I’m sure there’s a good reason for this
There was a problem hiding this comment.
Assume name.text is 1e1 as a string. Convert to number, get out 10, convert back to string, have the string "10". This matters for indexing, since only the canonical representation is the stringy key name.
There was a problem hiding this comment.
@rbuckton points out that the misleadingly named .text has already been canonicalized in the scanner, so this is unneeded 😛
| @@ -814,7 +814,7 @@ namespace ts { | |||
|
|
|||
| export type PropertyName = Identifier | StringLiteral | NumericLiteral | ComputedPropertyName; | |||
|
|
|||
| export type DeclarationName = Identifier | StringLiteralLike | NumericLiteral | ComputedPropertyName | BindingPattern; | |||
| export type DeclarationName = Identifier | StringLiteralLike | NumericLiteral | ComputedPropertyName | ElementAccessExpression | BindingPattern; | |||
There was a problem hiding this comment.
navigationBar uses DeclarationName and getNameOfDeclaration. Is it affected by this change?
There was a problem hiding this comment.
Hmmm, it uses nodeText on the name, which probably works fine for ComputedPropertyDeclarations, but probably falls a little short for ElementAccessExpressions. Can you point me to a test for computed property name tests for navigationBar? I'll (speculatively) build in the fix, but I'd like to add a test.
There was a problem hiding this comment.
tests\cases\fourslash\navigationBarItemsSymbols1.ts
There was a problem hiding this comment.
There was a problem hiding this comment.
So, turns out js assignment members aren't syntactically children, so they can't be in the nav bar (which only looks at syntax members). Additionally, the nav bar excludes dynamically named members, so on two fronts these shouldn't appear in the nav bar. I've added a test showing the behavior~
…ve fix to navigation bar, merge check and type for elem/property accesses
Fixes #33129
In some of the projects I was looking over for declarations for js, our inability to recognize symbolically named members in JS was one of the big holes I still saw that blocked well-written JS from making good declarations.
This means you can now do, in TS:
and in JS:
and the
unique symboltyped implied members are well-typed.Currently in TS, the declaration emit just silently strips away the late-bound function members. This is certainly worth revisiting and improving, I think, but would necessitate rewriting the function declaration into a
constwith a type annotation, rather than a function/namespace merge (since namespace members cannot have late bound names, syntactically...).