Avoid dependent parameters narrowings if any declared symbol of the parameter is assigned to #56313
Conversation
…parameter are assigned to
typescript-bot
added
the
For Uncommitted Bug
Andarist
changed the title
typescript-bot
added
For Backlog Bug
|
@typescript-bot run DT |
|
Heya @gabritto, I've started to run the regular perf test suite on this PR at 6e7aa4d. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at 6e7aa4d. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at 6e7aa4d. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at 6e7aa4d. You can monitor the build here. Update: The results are in! |
|
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@gabritto Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @gabritto, the results of running the DT tests are ready. |
|
It seems some error has gone missing from lodash? #56313 (comment) Also, this one does seem like a bug: #56313 (comment) |
|
@gabritto Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@typescript-bot pack this |
|
Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
src/compiler/checker.ts
Outdated
| } | ||
|
|
||
| function hasParentWithAssignmentsMarked(node: Node) { | ||
| return !!findAncestor(node.parent, node => (isFunctionLike(node) || isCatchClause(node)) && !!(getNodeLinks(node).flags & NodeCheckFlags.AssignmentsMarked)); | ||
| } | ||
|
|
||
| function markNodeAssignments(node: Node) { | ||
| function markNodeAssignments(node: Node): true | undefined { |
There was a problem hiding this comment.
Can we not return in this function, the way it was before? I think it's not really necessary and potentially confusing, since we don't use the results returned by this function.
There was a problem hiding this comment.
since we don't use the results returned by this function.
It is used by forEachChild. This return acts as an early return for its iteration.
That being said, I probably need to mark both individual symbols as assigned or not together with the aggregate information on the root declaration to fix the regression found here.
A minimal repro case for this regression:
declare function useRouter(): {
push: (href: string) => void;
}
type SidebarFooterButtonProps = {
href?: string;
onClick?: () => void;
};
export const SidebarFooterButton = ({
href,
onClick,
}: SidebarFooterButtonProps): JSX.Element => {
const router = useRouter();
if (href !== undefined) {
onClick = () => {
void router.push(href);
};
}
return (
<button type="button" onClick={onClick} />
);
};We still want to narrow "const" parameters and only turn off the dependent parameter narrowing if some of the relevant symbols are reassigned - so we need to keep track of both (or well, derive the aggregate information on demand from the root declaration's symbols)
So If I do any of those, this confusing return will go way :p
| @@ -28921,7 +28918,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| while ( | |||
| flowContainer !== declarationContainer && (flowContainer.kind === SyntaxKind.FunctionExpression || | |||
| flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethodOrAccessor(flowContainer)) && | |||
| (isConstantVariable(localOrExportSymbol) && type !== autoArrayType || isParameter && !isSymbolAssigned(localOrExportSymbol)) | |||
| (isConstantVariable(localOrExportSymbol) && type !== autoArrayType || isParameter && !isSomeSymbolAssigned(rootDeclaration)) | |||
There was a problem hiding this comment.
It looks like the change to isSymbolAssigned behavior, to aggregate isAssigned at the root declaration level, might not be the correct thing to do here, for this particular usage of isSymbolAssigned, as evidenced by this breaking change: #56313 (comment).
Maybe we want to keep marking symbols with isAssigned, and also aggregate that info at the root declaration level, since getNarrowedTypeOfSymbol needs the aggregate info but checkIdentifier needs the symbol info.
Here's a more minimal repro of the break linked above:
function ff({ a, b }: { a: string | undefined, b: () => void }) {
if (a !== undefined) {
b = () => {
const x: string = a;
}
}
}There was a problem hiding this comment.
Heh, I really need to start reading through all of the existing comments at once instead of reading through them one by one. It would save me a few minutes here 😅
src/compiler/checker.ts
Outdated
| @@ -28766,7 +28762,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const contextualSignature = getContextualSignature(func); | |||
| if (contextualSignature && contextualSignature.parameters.length === 1 && signatureHasRestParameter(contextualSignature)) { | |||
| const restType = getReducedApparentType(instantiateType(getTypeOfSymbol(contextualSignature.parameters[0]), getInferenceContext(func)?.nonFixingMapper)); | |||
| if (restType.flags & TypeFlags.Union && everyType(restType, isTupleType) && !isSymbolAssigned(symbol)) { | |||
| if (restType.flags & TypeFlags.Union && everyType(restType, isTupleType) && !isSomeSymbolAssigned(declaration)) { | |||
There was a problem hiding this comment.
I think this case here is not fixed by aggregating isAssigned at root declaration level, since the root declaration will be a parameter declaration, but the other dependent parameter could have been reassigned:
const test2: (...args: [1, 2] | [3, 4]) => void = (x, y) => {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x
// ^? (parameter) x: 1
}
}There was a problem hiding this comment.
A great catch! The "dependent root declaration" might come from the context... I'll have to think about this and recheck how this kind of narrowing is handled today.
src/compiler/utilities.ts
Outdated
| @@ -5278,7 +5278,7 @@ export function isPushOrUnshiftIdentifier(node: Identifier) { | |||
| * | |||
| * @internal | |||
| */ | |||
| export function isParameterDeclaration(node: Declaration): boolean { | |||
| export function isParameterDeclaration(node: Declaration): node is ParameterDeclaration { | |||
There was a problem hiding this comment.
Did you forget to undo this maybe? I think despite the terribly misleading name, this function is not meant to return whether node is ParameterDeclaration.
There was a problem hiding this comment.
Everything else seems good now and I'll approve and merge once this parameter declaration thing is gone.
There was a problem hiding this comment.
Yeah, totally! I could swear that I have reverted this... but I guess I didn't 😬 I fixed this just now.
There was a problem hiding this comment.
Yeah, this definitely is not a type guard. See also #52283.
|
@typescript-bot user test this |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at bbda1ed. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at bbda1ed. You can monitor the build here. Update: The results are in! |
|
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
…arameter is assigned to (microsoft#56313)
fixes #56312