Skip to content

Avoid incorrectly reusing assertion nodes from property assignments - #60576

Merged
Jake Bailey (jakebailey) merged 6 commits into
microsoft:mainfrom
Andarist:dont-reuse-assertion-nodes-with-different-types
Dec 4, 2024
Merged

Avoid incorrectly reusing assertion nodes from property assignments#60576
Jake Bailey (jakebailey) merged 6 commits into
microsoft:mainfrom
Andarist:dont-reuse-assertion-nodes-with-different-types

Conversation

@Andarist

@AndaristMateusz Burzyński (Andarist) commented Nov 24, 2024

Copy link
Copy Markdown
Contributor

fixes#60573 (a regression from #59282 )

@typescript-botTypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 24, 2024
@typescript-bot

Copy link
Copy Markdown
Contributor

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Comment threadsrc/compiler/expressionToTypeNode.ts Outdated
@typescript-botTypeScript 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 Dec 4, 2024
@jakebailey

Copy link
Copy Markdown
Member

If this fixes #60573, we should definitely add a dts test that shows that things are fixed.

Comment on lines +713 to +714
if (assertionNode && !isConstTypeReference(assertionNode) && resolver.canReuseTypeNodeAnnotation(context, node, assertionNode, symbol)) {
result = serializeExistingTypeNode(assertionNode, context);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was largely unsure about this given you ask a question about node then use that to work with assertionNode, but this is effectively the same check as:

functiontypeFromPropertyAssignment(node: PropertyAssignment|ShorthandPropertyAssignment,symbol: Symbol,context: SyntacticTypeNodeBuilderContext){consttypeAnnotation=getEffectiveTypeAnnotationNode(node);letresult;if(typeAnnotation&&resolver.canReuseTypeNodeAnnotation(context,node,typeAnnotation,symbol)){result=serializeExistingTypeNode(typeAnnotation,context);}

So I definitely like this change more than the initial version.

@jakebailey

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) test it

@typescript-bot

TypeScript Bot (typescript-bot) commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

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

CommandStatusResults
test top400✅ Started✅ Results
user test this✅ Started✅ Results
run dt✅ Started✅ Results
perf test this faster✅ Started👀 Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the user tests with tsc comparing main and refs/pull/60576/merge:

Everything looks good!

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Compiler-Unions - node (v18.15.0, x64)
Errors3434~~~p=1.000 n=6
Symbols62,36362,363~~~p=1.000 n=6
Types50,39550,395~~~p=1.000 n=6
Memory used194,233k (± 0.95%)194,267k (± 0.97%)~193,042k196,713kp=0.748 n=6
Parse Time1.30s (± 1.15%)1.31s (± 0.68%)~1.30s1.32sp=0.456 n=6
Bind Time0.72s0.72s~~~p=1.000 n=6
Check Time9.78s (± 0.36%)9.79s (± 0.46%)~9.72s9.85sp=0.872 n=6
Emit Time2.73s (± 0.38%)2.73s (± 0.65%)~2.71s2.76sp=0.805 n=6
Total Time14.53s (± 0.29%)14.54s (± 0.41%)~14.48s14.64sp=0.936 n=6
angular-1 - node (v18.15.0, x64)
Errors3737~~~p=1.000 n=6
Symbols947,936947,936~~~p=1.000 n=6
Types410,955410,955~~~p=1.000 n=6
Memory used1,226,007k (± 0.00%)1,226,016k (± 0.00%)~1,225,935k1,226,080kp=0.575 n=6
Parse Time6.63s (± 0.55%)6.64s (± 0.52%)~6.58s6.68sp=0.871 n=6
Bind Time1.89s (± 0.22%)1.89s (± 0.58%)~1.88s1.91sp=1.000 n=6
Check Time32.00s (± 0.34%)31.96s (± 0.33%)~31.87s32.15sp=0.517 n=6
Emit Time15.22s (± 0.37%)15.18s (± 0.31%)~15.10s15.22sp=0.167 n=6
Total Time55.74s (± 0.25%)55.66s (± 0.25%)~55.48s55.86sp=0.471 n=6
mui-docs - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols2,501,8232,501,823~~~p=1.000 n=6
Types909,645909,645~~~p=1.000 n=6
Memory used2,319,287k (± 0.00%)2,319,273k (± 0.00%)~2,319,207k2,319,317kp=0.378 n=6
Parse Time9.30s (± 0.18%)9.30s (± 0.23%)~9.28s9.33sp=0.514 n=6
Bind Time2.16s (± 0.83%)2.15s (± 0.97%)~2.12s2.18sp=0.452 n=6
Check Time74.95s (± 0.23%)74.98s (± 0.37%)~74.57s75.42sp=0.936 n=6
Emit Time0.28s0.28s (± 3.53%)~0.27s0.29sp=0.652 n=6
Total Time86.69s (± 0.21%)86.72s (± 0.32%)~86.31s87.17sp=0.686 n=6
self-build-src - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols1,225,2871,225,287~~~p=1.000 n=6
Types266,555266,555~~~p=1.000 n=6
Memory used2,354,218k (± 0.03%)2,354,129k (± 0.02%)~2,353,675k2,354,651kp=0.810 n=6
Parse Time5.23s (± 0.59%)5.25s (± 0.44%)~5.23s5.29sp=0.683 n=6
Bind Time1.77s (± 1.57%)1.78s (± 1.56%)~1.76s1.83sp=0.746 n=6
Check Time35.19s (± 0.65%)35.19s (± 0.44%)~35.07s35.49sp=0.873 n=6
Emit Time2.97s (± 1.21%)3.02s (± 3.00%)~2.95s3.19sp=0.630 n=6
Total Time45.17s (± 0.53%)45.25s (± 0.55%)~45.10s45.74sp=0.298 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols1,225,2871,225,287~~~p=1.000 n=6
Types266,555266,555~~~p=1.000 n=6
Memory used2,907,885k (±12.87%)2,786,589k (±14.24%)~2,423,271k3,150,329kp=0.298 n=6
Parse Time6.97s (± 1.41%)6.91s (± 2.01%)~6.77s7.08sp=0.575 n=6
Bind Time2.16s (± 1.67%)2.16s (± 1.82%)~2.10s2.20sp=1.000 n=6
Check Time42.73s (± 0.76%)42.80s (± 0.52%)~42.60s43.21sp=1.000 n=6
Emit Time3.48s (± 2.40%)3.57s (± 5.02%)~3.40s3.83sp=0.575 n=6
Total Time55.37s (± 0.76%)55.43s (± 0.53%)~55.01s55.91sp=0.810 n=6
self-compiler - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols262,259262,259~~~p=1.000 n=6
Types106,616106,616~~~p=1.000 n=6
Memory used439,871k (± 0.01%)439,875k (± 0.01%)~439,813k439,921kp=0.748 n=6
Parse Time3.54s (± 0.52%)3.52s (± 0.69%)~3.50s3.56sp=0.164 n=6
Bind Time1.31s (± 0.89%)1.31s (± 0.89%)~1.30s1.33sp=1.000 n=6
Check Time18.96s (± 0.50%)18.87s (± 0.36%)~18.79s18.98sp=0.093 n=6
Emit Time1.52s (± 1.56%)1.54s (± 0.89%)~1.52s1.56sp=0.191 n=6
Total Time25.33s (± 0.45%)25.24s (± 0.25%)~25.16s25.34sp=0.173 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors7070~~~p=1.000 n=6
Symbols226,062226,062~~~p=1.000 n=6
Types94,48894,488~~~p=1.000 n=6
Memory used371,772k (± 0.07%)371,606k (± 0.05%)~371,472k371,958kp=0.173 n=6
Parse Time2.92s (± 1.14%)2.88s (± 1.45%)~2.82s2.94sp=0.199 n=6
Bind Time1.58s (± 0.74%)1.59s (± 1.01%)~1.57s1.61sp=0.285 n=6
Check Time16.53s (± 0.41%)16.48s (± 0.16%)~16.44s16.50sp=0.327 n=6
Emit Time0.00s (±244.70%)0.00s~~~p=0.405 n=6
Total Time21.03s (± 0.32%)20.96s (± 0.28%)~20.89s21.04sp=0.108 n=6
vscode - node (v18.15.0, x64)
Errors33~~~p=1.000 n=6
Symbols3,197,6633,197,663~~~p=1.000 n=6
Types1,099,1621,099,162~~~p=1.000 n=6
Memory used3,271,365k (± 0.01%)3,271,502k (± 0.01%)~3,271,145k3,272,144kp=0.173 n=6
Parse Time14.08s (± 0.49%)14.11s (± 0.53%)~14.02s14.23sp=0.688 n=6
Bind Time4.54s (± 0.37%)4.52s (± 0.28%)-0.02s (- 0.44%)4.51s4.54sp=0.048 n=6
Check Time88.17s (± 1.86%)87.70s (± 1.90%)~86.28s90.15sp=0.689 n=6
Emit Time28.27s (± 2.92%)27.60s (± 8.67%)~22.81s29.12sp=1.000 n=6
Total Time135.07s (± 0.95%)133.93s (± 1.01%)~131.70s135.77sp=0.173 n=6
webpack - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols288,751288,751~~~p=1.000 n=6
Types117,157117,157~~~p=1.000 n=6
Memory used441,035k (± 0.03%)441,029k (± 0.03%)~440,839k441,224kp=1.000 n=6
Parse Time4.06s (± 0.83%)4.08s (± 0.78%)~4.03s4.12sp=0.376 n=6
Bind Time1.76s (± 2.28%)1.77s (± 1.82%)~1.74s1.82sp=0.687 n=6
Check Time18.90s (± 0.75%)18.92s (± 0.55%)~18.72s19.00sp=1.000 n=6
Emit Time0.00s0.00s~~~p=1.000 n=6
Total Time24.72s (± 0.64%)24.77s (± 0.48%)~24.57s24.91sp=0.810 n=6
xstate-main - node (v18.15.0, x64)
Errors55~~~p=1.000 n=6
Symbols552,390552,390~~~p=1.000 n=6
Types185,096185,096~~~p=1.000 n=6
Memory used492,463k (± 0.02%)492,518k (± 0.01%)~492,450k492,614kp=0.936 n=6
Parse Time3.41s (± 1.01%)3.41s (± 0.86%)~3.36s3.45sp=0.935 n=6
Bind Time1.18s (± 0.69%)1.17s (± 1.03%)~1.15s1.18sp=1.000 n=6
Check Time19.55s (± 1.11%)19.66s (± 2.43%)~19.34s20.62sp=0.810 n=6
Emit Time0.00s0.00s~~~p=1.000 n=6
Total Time24.14s (± 0.88%)24.25s (± 2.00%)~23.95s25.22sp=1.000 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)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@jakebailey

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

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

