Skip to content

Compare optional property flag when comparing against discriminant properties under exactOptionalPropertyTypes #61682

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Compare optional property flag when comparing against discriminant properties under exactOptionalPropertyTypes #61682

wants to merge 1 commit into from
+1,103 −1

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented May 9, 2025

fixes #61678

@Copilot Copilot AI review requested due to automatic review settings May 9, 2025 17:36
@github-project-automation github-project-automation bot moved this to Not started in PR Backlog May 9, 2025
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 9, 2025
Copilot
Copilot AI reviewed May 9, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue (#61678) in which optional property flags are compared incorrectly against discriminant properties when the compiler flag exactOptionalPropertyTypes is enabled. Key changes include updated test cases that expect errors in union discriminant assignments and a modification of the comparison logic in the checker.

Reviewed Changes

Copilot reviewed 3 out of 15 changed files in this pull request and generated 1 comment.

File Description
tests/cases/compiler/unionRelationshipCheckPasses3.ts New test case to verify union assignment errors with different optional property flags
tests/cases/compiler/unionRelationshipCheckPasses2.ts Additional test confirming error handling with union discriminants
src/compiler/checker.ts Updated property comparison logic to consider exactOptionalPropertyTypes when checking optional properties
Files not reviewed (12)
  • tests/baselines/reference/unionRelationshipCheckPasses2(exactoptionalpropertytypes=false).errors.txt: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses2(exactoptionalpropertytypes=false).symbols: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses2(exactoptionalpropertytypes=false).types: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses2(exactoptionalpropertytypes=true).errors.txt: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses2(exactoptionalpropertytypes=true).symbols: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses2(exactoptionalpropertytypes=true).types: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses3(exactoptionalpropertytypes=false).errors.txt: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses3(exactoptionalpropertytypes=false).symbols: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses3(exactoptionalpropertytypes=false).types: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses3(exactoptionalpropertytypes=true).errors.txt: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses3(exactoptionalpropertytypes=true).symbols: Language not supported
  • tests/baselines/reference/unionRelationshipCheckPasses3(exactoptionalpropertytypes=true).types: Language not supported

src/compiler/checker.ts
@@ -23998,7 +23998,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (!targetProperty) continue outer;
if (sourceProperty === targetProperty) continue;
// We compare the source property to the target in the context of a single discriminant type.
const related = propertyRelatedTo(source, target, sourceProperty, targetProperty, _ => combination[i], /*reportErrors*/ false, IntersectionState.None, /*skipOptional*/ strictNullChecks || relation === comparableRelation);
const related = propertyRelatedTo(source, target, sourceProperty, targetProperty, _ => combination[i], /*reportErrors*/ false, IntersectionState.None, /*skipOptional*/ strictNullChecks && !exactOptionalPropertyTypes || relation === comparableRelation);
Copy link
Preview

Copilot AI May 9, 2025

Choose a reason for hiding this comment

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

Consider adding parentheses to clarify the evaluation order of the skipOptional argument. For example, change it to '(strictNullChecks && !exactOptionalPropertyTypes) || relation === comparableRelation' to ensure the intended precedence and improve code readability.

Copilot uses AI. Check for mistakes.

Andarist
Andarist commented May 9, 2025
View reviewed changes
src/compiler/checker.ts
@@ -23998,7 +23998,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (!targetProperty) continue outer;
if (sourceProperty === targetProperty) continue;
// We compare the source property to the target in the context of a single discriminant type.
const related = propertyRelatedTo(source, target, sourceProperty, targetProperty, _ => combination[i], /*reportErrors*/ false, IntersectionState.None, /*skipOptional*/ strictNullChecks || relation === comparableRelation);
const related = propertyRelatedTo(source, target, sourceProperty, targetProperty, _ => combination[i], /*reportErrors*/ false, IntersectionState.None, /*skipOptional*/ strictNullChecks && !exactOptionalPropertyTypes || relation === comparableRelation);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

skipOptional got introduced in #38101 . According to that PR's description its main goal is to align with this principle:

an optional undefined is the same as an undefined field in everything except what we require be written in declarations

But that's not true under exactOptionalPropertyTypes. So the easiest fix to this issue seems to be to just pass skipOptional === false when exactOptionalPropertyTypes is enabled.

@RyanCavanaugh
Copy link
Member

@typescript-bot test it

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 9, 2025 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

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

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh
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 34 34 ~ ~ ~ p=1.000 n=6
Symbols 62,390 62,390 ~ ~ ~ p=1.000 n=6
Types 50,395 50,395 ~ ~ ~ p=1.000 n=6
Memory used 194,305k (± 0.99%) 192,970k (± 0.01%) ~ 192,947k 193,005k p=0.173 n=6
Parse Time 1.30s (± 0.79%) 1.31s (± 0.39%) ~ 1.31s 1.32s p=0.069 n=6
Bind Time 0.73s 0.73s ~ ~ ~ p=1.000 n=6
Check Time 9.75s (± 0.24%) 9.76s (± 0.14%) ~ 9.74s 9.78s p=0.677 n=6
Emit Time 2.73s (± 0.43%) 2.74s (± 0.68%) ~ 2.72s 2.77s p=0.222 n=6
Total Time 14.52s (± 0.20%) 14.54s (± 0.18%) ~ 14.51s 14.57s p=0.126 n=6
angular-1 - node (v18.15.0, x64)
Errors 56 56 ~ ~ ~ p=1.000 n=6
Symbols 949,240 949,240 ~ ~ ~ p=1.000 n=6
Types 411,065 411,065 ~ ~ ~ p=1.000 n=6
Memory used 1,225,084k (± 0.00%) 1,225,100k (± 0.00%) ~ 1,225,063k 1,225,133k p=0.575 n=6
Parse Time 6.65s (± 0.70%) 6.64s (± 0.79%) ~ 6.56s 6.68s p=0.807 n=6
Bind Time 1.88s (± 0.48%) 1.88s (± 0.29%) ~ 1.88s 1.89s p=0.341 n=6
Check Time 31.94s (± 0.28%) 31.87s (± 0.29%) ~ 31.75s 32.00s p=0.298 n=6
Emit Time 15.23s (± 0.39%) 15.24s (± 0.20%) ~ 15.19s 15.27s p=0.571 n=6
Total Time 55.70s (± 0.16%) 55.63s (± 0.22%) ~ 55.39s 55.74s p=0.423 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,439,920 2,439,920 ~ ~ ~ p=1.000 n=6
Types 872,553 872,553 ~ ~ ~ p=1.000 n=6
Memory used 2,448,218k (± 0.00%) 2,448,236k (± 0.00%) ~ 2,448,170k 2,448,307k p=0.689 n=6
Parse Time 8.76s (± 0.47%) 8.75s (± 0.31%) ~ 8.72s 8.79s p=0.872 n=6
Bind Time 2.22s (± 0.62%) 2.21s (± 0.48%) ~ 2.19s 2.22s p=0.160 n=6
Check Time 72.33s (± 0.78%) 72.64s (± 0.44%) ~ 72.08s 73.04s p=0.377 n=6
Emit Time 0.30s (± 2.52%) 0.31s (± 3.79%) ~ 0.29s 0.32s p=0.136 n=6
Total Time 83.60s (± 0.63%) 83.90s (± 0.39%) ~ 83.34s 84.35s p=0.173 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,228,052 1,228,052 ~ ~ ~ p=1.000 n=6
Types 267,262 267,262 ~ ~ ~ p=1.000 n=6
Memory used 2,360,607k (± 0.02%) 2,360,755k (± 0.03%) ~ 2,359,481k 2,361,666k p=0.689 n=6
Parse Time 5.25s (± 0.53%) 5.21s (± 1.45%) ~ 5.11s 5.30s p=0.575 n=6
Bind Time 1.80s (± 0.67%) 1.80s (± 1.11%) ~ 1.77s 1.82s p=0.934 n=6
Check Time 35.44s (± 0.42%) 35.52s (± 0.53%) ~ 35.31s 35.73s p=0.297 n=6
Emit Time 3.01s (± 2.58%) 3.06s (± 4.33%) ~ 2.94s 3.32s p=0.471 n=6
Total Time 45.50s (± 0.48%) 45.61s (± 0.46%) ~ 45.38s 45.85s p=0.471 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,228,052 1,228,052 ~ ~ ~ p=1.000 n=6
Types 267,262 267,262 ~ ~ ~ p=1.000 n=6
Memory used 3,038,092k (± 9.78%) 2,942,598k (±11.54%) ~ 2,428,969k 3,160,662k p=0.936 n=6
Parse Time 6.91s (± 1.35%) 6.94s (± 0.78%) ~ 6.89s 7.02s p=0.873 n=6
Bind Time 2.18s (± 1.63%) 2.18s (± 1.90%) ~ 2.13s 2.24s p=0.688 n=6
Check Time 42.91s (± 0.51%) 42.80s (± 1.04%) ~ 42.00s 43.22s p=1.000 n=6
Emit Time 3.46s (± 1.46%) 3.54s (± 2.21%) ~ 3.41s 3.62s p=0.093 n=6
Total Time 55.47s (± 0.48%) 55.46s (± 0.83%) ~ 54.64s 55.99s p=0.936 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 263,419 263,419 ~ ~ ~ p=1.000 n=6
Types 107,098 107,098 ~ ~ ~ p=1.000 n=6
Memory used 441,719k (± 0.01%) 441,751k (± 0.01%) ~ 441,679k 441,854k p=0.378 n=6
Parse Time 3.55s (± 0.70%) 3.55s (± 1.28%) ~ 3.51s 3.62s p=0.936 n=6
Bind Time 1.32s (± 1.27%) 1.33s (± 1.16%) ~ 1.30s 1.34s p=0.186 n=6
Check Time 19.07s (± 0.16%) 19.05s (± 0.41%) ~ 18.94s 19.15s p=0.936 n=6
Emit Time 1.53s (± 1.51%) 1.54s (± 1.14%) ~ 1.51s 1.56s p=0.808 n=6
Total Time 25.46s (± 0.22%) 25.47s (± 0.43%) ~ 25.35s 25.65s p=0.936 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 71 71 ~ ~ ~ p=1.000 n=6
Symbols 225,981 225,981 ~ ~ ~ p=1.000 n=6
Types 94,356 94,356 ~ ~ ~ p=1.000 n=6
Memory used 371,359k (± 0.05%) 371,340k (± 0.02%) ~ 371,285k 371,442k p=0.298 n=6
Parse Time 2.90s (± 0.71%) 2.86s (± 1.67%) ~ 2.81s 2.93s p=0.145 n=6
Bind Time 1.60s (± 1.75%) 1.59s (± 1.47%) ~ 1.57s 1.64s p=0.932 n=6
Check Time 16.44s (± 0.17%) 16.44s (± 0.15%) ~ 16.41s 16.48s p=1.000 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 20.94s (± 0.23%) 20.90s (± 0.28%) ~ 20.81s 20.97s p=0.225 n=6
vscode - node (v18.15.0, x64)
Errors 51 51 ~ ~ ~ p=1.000 n=6
Symbols 3,424,652 3,424,652 ~ ~ ~ p=1.000 n=6
Types 1,155,330 1,155,330 ~ ~ ~ p=1.000 n=6
Memory used 3,475,455k (± 0.01%) 3,475,635k (± 0.01%) ~ 3,475,406k 3,475,985k p=0.470 n=6
Parse Time 14.73s (± 0.58%) 14.76s (± 0.70%) ~ 14.68s 14.96s p=0.687 n=6
Bind Time 4.78s (± 0.34%) 4.77s (± 0.85%) ~ 4.72s 4.82s p=0.685 n=6
Check Time 96.77s (± 3.52%) 93.61s (± 0.24%) ~ 93.32s 93.87s p=0.066 n=6
Emit Time 30.60s (± 2.60%) 29.94s (± 0.87%) ~ 29.62s 30.35s p=0.173 n=6
Total Time 146.89s (± 2.84%) 143.08s (± 0.29%) ~ 142.55s 143.69s p=0.109 n=6
webpack - node (v18.15.0, x64)
Errors 2 2 ~ ~ ~ p=1.000 n=6
Symbols 317,848 317,848 ~ ~ ~ p=1.000 n=6
Types 140,485 140,485 ~ ~ ~ p=1.000 n=6
Memory used 473,083k (± 0.02%) 473,127k (± 0.02%) ~ 473,016k 473,231k p=0.378 n=6
Parse Time 5.16s (± 0.61%) 5.10s (± 1.34%) ~ 5.02s 5.18s p=0.170 n=6
Bind Time 2.25s (± 1.46%) 2.28s (± 0.95%) ~ 2.26s 2.31s p=0.075 n=6
Check Time 25.96s (± 0.50%) 26.00s (± 0.40%) ~ 25.87s 26.16s p=0.375 n=6
Emit Time 0.00s 0.00s (±244.70%) ~ 0.00s 0.01s p=0.405 n=6
Total Time 33.38s (± 0.37%) 33.38s (± 0.39%) ~ 33.15s 33.53s p=0.630 n=6
xstate-main - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 570,733 570,733 ~ ~ ~ p=1.000 n=6
Types 191,446 191,446 ~ ~ ~ p=1.000 n=6
Memory used 501,314k (± 0.01%) 501,319k (± 0.01%) ~ 501,200k 501,373k p=0.936 n=6
Parse Time 4.33s (± 0.38%) 4.32s (± 0.79%) ~ 4.26s 4.35s p=0.372 n=6
Bind Time 1.53s (± 0.69%) 1.52s (± 0.77%) ~ 1.51s 1.54s p=0.619 n=6
Check Time 24.95s (± 0.50%) 24.96s (± 0.27%) ~ 24.90s 25.09s p=1.000 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 30.81s (± 0.43%) 30.80s (± 0.22%) ~ 30.71s 30.91s p=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)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@ahejlsberg ahejlsberg

Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
Status: Not started
Milestone
No milestone
4 participants
@Andarist @RyanCavanaugh @typescript-bot @ahejlsberg