Skip to content

consistently check expressions with >, >=, <, <= for unconstrained types in strictNullChecks #59352

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

consistently check expressions with >, >=, <, <= for unconstrained types in strictNullChecks #59352

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee33225
add check for >, >=, <, <= for unconstrained types
iisaduan Jul 19, 2024
fca2ca0
fmt
iisaduan Jul 19, 2024
1fa487e
change error to `possibly null or undefined`
iisaduan Jul 19, 2024
0308dde
add intersections
iisaduan Jul 22, 2024
43c24bb
add tests, fix comment
iisaduan Jul 22, 2024
529331b
format
iisaduan Jul 22, 2024
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
9 changes: 7 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27493,6 +27493,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return type === unknownUnionType ? unknownType : type;
}

// use to determine if a parameter may be undefined or null (or is unknown/unconstrained)
function getUnknownIfMaybeUnknown(type: Type) {
return (strictNullChecks && type.flags & TypeFlags.Instantiable) ? getBaseConstraintOfType(type) || unknownUnionType : type;
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming from #59059 that making this change to getTypeFacts breaks a lot of existing code, is that right?

Also, does making this change to checkNonNullType instead also break a lot of code?

Copy link
Member Author

@iisaduan iisaduan Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I tried that and putting this change directly into checkNonNullType gets a very similar result to #59059 in terms of breaking object index checking

Copy link
Member Author

Choose a reason for hiding this comment

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

A lot of the cases that break relate to this type of index access #59059 (comment)

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 know if this it's noticeably different in semantics, but istead of doing this, you might want to try to modify getBaseTypeOfLiteralTypeForComparison and rename it getBaseTypeForComparison. Then pass the result of that into checkNonNullType.

Copy link
Member

Choose a reason for hiding this comment

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

I would also consider renaming this to getBaseConstraintOrUnknown, use ?? instead of ||, and just get rid of the strictNullChecks check.

Copy link
Member

Choose a reason for hiding this comment

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

Also - any clue if this works on unions of unconstrained type parameters? Can you add a test?

function f<T, U>(x: T | U, y: T | U) {
  return x < y;
}

}

function getTypeWithDefault(type: Type, defaultExpression: Expression) {
return defaultExpression ?
getUnionType([getNonUndefinedType(type), getTypeOfExpression(defaultExpression)]) :
Expand Down Expand Up @@ -39677,8 +39682,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
case SyntaxKind.LessThanEqualsToken:
case SyntaxKind.GreaterThanEqualsToken:
if (checkForDisallowedESSymbolOperand(operator)) {
leftType = getBaseTypeOfLiteralTypeForComparison(checkNonNullType(leftType, left));
rightType = getBaseTypeOfLiteralTypeForComparison(checkNonNullType(rightType, right));
leftType = getBaseTypeOfLiteralTypeForComparison(checkNonNullType(getUnknownIfMaybeUnknown(leftType), left));
rightType = getBaseTypeOfLiteralTypeForComparison(checkNonNullType(getUnknownIfMaybeUnknown(rightType), right));
reportOperatorErrorUnless((left, right) => {
if (isTypeAny(left) || isTypeAny(right)) {
return true;
Expand Down
109 changes: 109 additions & 0 deletions tests/baselines/reference/unconstrainedTypeComparison.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
unconstrainedTypeComparison.ts(2,12): error TS18049: 'a' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(2,16): error TS18049: 'b' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(6,12): error TS18049: 'a' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(6,16): error TS18049: 'b' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(10,12): error TS18046: 'a' is of type 'unknown'.
unconstrainedTypeComparison.ts(10,16): error TS18046: 'b' is of type 'unknown'.
unconstrainedTypeComparison.ts(14,12): error TS18049: 'a' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(14,16): error TS18049: 'b' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(18,12): error TS18049: 'a' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(18,16): error TS18049: 'b' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(22,12): error TS18046: 'a' is of type 'unknown'.
unconstrainedTypeComparison.ts(22,16): error TS18046: 'b' is of type 'unknown'.
unconstrainedTypeComparison.ts(26,12): error TS18048: 'a' is possibly 'undefined'.
unconstrainedTypeComparison.ts(26,16): error TS18048: 'b' is possibly 'undefined'.
unconstrainedTypeComparison.ts(30,12): error TS18047: 'a' is possibly 'null'.
unconstrainedTypeComparison.ts(30,16): error TS18047: 'b' is possibly 'null'.
unconstrainedTypeComparison.ts(34,12): error TS18049: 'a' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(34,16): error TS18049: 'b' is possibly 'null' or 'undefined'.
unconstrainedTypeComparison.ts(45,12): error TS18047: 'a' is possibly 'null'.
unconstrainedTypeComparison.ts(45,16): error TS18048: 'b' is possibly 'undefined'.


==== unconstrainedTypeComparison.ts (20 errors) ====
function f1<T>(a: T, b: T): boolean {
return a > b;
~
!!! error TS18049: 'a' is possibly 'null' or 'undefined'.
~
!!! error TS18049: 'b' is possibly 'null' or 'undefined'.
}

function f2<T extends {} | undefined | null>(a: T, b: T): boolean {
return a > b;
~
!!! error TS18049: 'a' is possibly 'null' or 'undefined'.
~
!!! error TS18049: 'b' is possibly 'null' or 'undefined'.
}

function f3<T extends unknown>(a: T, b: T): boolean {
return a > b;
~
!!! error TS18046: 'a' is of type 'unknown'.
~
!!! error TS18046: 'b' is of type 'unknown'.
}

function f4<T, U extends T>(a: U, b: U): boolean {
return a > b;
~
!!! error TS18049: 'a' is possibly 'null' or 'undefined'.
~
!!! error TS18049: 'b' is possibly 'null' or 'undefined'.
}

function f5<T extends {} | undefined | null, U extends T>(a: U, b: U): boolean {
return a > b;
~
!!! error TS18049: 'a' is possibly 'null' or 'undefined'.
~
!!! error TS18049: 'b' is possibly 'null' or 'undefined'.
}

function f6<T extends unknown, U extends T>(a: U, b: U): boolean {
return a > b;
~
!!! error TS18046: 'a' is of type 'unknown'.
~
!!! error TS18046: 'b' is of type 'unknown'.
}

function f7<T extends {} | undefined, U extends T>(a: U, b: U): boolean {
return a > b;
~
!!! error TS18048: 'a' is possibly 'undefined'.
~
!!! error TS18048: 'b' is possibly 'undefined'.
}

function f8<T extends {} | null, U extends T>(a: U, b: U): boolean {
return a > b;
~
!!! error TS18047: 'a' is possibly 'null'.
~
!!! error TS18047: 'b' is possibly 'null'.
}

function f9<T extends undefined | null, U extends T>(a: U, b: U): boolean {
return a > b;
~
!!! error TS18049: 'a' is possibly 'null' or 'undefined'.
~
!!! error TS18049: 'b' is possibly 'null' or 'undefined'.
}


function compare<T>(a: T, b: T): boolean {
if (a === undefined) {
return false;
}
if (b === null) {
return false;
}
return a > b;
~
!!! error TS18047: 'a' is possibly 'null'.
~
!!! error TS18048: 'b' is possibly 'undefined'.
}
156 changes: 156 additions & 0 deletions tests/baselines/reference/unconstrainedTypeComparison.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
//// [tests/cases/compiler/unconstrainedTypeComparison.ts] ////

=== unconstrainedTypeComparison.ts ===
function f1<T>(a: T, b: T): boolean {
>f1 : Symbol(f1, Decl(unconstrainedTypeComparison.ts, 0, 0))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 0, 12))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 0, 15))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 0, 12))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 0, 20))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 0, 12))