CommandStatusResults
pack this✅ Started

Comment on lines +31 to +34
//// [declarationAssertionNodeNotReusedWhenTypeNotEquivalent1.d.ts]
export declare const unwrapped: {
prop1: "hello";
};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Jake Bailey (@jakebailey) here is the requested dts test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, this looks solid now. It'd probably be relevant to ask what output this limits under isolatedDeclarations, but... 🤷‍♂️ it kinda is what it is.

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the top 400 repos with tsc comparing main and refs/pull/60576/merge:

Everything looks good!

@jakebailey

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

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

CommandStatusResults
pack this✅ Started✅ Results

@typescript-bot

TypeScript Bot (typescript-bot) commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@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/164278/artifacts?artifactName=tgz&fileId=579FD5D10F806287DEC373A9533F60476A5F5163C508B9047F1A00060D2FA2A002&fileName=/typescript-5.8.0-insiders.20241204.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/pr-build@5.8.0-pr-60576-12".;

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the user tests with tsc comparing main and refs/pull/60576/merge:

Everything looks good!

@jakebailey
Jake Bailey (jakebailey) merged commit 676d329 into microsoft:mainDec 4, 2024
@jakebailey

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) cherry-pick this to release-5.7

@typescript-bot

TypeScript Bot (typescript-bot) commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

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

CommandStatusResults
cherry-pick this to release-5.7✅ Started✅ Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey, Jake Bailey (@jakebailey)! I've created #60679 for you.

Daniel Rosenwasser (DanielRosenwasser) pushed a commit that referenced this pull request Dec 11, 2024
…e-5.7 (#60679)
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 15, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Milestone BugPRs that fix a bug with a specific milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Type assertion regression in TypeScript 5.7

6 participants

@Andarist@typescript-bot@jakebailey@weswigham@dragomirtitian@sandersn