Skip to content

Conversation

ahejlsberg
Copy link
Member

@Copilot Copilot AI review requested due to automatic review settings September 28, 2025 13:34
@github-project-automation github-project-automation bot moved this to Not started in PR Backlog Sep 28, 2025
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 28, 2025
@ahejlsberg
Copy link
Member Author

@typescript-bot test it

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 28, 2025

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

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 ports a fix from the TypeScript Go repository (#1757) that addresses discriminated union type handling when discriminants are missing. The change modifies the discriminated union logic to better handle cases where union members lack discriminant properties.

Key Changes

  • Modified discriminant property checking logic in the type checker
  • Simplified conditional structure by combining null and literal type checks
  • Added test case demonstrating the fixed behavior with missing discriminants

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/compiler/checker.ts Simplifies discriminant validation logic by combining discriminant existence and literal type checks
tests/cases/compiler/missingDiscriminants2.ts Adds test case with union types having missing discriminant properties
tests/baselines/reference/missingDiscriminants2.types Baseline for type checking results of the new test
tests/baselines/reference/missingDiscriminants2.symbols Baseline for symbol resolution results of the new test
tests/baselines/reference/missingDiscriminants2.errors.txt Baseline for expected error messages in the new test
Comments suppressed due to low confidence (1)

src/compiler/checker.ts:1

  • [nitpick] The refactored condition combines null check and literal type validation into a single statement, which is more concise. However, consider adding a comment explaining why we return undefined when either condition is met, as this affects discriminated union behavior.
import {

@typescript-bot
Copy link
Collaborator

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

There were interesting changes:

Branch only errors:

Package: css-tree
Error:

Error: 
/mnt/vss/_work/1/DefinitelyTyped/types/css-tree/css-tree-tests.ts
  18:11  error  TypeScript@local compile error: 
Parameter 'node' implicitly has an 'any' type                                                                                                                                                       @definitelytyped/expect
  18:17  error  TypeScript@local compile error: 
Parameter 'item' implicitly has an 'any' type                                                                                                                                                       @definitelytyped/expect
  18:23  error  TypeScript@local compile error: 
Parameter 'list' implicitly has an 'any' type                                                                                                                                                       @definitelytyped/expect
  19:9   error  TypeScript@local expected type to be:
  symbol
got:
  any                                                                                                                                                                            @definitelytyped/expect
  19:14  error  TypeScript@local compile error: 
Property 'break' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'break' does not exist on type 'WalkOptionsNoVisit'            @definitelytyped/expect
  20:9   error  TypeScript@local expected type to be:
  symbol
got:
  any                                                                                                                                                                            @definitelytyped/expect
  20:14  error  TypeScript@local compile error: 
Property 'skip' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'skip' does not exist on type 'WalkOptionsNoVisit'              @definitelytyped/expect
  21:9   error  TypeScript@local expected type to be:
  CssNode
got:
  any                                                                                                                                                                           @definitelytyped/expect
  21:14  error  TypeScript@local compile error: 
Property 'root' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'root' does not exist on type 'WalkOptionsNoVisit'              @definitelytyped/expect
  22:9   error  TypeScript@local expected type to be:
  StyleSheet | null
got:
  any                                                                                                                                                                 @definitelytyped/expect
  22:14  error  TypeScript@local compile error: 
Property 'stylesheet' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'stylesheet' does not exist on type 'WalkOptionsNoVisit'  @definitelytyped/expect
  23:9   error  TypeScript@local expected type to be:
  ClassSelector
got:
  any                                                                                                                                                                     @definitelytyped/expect
  24:9   error  TypeScript@local expected type to be:
  ListItem<CssNode>
got:
  any                                                                                                                                                                 @definitelytyped/expect
  25:9   error  TypeScript@local expected type to be:
  List<CssNode>
got:
  any                                                                                                                                                                     @definitelytyped/expect
  26:9   error  TypeScript@local expected type to be:
  Atrule | null
got:
  any                                                                                                                                                                     @definitelytyped/expect
  26:14  error  TypeScript@local compile error: 
Property 'atrule' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'atrule' does not exist on type 'WalkOptionsNoVisit'          @definitelytyped/expect
  28:11  error  TypeScript@local compile error: 
Parameter 'node' implicitly has an 'any' type                                                                                                                                                       @definitelytyped/expect
  28:17  error  TypeScript@local compile error: 
Parameter 'item' implicitly has an 'any' type                                                                                                                                                       @definitelytyped/expect
  28:23  error  TypeScript@local compile error: 
Parameter 'list' implicitly has an 'any' type                                                                                                                                                       @definitelytyped/expect
  29:9   error  TypeScript@local expected type to be:
  symbol
got:
  any                                                                                                                                                                            @definitelytyped/expect
  29:14  error  TypeScript@local compile error: 
Property 'break' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'break' does not exist on type 'WalkOptionsNoVisit'            @definitelytyped/expect
  30:9   error  TypeScript@local expected type to be:
  symbol
got:
  any                                                                                                                                                                            @definitelytyped/expect
  30:14  error  TypeScript@local compile error: 
Property 'skip' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'skip' does not exist on type 'WalkOptionsNoVisit'              @definitelytyped/expect
  31:9   error  TypeScript@local expected type to be:
  CssNode
got:
  any                                                                                                                                                                           @definitelytyped/expect
  31:14  error  TypeScript@local compile error: 
Property 'root' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'root' does not exist on type 'WalkOptionsNoVisit'              @definitelytyped/expect
  32:9   error  TypeScript@local expected type to be:
  StyleSheet | null
got:
  any                                                                                                                                                                 @definitelytyped/expect
  32:14  error  TypeScript@local compile error: 
Property 'stylesheet' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'stylesheet' does not exist on type 'WalkOptionsNoVisit'  @definitelytyped/expect
  33:9   error  TypeScript@local expected type to be:
  ClassSelector
got:
  any                                                                                                                                                                     @definitelytyped/expect
  34:9   error  TypeScript@local expected type to be:
  ListItem<CssNode>
got:
  any                                                                                                                                                                 @definitelytyped/expect
  35:9   error  TypeScript@local expected type to be:
  List<CssNode>
got:
  any                                                                                                                                                                     @definitelytyped/expect
  36:9   error  TypeScript@local expected type to be:
  Atrule | null
got:
  any                                                                                                                                                                     @definitelytyped/expect
  36:14  error  TypeScript@local compile error: 
Property 'atrule' does not exist on type 'WalkOptionsNoVisit | EnterOrLeaveFn<CssNode> | WalkOptionsVisit<ClassSelector>'.
  Property 'atrule' does not exist on type 'WalkOptionsNoVisit'          @definitelytyped/expect

✖ 32 problems (32 errors, 0 warnings)

    at combineErrorsAndWarnings (/mnt/vss/_work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.34_typescript@6.0.0-dev.20250928/node_modules/@definitelytyped/dtslint/dist/index.js:199:28)
    at runTests (/mnt/vss/_work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.34_typescript@6.0.0-dev.20250928/node_modules/@definitelytyped/dtslint/dist/index.js:191:20)

You can check the log here.

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Git clone failed"

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

@ahejlsberg
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,370 62,370 ~ ~ ~ p=1.000 n=6
Types 50,386 50,386 ~ ~ ~ p=1.000 n=6
Memory used 193,398k (± 0.77%) 193,945k (± 0.96%) ~ 192,724k 196,376k p=0.230 n=6
Parse Time 1.29s (± 0.63%) 1.30s (± 0.58%) ~ 1.29s 1.31s p=0.120 n=6
Bind Time 0.72s (± 0.57%) 0.72s ~ ~ ~ p=0.405 n=6
Check Time 9.77s (± 0.46%) 9.73s (± 0.48%) ~ 9.68s 9.81s p=0.228 n=6
Emit Time 2.75s (± 0.78%) 2.75s (± 0.86%) ~ 2.72s 2.79s p=0.746 n=6
Total Time 14.54s (± 0.39%) 14.51s (± 0.39%) ~ 14.45s 14.60s p=0.421 n=6
angular-1 - node (v18.15.0, x64)
Errors 1 1 ~ ~ ~ p=1.000 n=6
Symbols 948,914 948,812 -102 (- 0.01%) ~ ~ p=0.001 n=6
Types 410,884 410,869 -15 (- 0.00%) ~ ~ p=0.001 n=6
Memory used 1,245,599k (± 0.00%) 1,245,541k (± 0.00%) ~ 1,245,476k 1,245,618k p=0.066 n=6
Parse Time 6.49s (± 1.03%) 6.50s (± 0.74%) ~ 6.45s 6.58s p=0.377 n=6
Bind Time 1.87s (± 0.40%) 1.88s (± 0.29%) ~ 1.87s 1.88s p=0.476 n=6
Check Time 32.24s (± 0.28%) 32.24s (± 0.23%) ~ 32.11s 32.34s p=1.000 n=6
Emit Time 14.77s (± 0.37%) 14.81s (± 0.42%) ~ 14.71s 14.90s p=0.421 n=6
Total Time 55.37s (± 0.37%) 55.42s (± 0.20%) ~ 55.27s 55.58s p=0.422 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,544,632 2,544,624 -8 (- 0.00%) ~ ~ p=0.001 n=6
Types 903,208 903,207 -1 (- 0.00%) ~ ~ p=0.001 n=6
Memory used 2,829,870k (± 0.01%) 2,829,900k (± 0.01%) ~ 2,829,710k 2,830,104k p=0.575 n=6
Parse Time 8.72s (± 0.41%) 8.71s (± 0.39%) ~ 8.65s 8.75s p=0.872 n=6
Bind Time 2.19s (± 0.24%) 2.20s (± 0.55%) ~ 2.18s 2.21s p=0.142 n=6
Check Time 86.74s (± 1.13%) 86.60s (± 1.35%) ~ 85.75s 88.15s p=0.298 n=6
Emit Time 0.34s (±29.11%) 0.62s (±125.19%) ~ 0.30s 2.20s p=0.787 n=6
Total Time 97.99s (± 0.98%) 98.13s (± 1.21%) ~ 96.98s 99.30s p=0.810 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,237,064 1,237,064 ~ ~ ~ p=1.000 n=6
Types 259,743 259,743 ~ ~ ~ p=1.000 n=6
Memory used 2,425,016k (± 6.12%) 2,364,458k (± 0.01%) ~ 2,364,264k 2,364,893k p=0.936 n=6
Parse Time 5.14s (± 0.69%) 5.15s (± 1.28%) ~ 5.08s 5.24s p=0.810 n=6
Bind Time 1.77s (± 0.55%) 1.78s (± 0.92%) ~ 1.76s 1.80s p=0.934 n=6
Check Time 35.05s (± 0.68%) 35.13s (± 0.33%) ~ 34.99s 35.26s p=0.574 n=6
Emit Time 3.00s (± 0.46%) 3.03s (± 1.99%) ~ 2.96s 3.13s p=0.296 n=6
Total Time 44.99s (± 0.50%) 45.08s (± 0.27%) ~ 44.91s 45.21s p=0.575 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,237,064 1,237,064 ~ ~ ~ p=1.000 n=6
Types 259,743 259,743 ~ ~ ~ p=1.000 n=6
Memory used 3,158,348k (± 0.02%) 3,158,381k (± 0.04%) ~ 3,156,854k 3,159,950k p=1.000 n=6
Parse Time 6.85s (± 0.79%) 6.86s (± 0.64%) ~ 6.79s 6.91s p=0.748 n=6
Bind Time 2.16s (± 1.23%) 2.14s (± 1.43%) ~ 2.09s 2.18s p=0.226 n=6
Check Time 42.57s (± 0.23%) 42.61s (± 0.29%) ~ 42.46s 42.74s p=0.521 n=6
Emit Time 3.50s (± 2.43%) 3.51s (± 2.49%) ~ 3.37s 3.62s p=0.936 n=6
Total Time 55.09s (± 0.24%) 55.14s (± 0.34%) ~ 54.91s 55.39s p=0.688 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 262,412 262,412 ~ ~ ~ p=1.000 n=6
Types 103,924 103,924 ~ ~ ~ p=1.000 n=6
Memory used 440,512k (± 0.01%) 440,508k (± 0.01%) ~ 440,444k 440,558k p=1.000 n=6
Parse Time 3.52s (± 1.49%) 3.51s (± 0.58%) ~ 3.47s 3.52s p=0.360 n=6
Bind Time 1.32s (± 1.30%) 1.32s (± 1.42%) ~ 1.28s 1.33s p=0.455 n=6
Check Time 19.00s (± 0.33%) 18.99s (± 0.36%) ~ 18.87s 19.06s p=0.936 n=6
Emit Time 1.53s (± 1.69%) 1.54s (± 1.42%) ~ 1.51s 1.57s p=0.373 n=6
Total Time 25.36s (± 0.45%) 25.36s (± 0.25%) ~ 25.26s 25.45s p=1.000 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 72 72 ~ ~ ~ p=1.000 n=6
Symbols 225,367 225,367 ~ ~ ~ p=1.000 n=6
Types 94,290 94,290 ~ ~ ~ p=1.000 n=6
Memory used 370,048k (± 0.04%) 370,098k (± 0.03%) ~ 369,998k 370,220k p=0.378 n=6
Parse Time 2.83s (± 0.32%) 2.85s (± 1.19%) ~ 2.81s 2.89s p=0.292 n=6
Bind Time 1.60s (± 1.02%) 1.59s (± 0.62%) ~ 1.58s 1.60s p=0.117 n=6
Check Time 16.45s (± 0.27%) 16.53s (± 0.58%) ~ 16.45s 16.71s p=0.107 n=6
Emit Time 0.00s (±244.70%) 0.00s ~ ~ ~ p=0.405 n=6
Total Time 20.89s (± 0.26%) 20.97s (± 0.53%) ~ 20.88s 21.19s p=0.143 n=6
vscode - node (v18.15.0, x64)
Errors 1 1 ~ ~ ~ p=1.000 n=6
Symbols 3,890,940 3,890,959 +19 (+ 0.00%) ~ ~ p=0.001 n=6
Types 1,224,250 1,224,257 +7 (+ 0.00%) ~ ~ p=0.001 n=6
Memory used 3,704,544k (± 0.01%) 3,704,490k (± 0.01%) ~ 3,704,207k 3,704,640k p=0.471 n=6
Parse Time 15.14s (± 0.70%) 15.18s (± 0.76%) ~ 14.98s 15.31s p=0.574 n=6
Bind Time 4.90s (± 0.76%) 4.90s (± 0.66%) ~ 4.88s 4.96s p=1.000 n=6
Check Time 103.67s (± 5.25%) 99.77s (± 0.33%) ~ 99.33s 100.25s p=0.173 n=6
Emit Time 40.51s (±17.06%) 34.51s (±19.63%) ~ 30.40s 47.24s p=0.066 n=6
Total Time 164.23s (± 4.74%) 154.36s (± 4.46%) 🟩-9.87s (- 6.01%) 149.96s 167.34s p=0.031 n=6
webpack - node (v18.15.0, x64)
Errors 38 38 ~ ~ ~ p=1.000 n=6
Symbols 361,753 361,753 ~ ~ ~ p=1.000 n=6
Types 158,862 158,862 ~ ~ ~ p=1.000 n=6
Memory used 516,556k (± 0.02%) 516,452k (± 0.02%) ~ 516,351k 516,550k p=0.066 n=6
Parse Time 4.46s (± 0.46%) 4.45s (± 1.11%) ~ 4.39s 4.53s p=0.687 n=6
Bind Time 1.90s (± 1.88%) 1.91s (± 2.14%) ~ 1.87s 1.98s p=1.000 n=6
Check Time 21.79s (± 0.21%) 21.77s (± 0.28%) ~ 21.67s 21.84s p=0.521 n=6
Emit Time 0.00s 0.00s (±244.70%) ~ 0.00s 0.01s p=0.405 n=6
Total Time 28.14s (± 0.23%) 28.13s (± 0.31%) ~ 27.97s 28.20s p=0.936 n=6
xstate-main - node (v18.15.0, x64)
Errors 30 30 ~ ~ ~ p=1.000 n=6
Symbols 672,790 672,790 ~ ~ ~ p=1.000 n=6
Types 201,962 201,962 ~ ~ ~ p=1.000 n=6
Memory used 574,902k (± 0.02%) 574,934k (± 0.02%) ~ 574,700k 575,017k p=0.575 n=6
Parse Time 4.18s (± 0.53%) 4.20s (± 0.82%) ~ 4.17s 4.25s p=0.285 n=6
Bind Time 1.33s (± 1.29%) 1.34s (± 0.62%) ~ 1.34s 1.36s p=0.096 n=6
Check Time 20.67s (± 2.18%) 20.38s (± 1.16%) ~ 20.16s 20.84s p=0.423 n=6
Emit Time 0.00s 0.00s (±244.70%) ~ 0.00s 0.01s p=0.405 n=6
Total Time 26.18s (± 1.65%) 25.92s (± 0.87%) ~ 25.75s 26.37s p=0.471 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

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

Everything looks good!

@jakebailey
Copy link
Member

Seems like there's a break?

@ahejlsberg
Copy link
Member Author

Seems like there's a break?

Yes, that's the same break that caused us to look at the issue in the first place. The problem is that the WalkOptions union includes a WalkOptionsNoVisit constituent that doesn't declare a discriminant. The discrimination logic used to eliminate it, but that isn't correct and doesn't happen following #61828 and this PR.

The fix is to add a visit discriminant property to WalkOptionsNoVisit:

export interface WalkOptionsNoVisit {
    visit?: never;
    enter?: EnterOrLeaveFn | undefined;
    leave?: EnterOrLeaveFn | undefined;
    reverse?: boolean | undefined;
}

@jakebailey
Copy link
Member

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2025

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

Command Status Results
run dt ✅ Started ✅ Results

@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.

@github-project-automation github-project-automation bot moved this from Not started to Needs merge in PR Backlog Sep 29, 2025
@ahejlsberg ahejlsberg merged commit 97610a8 into main Sep 29, 2025
33 checks passed
@ahejlsberg ahejlsberg deleted the port-tsgo-1757 branch September 29, 2025 21:42
@github-project-automation github-project-automation bot moved this from Needs merge to Done in PR Backlog Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants