-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Port "Improve type discrimination algorithm" from tsgo #61828
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
Conversation
@typescript-bot test it |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Despite the above, xstate has 30 new errors. |
Are those published anywhere? I don't see them being reported here anywhere. |
They're silent during benchmarks since benchmarks often have errors. Would have to build and try directly. |
The failures are:
These are all just expected errors that move. So, seemingly fine. |
fix = { | ||
...existingFix, | ||
addAsTypeOnly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broke because fix
is a union of objects, except the two props added here are not actually in two of the fixes, so the code was actually tacking random props onto fixes that would then simply never be read by anyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Port the improved type discrimination algorithm from typescript-go
along with related test updates and import-fix refinements.
- Add new
missingDiscriminants.ts
tests and corresponding baselines to validate discriminant property handling. - Remove outdated
discriminateWithMissingProperty
error baseline. - Refine
createImportAdderWorker
to skip reusing namespace and JSDoc-type imports. - Adjust discrimination loop in
checker.ts
to explicitly guard on property existence before relational checks.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/cases/compiler/missingDiscriminants.ts | New test cases for missing-discriminant scenarios |
tests/baselines/reference/missingDiscriminants.types | Updated expected type-print baselines for discriminant improvements |
tests/baselines/reference/missingDiscriminants.symbols | Updated symbol baselines reflecting new discriminant logic |
tests/baselines/reference/missingDiscriminants.errors.txt | Updated error baselines for discriminant test failures |
tests/baselines/reference/discriminateWithMissingProperty.errors.txt | Removed obsolete error baseline for missing-property discrimination |
src/services/codefixes/importFixes.ts | Exclude namespace and JSDoc-type imports from reuse in import fixes |
src/compiler/checker.ts | Split discriminant property check from relation check in loop |
Comments suppressed due to low confidence (2)
src/services/codefixes/importFixes.ts:373
- No tests cover the new conditions excluding
ImportFixKind.UseNamespace
andImportFixKind.JsdocTypeImport
; consider adding unit tests to validate that existing imports of these kinds are not reused.
if (existingFix && importKind !== ImportKind.Namespace && existingFix.kind !== ImportFixKind.UseNamespace && existingFix.kind !== ImportFixKind.JsdocTypeImport) {
src/compiler/checker.ts:24771
- [nitpick] Add a comment explaining why types without the
propertyName
discriminant are skipped (i.e., notargetType
), to clarify the intended control flow in the discrimination algorithm.
if (targetType) {
@@ -370,7 +370,7 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog | ||
); | ||
|
||
let fix: FixAddNewImport | ImportFixWithModuleSpecifier; | ||
if (existingFix && importKind !== ImportKind.Namespace) { | ||
if (existingFix && importKind !== ImportKind.Namespace && existingFix.kind !== ImportFixKind.UseNamespace && existingFix.kind !== ImportFixKind.JsdocTypeImport) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This boolean condition is getting lengthy and complex; consider extracting the kind checks into a well-named helper function (e.g., shouldReuseImportFix(existingFix, importKind)
) for readability.
Copilot uses AI. Check for mistakes.
Porting microsoft/typescript-go#1085.