Skip to content

Avoid dependent parameters narrowings if any declared symbol of the parameter is assigned to #56313

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

Merged
merged 8 commits into from
Dec 11, 2023
Merged

Avoid dependent parameters narrowings if any declared symbol of the parameter is assigned to #56313

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
982f99a
Avoid dependent parameters narrowings if any declared symbols of the …
Andarist Nov 5, 2023
72b5004
Remove `isSymbolAssigned`
Andarist Nov 5, 2023
6e7aa4d
Merge remote-tracking branch 'origin/main' into fix/depenent-paramete…
Andarist Nov 29, 2023
0ccedd6
Merge remote-tracking branch 'origin/main' into fix/depenent-paramete…
Andarist Dec 6, 2023
a02be27
add a comment back
Andarist Dec 6, 2023
fc368df
Fixed the dependent parameters narrowing invalidation with contextual…
Andarist Dec 7, 2023
bbda1ed
Keep narrowing non-reassigned names coming from parameters
Andarist Dec 7, 2023
f82fbb4
Revert `isParameterDeclaration` change
Andarist Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 12 additions & 15 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27252,7 +27252,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
case SyntaxKind.Identifier:
if (!isThisInTypeQuery(node)) {
const symbol = getResolvedSymbol(node as Identifier);
return isConstantVariable(symbol) || isParameterOrCatchClauseVariable(symbol) && !isSymbolAssigned(symbol);
return isConstantVariable(symbol) || isParameterOrCatchClauseVariable(symbol) && !isSomeSymbolAssigned(getRootDeclaration(symbol.valueDeclaration!));
}
break;
case SyntaxKind.PropertyAccessExpression:
Expand Down Expand Up @@ -28527,37 +28527,33 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
node.kind === SyntaxKind.PropertyDeclaration)!;
}

// Check if a parameter or catch variable is assigned anywhere
function isSymbolAssigned(symbol: Symbol) {
if (!symbol.valueDeclaration) {
return false;
}
const parent = getRootDeclaration(symbol.valueDeclaration).parent;
function isSomeSymbolAssigned(rootDeclaration: Node) {
const parent = rootDeclaration.parent;
const links = getNodeLinks(parent);
if (!(links.flags & NodeCheckFlags.AssignmentsMarked)) {
links.flags |= NodeCheckFlags.AssignmentsMarked;
if (!hasParentWithAssignmentsMarked(parent)) {
markNodeAssignments(parent);
}
}
return symbol.isAssigned || false;
return getNodeLinks(rootDeclaration).someSymbolAssigned;
}

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 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Andarist Andarist Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (node.kind === SyntaxKind.Identifier) {
if (isAssignmentTarget(node)) {
const symbol = getResolvedSymbol(node as Identifier);
if (isParameterOrCatchClauseVariable(symbol)) {
symbol.isAssigned = true;
return getNodeLinks(getRootDeclaration(symbol.valueDeclaration!)).someSymbolAssigned = true;
}
}
}
else {
forEachChild(node, markNodeAssignments);
return forEachChild(node, markNodeAssignments);
}
}

