Skip to content

Fix captured shorthand properties in ES2015 loops #59285

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

Merged
merged 6 commits into from
Jul 16, 2024

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jul 15, 2024

Fixes #59262

Two problems:

isInExpressionContext (and by extension isExpressionNode) didn't consider the case of "name only" object prop shorthand. This meant we weren't even walking into the shorthand in this example. Fixing this caused knock-on effects in some baselines, but I'm not sure any are actually a problem. They are ☹️

Then, we were using getSymbolAtLocation to get the node for the shorthand prop, but that ended up returning the wrong symbol; we want the one for the "value side" of the prop. Switching to getResolvedSymbol (same as the other caller of checkIdentifierCalculateNodeCheckFlags) seems to fix that.

Not 100% certain if this makes perf bad, though. I assume not given we already had to call the same symbol stuff via getSymbolAtLocation.

@jakebailey jakebailey requested a review from weswigham July 15, 2024 20:30
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 15, 2024
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not particularly worried about the perf - I think I only even used getSymbolAtLocation over getResolvedSymbol because a previous WIP version passed in non-identifier nodes (or I was explicitly looking for the ignoreErrors flag because at the time of it being very WIP, I needed it to cut down on the error diff).

The isExpressionNode change is the one that's a bit more iffy, since it'll affect other callsites (like the type baselines), but if that ends up having weird knock-on effects we can't predict, we can always undo that part and put an exception into the condition in checkSingleIdentifier specifically (since that's the place we definitely need it to behave like this). But @rbuckton might have more opinions on that, and weather the { name } in a shorthand property should truly count as an expression or not.

@jakebailey jakebailey requested a review from rbuckton July 15, 2024 20:50
@jakebailey
Copy link
Member Author

