Use faster, stricter prop type comparison when merging props in union prop creation #43696
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot run dt |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 1035416. You can monitor the build here. |
weswigham
force-pushed
the
fix-backbone-stackout
branch
from
1035416 to
586bb7b
Compare
|
@typescript-bot run dt |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 488ed1f. You can monitor the build here. |
src/compiler/checker.ts
Outdated
| typeParameters = (target.objectFlags & ObjectFlags.Reference || target.symbol.flags & SymbolFlags.TypeLiteral) && !target.aliasTypeArguments ? | ||
| filter(typeParameters, tp => isTypeParameterPossiblyReferenced(tp, declaration)) : | ||
| typeParameters = (target.objectFlags & ObjectFlags.Reference || target.symbol.flags & SymbolFlags.Method || target.symbol.flags & SymbolFlags.TypeLiteral) && !target.aliasTypeArguments ? | ||
| filter(typeParameters, tp => isTypeParameterPossiblyReferenced(tp, declaration) || !(type.objectFlags & ObjectFlags.Reference) && some(type.symbol.declarations, d => isTypeParameterPossiblyReferenced(tp, d))) : |
There was a problem hiding this comment.
why skip object references here? are references really the only object types that won't work?
I'd suggest instead declaring
const allDeclarations = type.objectFlags & ObjectFlags.Reference ? [(<TypeReference>type).node!] : type.symbol.declarations! just below declarations, then:
| filter(typeParameters, tp => isTypeParameterPossiblyReferenced(tp, declaration) || !(type.objectFlags & ObjectFlags.Reference) && some(type.symbol.declarations, d => isTypeParameterPossiblyReferenced(tp, d))) : | |
| filter(typeParameters, tp => some(allDeclarations, d => isTypeParameterPossiblyReferenced(tp, d)) : |
There was a problem hiding this comment.
Because References use their .node to track their content, not their declarations. Because reasons.
There was a problem hiding this comment.
I think I'd still prefer
const allDeclarations = type.objectFlags & ObjectFlags.Reference ? [] : type.symbol.declarations!
then
|
@typescript-bot run dt |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 252b34b. You can monitor the build here. |
| @@ -15930,6 +15931,9 @@ namespace ts { | |||
| getTypeFromTypeNodeWorker(<TypeNode>node) === tp; // use worker because we're looking for === equality | |||
| case SyntaxKind.TypeQuery: | |||
| return true; | |||
| case SyntaxKind.MethodDeclaration: | |||
| case SyntaxKind.MethodSignature: | |||
| return (!(node as FunctionLikeDeclaration).type && !!(node as FunctionLikeDeclaration).body) || !!forEachChild(node, containsReference); | |||
There was a problem hiding this comment.
@weswigham I'm fixing #44292 which was indirectly caused by the changes in this PR. Now I'm looking at this code and I'm not really sure why you return true if there's no type annotation but a function body, but otherwise you visit every node in the function body.
There was a problem hiding this comment.
Before your changes in this PR, we'd only call isTypeParameterPossiblyReferenced on something that is a type. Now we can end up crawling through method bodies which this code wasn't intended to do.
Fixes #43690