Expand Down Expand Up @@ -28726,7 +28722,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const parentType = getTypeForBindingElementParent(parent, CheckMode.Normal);
const parentTypeConstraint = parentType && mapType(parentType, getBaseConstraintOrType);
links.flags &= ~NodeCheckFlags.InCheckIdentifier;
if (parentTypeConstraint && parentTypeConstraint.flags & TypeFlags.Union && !(rootDeclaration.kind === SyntaxKind.Parameter && isSymbolAssigned(symbol))) {
if (parentTypeConstraint && parentTypeConstraint.flags & TypeFlags.Union && !(rootDeclaration.kind === SyntaxKind.Parameter && isSomeSymbolAssigned(rootDeclaration))) {
const pattern = declaration.parent;
const narrowedType = getFlowTypeOfReference(pattern, parentTypeConstraint, parentTypeConstraint, /*flowContainer*/ undefined, location.flowNode);
if (narrowedType.flags & TypeFlags.Never) {
Expand Down Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

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

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
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const narrowedType = getFlowTypeOfReference(func, restType, restType, /*flowContainer*/ undefined, location.flowNode);
const index = func.parameters.indexOf(declaration) - (getThisParameter(func) ? 1 : 0);
return getIndexedAccessType(narrowedType, getNumberLiteralType(index));
Expand Down Expand Up @@ -28907,7 +28903,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// The declaration container is the innermost function that encloses the declaration of the variable
// or parameter. The flow container is the innermost function starting with which we analyze the control
// flow graph to determine the control flow based type.
const isParameter = getRootDeclaration(declaration).kind === SyntaxKind.Parameter;
const rootDeclaration = getRootDeclaration(declaration);
const isParameter = rootDeclaration.kind === SyntaxKind.Parameter;
const declarationContainer = getControlFlowContainer(declaration);
let flowContainer = getControlFlowContainer(node);
const isOuterVariable = flowContainer !== declarationContainer;
Expand All @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

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;
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😅

) {
flowContainer = getControlFlowContainer(flowContainer);
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5811,7 +5811,6 @@ export interface Symbol {
/** @internal */ constEnumOnlyModule: boolean | undefined; // True if module contains only const enums or other modules with only const enums
/** @internal */ isReferenced?: SymbolFlags; // True if the symbol is referenced elsewhere. Keeps track of the meaning of a reference in case a symbol is both a type parameter and parameter.
/** @internal */ isReplaceableByMethod?: boolean; // Can this Javascript class property be replaced by a method symbol?
/** @internal */ isAssigned?: boolean; // True if the symbol is a parameter with assignments
/** @internal */ assignmentDeclarationMembers?: Map<number, Declaration>; // detected late-bound assignment declarations associated with the symbol
}

Expand Down Expand Up @@ -6050,6 +6049,7 @@ export interface NodeLinks {
parameterInitializerContainsUndefined?: boolean; // True if this is a parameter declaration whose type annotation contains "undefined".
fakeScopeForSignatureDeclaration?: "params" | "typeParams"; // If present, this is a fake scope injected into an enclosing declaration chain.
assertionExpressionType?: Type; // Cached type of the expression of a type assertion
someSymbolAssigned?: boolean; // True if any symbol declared in a parameter or catch clause is being assigned to
}

/** @internal */
Expand Down
1 change: 0 additions & 1 deletion src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8155,7 +8155,6 @@ function Symbol(this: Symbol, flags: SymbolFlags, name: __String) {
this.exportSymbol = undefined;
this.constEnumOnlyModule = undefined;
this.isReferenced = undefined;
this.isAssigned = undefined;
(this as any).links = undefined; // used by TransientSymbol
}

Expand Down
20 changes: 20 additions & 0 deletions tests/baselines/reference/dependentDestructuredVariables.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,4 +443,24 @@ dependentDestructuredVariables.ts(431,15): error TS2322: Type 'number' is not as
!!! error TS2322: Type 'number' is not assignable to type 'never'.
}
}

// https://github.com/microsoft/TypeScript/issues/56312

function parameterReassigned1([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
x = 1;
}
if (y === 2) {
x; // 1 | 3
}
}

function parameterReassigned2([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
}

39 changes: 39 additions & 0 deletions tests/baselines/reference/dependentDestructuredVariables.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,26 @@ function tooNarrow([x, y]: [1, 1] | [1, 2] | [1]) {
const shouldNotBeOk: never = x; // Error
}
}

// https://github.com/microsoft/TypeScript/issues/56312

function parameterReassigned1([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
x = 1;
}
if (y === 2) {
x; // 1 | 3
}
}

function parameterReassigned2([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
}


//// [dependentDestructuredVariables.js]
Expand Down Expand Up @@ -766,6 +786,23 @@ function tooNarrow([x, y]) {
const shouldNotBeOk = x; // Error
}
}
// https://github.com/microsoft/TypeScript/issues/56312
function parameterReassigned1([x, y]) {
if (Math.random()) {
x = 1;
}
if (y === 2) {
x; // 1 | 3
}
}
function parameterReassigned2([x, y]) {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
}


//// [dependentDestructuredVariables.d.ts]
Expand Down Expand Up @@ -916,3 +953,5 @@ declare class Client {
declare const bot: Client;
declare function fz1([x, y]: [1, 2] | [3, 4] | [5]): void;
declare function tooNarrow([x, y]: [1, 1] | [1, 2] | [1]): void;
declare function parameterReassigned1([x, y]: [1, 2] | [3, 4]): void;
declare function parameterReassigned2([x, y]: [1, 2] | [3, 4]): void;
44 changes: 44 additions & 0 deletions tests/baselines/reference/dependentDestructuredVariables.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,3 +1100,47 @@ function tooNarrow([x, y]: [1, 1] | [1, 2] | [1]) {
}
}

// https://github.com/microsoft/TypeScript/issues/56312

function parameterReassigned1([x, y]: [1, 2] | [3, 4]) {
>parameterReassigned1 : Symbol(parameterReassigned1, Decl(dependentDestructuredVariables.ts, 432, 1))
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 436, 31))
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 436, 33))