@typescript-bot test it
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 15, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
test top400 ✅ Started ✅ Results
user test this ✅ Started ✅ Results
run dt ✅ Started ✅ Results
perf test this faster ✅ Started 👀 Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 15, 2024

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/162755/artifacts?artifactName=tgz&fileId=B34501455CD47CD8FC63C73EE5A64F096A0B1DC861328D8C5D76588BDB6C4B4202&fileName=/typescript-5.6.0-insiders.20240715.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user tests with tsc comparing main and refs/pull/59285/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-Unions - node (v18.15.0, x64)
Errors 30 30 ~ ~ ~ p=1.000 n=6
Symbols 62,153 62,153 ~ ~ ~ p=1.000 n=6
Types 50,242 50,242 ~ ~ ~ p=1.000 n=6
Memory used 192,779k (± 0.79%) 192,251k (± 0.11%) ~ 192,091k 192,520k p=0.936 n=6
Parse Time 1.95s (± 0.71%) 1.95s (± 0.99%) ~ 1.92s 1.97s p=0.459 n=6
Bind Time 1.06s (± 1.25%) 1.05s (± 1.11%) ~ 1.04s 1.07s p=0.216 n=6
Check Time 13.82s (± 0.38%) 13.83s (± 0.57%) ~ 13.75s 13.94s p=1.000 n=6
Emit Time 4.01s (± 0.49%) 4.04s (± 0.79%) ~ 4.00s 4.08s p=0.077 n=6
Total Time 20.83s (± 0.22%) 20.88s (± 0.41%) ~ 20.77s 21.01s p=0.374 n=6
angular-1 - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 944,120 944,120 ~ ~ ~ p=1.000 n=6
Types 407,047 407,047 ~ ~ ~ p=1.000 n=6
Memory used 1,218,367k (± 0.00%) 1,218,352k (± 0.00%) ~ 1,218,313k 1,218,385k p=0.630 n=6
Parse Time 8.03s (± 0.57%) 7.99s (± 0.54%) ~ 7.94s 8.06s p=0.295 n=6
Bind Time 2.23s (± 0.66%) 2.22s (± 0.46%) ~ 2.21s 2.24s p=0.616 n=6
Check Time 35.85s (± 0.48%) 35.91s (± 0.60%) ~ 35.62s 36.20s p=0.630 n=6
Emit Time 16.23s (± 0.60%) 16.18s (± 0.32%) ~ 16.10s 16.25s p=0.747 n=6
Total Time 62.34s (± 0.38%) 62.30s (± 0.33%) ~ 62.00s 62.56s p=0.873 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,249,830 2,249,830 ~ ~ ~ p=1.000 n=6
Types 937,437 937,437 ~ ~ ~ p=1.000 n=6
Memory used 2,165,082k (± 0.00%) 2,164,986k (± 0.01%) ~ 2,164,783k 2,165,189k p=0.230 n=6
Parse Time 9.66s (± 0.28%) 9.67s (± 0.35%) ~ 9.61s 9.70s p=0.629 n=6
Bind Time 3.39s (± 0.51%) 3.37s (± 0.74%) ~ 3.34s 3.41s p=0.192 n=6
Check Time 103.73s (± 0.48%) 103.88s (± 0.60%) ~ 102.77s 104.48s p=0.423 n=6
Emit Time 0.19s 0.20s (± 4.47%) 🔻+0.01s (+ 5.26%) 0.19s 0.21s p=0.028 n=6
Total Time 116.97s (± 0.44%) 117.11s (± 0.53%) ~ 116.02s 117.73s p=0.748 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,226,065 1,226,065 ~ ~ ~ p=1.000 n=6
Types 261,603 261,603 ~ ~ ~ p=1.000 n=6
Memory used 2,338,806k (± 0.02%) 2,339,656k (± 0.05%) ~ 2,337,885k 2,341,263k p=0.378 n=6
Parse Time 6.06s (± 0.61%) 6.07s (± 0.59%) ~ 6.01s 6.11s p=0.468 n=6
Bind Time 2.24s (± 0.56%) 2.23s (± 0.62%) ~ 2.22s 2.26s p=0.459 n=6
Check Time 39.88s (± 0.34%) 39.93s (± 0.22%) ~ 39.79s 40.04s p=0.688 n=6
Emit Time 3.83s (± 1.14%) 3.81s (± 1.72%) ~ 3.72s 3.92s p=0.470 n=6
Total Time 52.02s (± 0.28%) 52.06s (± 0.24%) ~ 51.90s 52.23s p=0.520 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,226,065 1,226,065 ~ ~ ~ p=1.000 n=6
Types 261,603 261,603 ~ ~ ~ p=1.000 n=6
Memory used 2,412,837k (± 0.01%) 2,413,309k (± 0.02%) ~ 2,412,773k 2,414,029k p=0.066 n=6
Parse Time 6.33s (± 0.92%) 6.34s (± 0.64%) ~ 6.28s 6.39s p=0.810 n=6
Bind Time 2.03s (± 1.01%) 2.04s (± 0.90%) ~ 2.01s 2.06s p=0.331 n=6
Check Time 40.61s (± 0.24%) 40.62s (± 0.34%) ~ 40.45s 40.78s p=0.810 n=6
Emit Time 3.90s (± 1.01%) 3.88s (± 1.01%) ~ 3.82s 3.92s p=0.378 n=6
Total Time 52.87s (± 0.24%) 52.87s (± 0.27%) ~ 52.68s 53.06s p=0.936 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 258,309 258,309 ~ ~ ~ p=1.000 n=6
Types 104,794 104,794 ~ ~ ~ p=1.000 n=6
Memory used 427,140k (± 0.03%) 427,066k (± 0.01%) ~ 427,015k 427,123k p=0.689 n=6
Parse Time 4.10s (± 0.28%) 4.11s (± 0.57%) ~ 4.08s 4.14s p=0.934 n=6
Bind Time 1.63s (± 1.14%) 1.62s (± 1.21%) ~ 1.59s 1.64s p=0.366 n=6
Check Time 22.08s (± 0.60%) 22.14s (± 0.62%) ~ 21.99s 22.30s p=0.575 n=6
Emit Time 2.03s (± 0.86%) 2.03s (± 1.41%) ~ 1.98s 2.06s p=0.805 n=6
Total Time 29.85s (± 0.42%) 29.90s (± 0.60%) ~ 29.73s 30.12s p=1.000 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 35 35 ~ ~ ~ p=1.000 n=6
Symbols 224,565 224,565 ~ ~ ~ p=1.000 n=6
Types 93,734 93,734 ~ ~ ~ p=1.000 n=6
Memory used 369,527k (± 0.02%) 369,548k (± 0.02%) ~ 369,443k 369,646k p=0.748 n=6
Parse Time 3.42s (± 0.39%) 3.42s (± 0.71%) ~ 3.38s 3.45s p=0.807 n=6
Bind Time 1.93s (± 0.53%) 1.93s (± 0.85%) ~ 1.91s 1.96s p=0.802 n=6
Check Time 19.12s (± 0.29%) 19.14s (± 0.28%) ~ 19.05s 19.18s p=0.748 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 24.48s (± 0.24%) 24.49s (± 0.21%) ~ 24.41s 24.57s p=0.746 n=6
vscode - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,891,299 2,891,299 ~ ~ ~ p=1.000 n=6
Types 978,316 978,316 ~ ~ ~ p=1.000 n=6
Memory used 3,055,242k (± 0.00%) 3,055,185k (± 0.00%) ~ 3,055,008k 3,055,268k p=0.471 n=6
Parse Time 16.98s (± 0.46%) 16.95s (± 0.22%) ~ 16.91s 17.00s p=0.574 n=6
Bind Time 5.12s (± 0.61%) 5.13s (± 0.20%) ~ 5.11s 5.14s p=0.250 n=6
Check Time 89.70s (± 0.39%) 89.57s (± 0.42%) ~ 88.96s 89.95s p=0.423 n=6
Emit Time 29.30s (± 0.88%) 29.19s (± 0.29%) ~ 29.06s 29.29s p=0.378 n=6
Total Time 141.09s (± 0.37%) 140.84s (± 0.30%) ~ 140.22s 141.25s p=0.575 n=6
webpack - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 267,280 267,280 ~ ~ ~ p=1.000 n=6
Types 108,834 108,834 ~ ~ ~ p=1.000 n=6
Memory used 411,757k (± 0.02%) 411,768k (± 0.03%) ~ 411,637k 411,911k p=1.000 n=6
Parse Time 3.84s (± 0.37%) 3.82s (± 0.46%) ~ 3.80s 3.85s p=0.162 n=6
Bind Time 1.69s (± 0.87%) 1.70s (± 0.81%) ~ 1.68s 1.71s p=0.743 n=6
Check Time 16.71s (± 0.44%) 16.76s (± 0.48%) ~ 16.62s 16.85s p=0.296 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 22.25s (± 0.36%) 22.28s (± 0.28%) ~ 22.18s 22.36s p=0.468 n=6
xstate-main - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 510,479 510,479 ~ ~ ~ p=1.000 n=6
Types 161,598 161,598 ~ ~ ~ p=1.000 n=6
Memory used 448,302k (± 0.03%) 448,247k (± 0.06%) ~ 447,674k 448,439k p=0.471 n=6
Parse Time 2.62s (± 0.67%) 2.63s (± 0.57%) ~ 2.61s 2.65s p=0.284 n=6
Bind Time 0.98s (± 1.06%) 0.98s (± 0.42%) ~ 0.97s 0.98s p=0.924 n=6
Check Time 14.41s (± 0.34%) 14.41s (± 0.44%) ~ 14.36s 14.50s p=1.000 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 18.01s (± 0.28%) 18.03s (± 0.31%) ~ 17.98s 18.12s p=0.870 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Compiler-Unions - node (v18.15.0, x64)
  • angular-1 - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • ts-pre-modules - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate-main - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@jakebailey
