Skip to content
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

Fix fallthrough and default in new switch-true narrowing #55991

Merged
merged 7 commits into from
Oct 6, 2023
Merged

Fix fallthrough and default in new switch-true narrowing #55991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e363ed5
Add tests
jakebailey Oct 5, 2023
e3b21af
Fix fallthrough and default in new switch-true narrowing
jakebailey Oct 5, 2023
cf0aa46
Add bad test case
jakebailey Oct 5, 2023
23f25c7
Narrow like we're executing
jakebailey Oct 6, 2023
7eeee09
Add a case which surprisingly works the same as ifs
jakebailey Oct 6, 2023
aad8562
Simplify, comment
jakebailey Oct 6, 2023
af2c612
Use plain loops for pre/post case set iteration
jakebailey Oct 6, 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
36 changes: 31 additions & 5 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27424,11 +27424,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
type = narrowTypeBySwitchOnTypeOf(type, flow.switchStatement, flow.clauseStart, flow.clauseEnd);
}
else if (expr.kind === SyntaxKind.TrueKeyword) {
const clause = flow.switchStatement.caseBlock.clauses.find((_, index) => index === flow.clauseStart);
const clauseExpression = clause && clause.kind === SyntaxKind.CaseClause ? clause.expression : undefined;
if (clauseExpression) {
type = narrowType(type, clauseExpression, /*assumeTrue*/ true);
}
type = narrowTypeBySwitchOnTrue(type, flow.switchStatement, flow.clauseStart, flow.clauseEnd);
}
else {
if (strictNullChecks) {
Expand Down Expand Up @@ -28043,6 +28039,36 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return getUnionType(map(clauseWitnesses, text => text ? narrowTypeByTypeName(type, text) : neverType));
}
DanielRosenwasser marked this conversation as resolved.
Show resolved Hide resolved

DanielRosenwasser marked this conversation as resolved.
Show resolved Hide resolved
function narrowTypeBySwitchOnTrue(type: Type, switchStatement: SwitchStatement, clauseStart: number, clauseEnd: number): Type {
const defaultIndex = findIndex(switchStatement.caseBlock.clauses, clause => clause.kind === SyntaxKind.DefaultClause);
Copy link
Member

Choose a reason for hiding this comment

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

The only time switchStatement is used is to get caseBlock which is only used for clauses. Maybe we just pass that in or destructure that out?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really know what clauseStart === clauseEnd means, but you could also avoid the findIndex by folding the walk into the first walk, and then you'd be able to avoid the range check, right?

You are basically walking across every switch/case per group of clauses (or per clause) which is N^2, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Like the other stuff, this is just me doing what the other switch case narrowings do. I can do whatever but it'll look different when it's possible that all of them could be improved.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, we should come back to these at another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, having now touched this, I do think there's a bunch to improve here and elsewhere. It's extremely likely that this just hasn't been touched in a while (when do we ever add new narrowings???)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want me to leave a TODO behind?

const hasDefaultClause = clauseStart === clauseEnd || (defaultIndex >= clauseStart && defaultIndex < clauseEnd);
jakebailey marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Reading this in the below code was confusing for me because it's not about the switch having a default, it's about the given range including a default.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just the terminology used above by the typeof narrowing. I can change it, but it will be inconsistent.


// First, narrow away all of the cases that preceded this set of cases.
for (let i = 0; i < clauseStart; i++) {
const clause = switchStatement.caseBlock.clauses[i];
if (clause.kind === SyntaxKind.CaseClause) {
type = narrowType(type, clause.expression, /*assumeTrue*/ false);
jakebailey marked this conversation as resolved.
Show resolved Hide resolved
}
}

// If our current set has a default, then none the other cases were hit either.
// There's no point in narrowing by the the other cases in the set, since we can
jakebailey marked this conversation as resolved.
Show resolved Hide resolved
// get here through other paths.
if (hasDefaultClause) {
for (let i = clauseEnd; i < switchStatement.caseBlock.clauses.length; i++) {
const clause = switchStatement.caseBlock.clauses[i];
if (clause.kind === SyntaxKind.CaseClause) {
type = narrowType(type, clause.expression, /*assumeTrue*/ false);
}
}
return type;
}

// Now, narrow based on the cases in this set.
const clauses = switchStatement.caseBlock.clauses.slice(clauseStart, clauseEnd);
Copy link
Member Author

Choose a reason for hiding this comment

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

I could theoretically try and make this directly iterate instead of slicing and mapping, if that's desirable. We already do a lot of slicing/mapping/etc in narrowing that isn't really needed, so this isn't new.

Copy link
Member Author

Choose a reason for hiding this comment

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

(see also narrowTypeBySwitchOnDiscriminant which just slices and doesn't try and be optimal)

return getUnionType(map(clauses, clause => clause.kind === SyntaxKind.CaseClause ? narrowType(type, clause.expression, /*assumeTrue*/ true) : neverType));
}

function isMatchingConstructorReference(expr: Expression) {
return (isPropertyAccessExpression(expr) && idText(expr.name) === "constructor" ||
isElementAccessExpression(expr) && isStringLiteralLike(expr.argumentExpression) && expr.argumentExpression.text === "constructor") &&
Expand Down
144 changes: 0 additions & 144 deletions tests/baselines/reference/narrowByClauseExpressionInSwitchTrue.symbols
View file
Open in desktop

This file was deleted.

144 changes: 144 additions & 0 deletions tests/baselines/reference/narrowByClauseExpressionInSwitchTrue1.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
//// [tests/cases/compiler/narrowByClauseExpressionInSwitchTrue1.ts] ////

=== narrowByClauseExpressionInSwitchTrue1.ts ===
// https://github.com/microsoft/TypeScript/issues/37178

type A = { type: "A" };
>A : Symbol(A, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 0, 0))
>type : Symbol(type, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 2, 10))

type B = { type: "B" };
>B : Symbol(B, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 2, 23))
>type : Symbol(type, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 10))