if (Math.random()) {
>Math.random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))
>Math : Symbol(Math, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.core.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --))
>random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))

x = 1;
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 436, 31))
}
if (y === 2) {
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 436, 33))

x; // 1 | 3
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 436, 31))
}
}

function parameterReassigned2([x, y]: [1, 2] | [3, 4]) {
>parameterReassigned2 : Symbol(parameterReassigned2, Decl(dependentDestructuredVariables.ts, 443, 1))
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 445, 31))
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 445, 33))

if (Math.random()) {
>Math.random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))
>Math : Symbol(Math, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.core.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --))
>random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))

y = 2;
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 445, 33))
}
if (y === 2) {
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 445, 33))

x; // 1 | 3
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 445, 31))
}
}

54 changes: 54 additions & 0 deletions tests/baselines/reference/dependentDestructuredVariables.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1263,3 +1263,57 @@ function tooNarrow([x, y]: [1, 1] | [1, 2] | [1]) {
}
}

// https://github.com/microsoft/TypeScript/issues/56312

function parameterReassigned1([x, y]: [1, 2] | [3, 4]) {
>parameterReassigned1 : ([x, y]: [1, 2] | [3, 4]) => void
>x : 1 | 3
>y : 2 | 4

if (Math.random()) {
>Math.random() : number
>Math.random : () => number
>Math : Math
>random : () => number

x = 1;
>x = 1 : 1
>x : 1 | 3
>1 : 1
}
if (y === 2) {
>y === 2 : boolean
>y : 2 | 4
>2 : 2

x; // 1 | 3
>x : 1 | 3
}
}

function parameterReassigned2([x, y]: [1, 2] | [3, 4]) {
>parameterReassigned2 : ([x, y]: [1, 2] | [3, 4]) => void
>x : 1 | 3
>y : 2 | 4

if (Math.random()) {
>Math.random() : number
>Math.random : () => number
>Math : Math
>random : () => number

y = 2;
>y = 2 : 2
>y : 2 | 4
>2 : 2
}
if (y === 2) {
>y === 2 : boolean
>y : 2 | 4
>2 : 2

x; // 1 | 3
>x : 1 | 3
}
}

20 changes: 20 additions & 0 deletions tests/cases/conformance/controlFlow/dependentDestructuredVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,3 +436,23 @@ function tooNarrow([x, y]: [1, 1] | [1, 2] | [1]) {
const shouldNotBeOk: never = x; // Error
}
}

// https://github.com/microsoft/TypeScript/issues/56312

function parameterReassigned1([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
x = 1;
}
if (y === 2) {
x; // 1 | 3
}
}

function parameterReassigned2([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
}