Copy link
Member Author

Preemptively sending this just so we don't forget:

@typescript-bot cherry-pick this to release-5.5

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 15, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
cherry-pick this to release-5.5 ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

Hey, @jakebailey! I've created #59288 for you.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top 400 repos with tsc comparing main and refs/pull/59285/merge:

Everything looks good!

DanielRosenwasser pushed a commit that referenced this pull request Jul 16, 2024
@jakebailey jakebailey added this to the TypeScript 5.5.4 milestone Jul 16, 2024
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 16, 2024
Copy link
Contributor

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are quite a few types baseline changes that seem like an unintentional consequence of considering the name of a shorthand assignment as an expression.

function isExpressionNodeOrShorthandPropertyAssignmentName(node: Identifier) {
// TODO(jakebailey): Just use isExpressionNode once that considers these identifiers to be expressions.
return isExpressionNode(node)
|| isShorthandPropertyAssignment(node.parent) && (node.parent.objectAssignmentInitializer ?? node.parent.name) === node;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the targeted fix.

jakebailey added a commit that referenced this pull request Jul 16, 2024
@@ -3550,6 +3550,7 @@ export function isInExpressionContext(node: Node): boolean {
case SyntaxKind.ExpressionWithTypeArguments:
return (parent as ExpressionWithTypeArguments).expression === node && !isPartOfTypeNode(parent);
case SyntaxKind.ShorthandPropertyAssignment:
// TODO(jakebailey): it's possible that node could be the name, too
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to follow-up on this after merging; hoping that it's straightforwad to fix getTypeAtLocation to retain its old behavior.

@jakebailey
Copy link
Member Author

@typescript-bot cherry-pick this to release-5.5

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 16, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
cherry-pick this to release-5.5 ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

Hey, @jakebailey! I've created #59307 for you.

@jakebailey jakebailey merged commit 003221b into microsoft:main Jul 16, 2024
29 checks passed
@jakebailey jakebailey deleted the fix-59262 branch July 16, 2024 20:30
DanielRosenwasser pushed a commit that referenced this pull request Jul 16, 2024
@sandersn sandersn removed this from PR Backlog Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect ES5 code emitted for closures in loops when noCheck is enabled
4 participants