return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 0, 15))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 0, 20))
}

function f2<T extends {} | undefined | null>(a: T, b: T): boolean {
>f2 : Symbol(f2, Decl(unconstrainedTypeComparison.ts, 2, 1))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 4, 12))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 4, 45))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 4, 12))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 4, 50))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 4, 12))

return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 4, 45))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 4, 50))
}

function f3<T extends unknown>(a: T, b: T): boolean {
>f3 : Symbol(f3, Decl(unconstrainedTypeComparison.ts, 6, 1))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 8, 12))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 8, 31))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 8, 12))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 8, 36))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 8, 12))

return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 8, 31))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 8, 36))
}

function f4<T, U extends T>(a: U, b: U): boolean {
>f4 : Symbol(f4, Decl(unconstrainedTypeComparison.ts, 10, 1))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 12, 12))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 12, 14))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 12, 12))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 12, 28))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 12, 14))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 12, 33))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 12, 14))

return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 12, 28))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 12, 33))
}

function f5<T extends {} | undefined | null, U extends T>(a: U, b: U): boolean {
>f5 : Symbol(f5, Decl(unconstrainedTypeComparison.ts, 14, 1))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 16, 12))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 16, 44))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 16, 12))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 16, 58))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 16, 44))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 16, 63))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 16, 44))

return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 16, 58))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 16, 63))
}

function f6<T extends unknown, U extends T>(a: U, b: U): boolean {
>f6 : Symbol(f6, Decl(unconstrainedTypeComparison.ts, 18, 1))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 20, 12))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 20, 30))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 20, 12))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 20, 44))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 20, 30))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 20, 49))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 20, 30))

return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 20, 44))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 20, 49))
}

function f7<T extends {} | undefined, U extends T>(a: U, b: U): boolean {
>f7 : Symbol(f7, Decl(unconstrainedTypeComparison.ts, 22, 1))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 24, 12))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 24, 37))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 24, 12))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 24, 51))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 24, 37))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 24, 56))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 24, 37))

return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 24, 51))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 24, 56))
}

function f8<T extends {} | null, U extends T>(a: U, b: U): boolean {
>f8 : Symbol(f8, Decl(unconstrainedTypeComparison.ts, 26, 1))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 28, 12))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 28, 32))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 28, 12))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 28, 46))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 28, 32))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 28, 51))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 28, 32))

return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 28, 46))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 28, 51))
}

function f9<T extends undefined | null, U extends T>(a: U, b: U): boolean {
>f9 : Symbol(f9, Decl(unconstrainedTypeComparison.ts, 30, 1))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 32, 12))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 32, 39))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 32, 12))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 32, 53))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 32, 39))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 32, 58))
>U : Symbol(U, Decl(unconstrainedTypeComparison.ts, 32, 39))

return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 32, 53))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 32, 58))
}


function compare<T>(a: T, b: T): boolean {
>compare : Symbol(compare, Decl(unconstrainedTypeComparison.ts, 34, 1))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 37, 17))
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 37, 20))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 37, 17))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 37, 25))
>T : Symbol(T, Decl(unconstrainedTypeComparison.ts, 37, 17))

if (a === undefined) {
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 37, 20))
>undefined : Symbol(undefined)

return false;
}
if (b === null) {
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 37, 25))

return false;
}
return a > b;
>a : Symbol(a, Decl(unconstrainedTypeComparison.ts, 37, 20))
>b : Symbol(b, Decl(unconstrainedTypeComparison.ts, 37, 25))
}
Loading