type AorB = A | B;
>AorB : Symbol(AorB, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 23))
>A : Symbol(A, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 0, 0))
>B : Symbol(B, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 2, 23))

const isA = (x: AorB): x is A => x.type === "A";
>isA : Symbol(isA, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 6, 5))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 6, 13))
>AorB : Symbol(AorB, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 23))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 6, 13))
>A : Symbol(A, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 0, 0))
>x.type : Symbol(type, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 2, 10), Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 10))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 6, 13))
>type : Symbol(type, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 2, 10), Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 10))

const isB = (x: AorB): x is B => x.type === "B";
>isB : Symbol(isB, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 7, 5))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 7, 13))
>AorB : Symbol(AorB, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 23))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 7, 13))
>B : Symbol(B, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 2, 23))
>x.type : Symbol(type, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 2, 10), Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 10))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 7, 13))
>type : Symbol(type, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 2, 10), Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 10))

function test1(x: AorB) {
>test1 : Symbol(test1, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 7, 48))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 9, 15))
>AorB : Symbol(AorB, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 23))

switch (true) {
case isA(x):
>isA : Symbol(isA, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 6, 5))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 9, 15))

x;
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 9, 15))

break;
case isB(x):
>isB : Symbol(isB, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 7, 5))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 9, 15))

x;
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 9, 15))

break;
}
}

function test2(x: AorB) {
>test2 : Symbol(test2, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 18, 1))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 20, 15))
>AorB : Symbol(AorB, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 3, 23))

switch (true) {
case isA(x):
>isA : Symbol(isA, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 6, 5))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 20, 15))

x;
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 20, 15))

// fallthrough
case isB(x):
>isB : Symbol(isB, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 7, 5))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 20, 15))

x;
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 20, 15))

break;
}
}

let x: string | undefined;
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 31, 3))

switch (true) {
case typeof x !== "undefined":
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 31, 3))

x.trim();
>x.trim : Symbol(String.trim, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 31, 3))
>trim : Symbol(String.trim, Decl(lib.es5.d.ts, --, --))
}

type SomeType = { type: "SomeType" };
>SomeType : Symbol(SomeType, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 36, 1))
>type : Symbol(type, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 38, 17))

declare function isSomeType(x: unknown): x is SomeType;
>isSomeType : Symbol(isSomeType, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 38, 37))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 39, 28))
>x : Symbol(x, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 39, 28))
>SomeType : Symbol(SomeType, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 36, 1))

function processInput(input: string | RegExp | SomeType) {
>processInput : Symbol(processInput, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 39, 55))
>input : Symbol(input, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 41, 22))
>RegExp : Symbol(RegExp, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>SomeType : Symbol(SomeType, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 36, 1))

switch (true) {
case typeof input === "string":
>input : Symbol(input, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 41, 22))

input;
>input : Symbol(input, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 41, 22))

break;
case input instanceof RegExp:
>input : Symbol(input, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 41, 22))
>RegExp : Symbol(RegExp, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))

input;
>input : Symbol(input, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 41, 22))

break;
case isSomeType(input):
>isSomeType : Symbol(isSomeType, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 38, 37))
>input : Symbol(input, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 41, 22))

input;
>input : Symbol(input, Decl(narrowByClauseExpressionInSwitchTrue1.ts, 41, 22))

break;
}
}

4 changes: 2 additions & 2 deletions ...arrowByClauseExpressionInSwitchTrue.types → ...rrowByClauseExpressionInSwitchTrue1.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//// [tests/cases/compiler/narrowByClauseExpressionInSwitchTrue.ts] ////
//// [tests/cases/compiler/narrowByClauseExpressionInSwitchTrue1.ts] ////

=== narrowByClauseExpressionInSwitchTrue.ts ===
=== narrowByClauseExpressionInSwitchTrue1.ts ===
// https://github.com/microsoft/TypeScript/issues/37178

type A = { type: "A" };
Expand Down
Loading