Skip to content

Track non-null unknown types in control flow analysis #45575

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.

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 2 commits into from
Sep 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 13 additions & 20 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,7 @@ namespace ts {
const nonInferrableAnyType = createIntrinsicType(TypeFlags.Any, "any", ObjectFlags.ContainsWideningType);
const intrinsicMarkerType = createIntrinsicType(TypeFlags.Any, "intrinsic");
const unknownType = createIntrinsicType(TypeFlags.Unknown, "unknown");
const nonNullUnknownType = createIntrinsicType(TypeFlags.Unknown, "unknown");
Copy link
Member

Choose a reason for hiding this comment

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

It's already pretty hard to understand what these are for. I'd encourage us to start documenting what the use-cases for each of these are. For example.

Suggested change
const nonNullUnknownType = createIntrinsicType(TypeFlags.Unknown, "unknown");
/**
* An `unknown` type that is used purely for narrowing.
* Used to record that a `x !== null` check has occurred to specially handle `typeof x === "object"`.
*/
const nonNullUnknownType = createIntrinsicType(TypeFlags.Unknown, "unknown");

const undefinedType = createIntrinsicType(TypeFlags.Undefined, "undefined");
const undefinedWideningType = strictNullChecks ? undefinedType : createIntrinsicType(TypeFlags.Undefined, "undefined", ObjectFlags.ContainsWideningType);
const optionalType = createIntrinsicType(TypeFlags.Undefined, "undefined");
Expand Down Expand Up @@ -13995,7 +13996,9 @@ namespace ts {
const includes = addTypesToUnion(typeSet, 0, types);
if (unionReduction !== UnionReduction.None) {
if (includes & TypeFlags.AnyOrUnknown) {
return includes & TypeFlags.Any ? includes & TypeFlags.IncludesWildcard ? wildcardType : anyType : unknownType;
return includes & TypeFlags.Any ?
includes & TypeFlags.IncludesWildcard ? wildcardType : anyType :
includes & TypeFlags.Null || containsType(typeSet, unknownType) ? unknownType : nonNullUnknownType;
Copy link
Member

Choose a reason for hiding this comment

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

What’s an example where this produces nonNullUnknownType? Would nonNullUnknownType itself have to be one of the union constituents?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it'll produce nonNullUnknownType when there are no regular unknownType instances in the set (which implies all are nonNullUnknownType instances) and the set contains no TypeFlags.Null types. Effectively, a non-null unknown type survives only if there are no regular unknown types and no null types.

}
if (exactOptionalPropertyTypes && includes & TypeFlags.Undefined) {
const missingIndex = binarySearch(typeSet, missingType, getTypeId, compareValues);
Expand Down Expand Up @@ -22100,13 +22103,6 @@ namespace ts {
return false;
}

// Given a source x, check if target matches x or is an && operation with an operand that matches x.
function containsTruthyCheck(source: Node, target: Node): boolean {
return isMatchingReference(source, target) ||
(target.kind === SyntaxKind.BinaryExpression && (target as BinaryExpression).operatorToken.kind === SyntaxKind.AmpersandAmpersandToken &&
(containsTruthyCheck(source, (target as BinaryExpression).left) || containsTruthyCheck(source, (target as BinaryExpression).right)));
}

function getPropertyAccess(expr: Expression) {
if (isAccessExpression(expr)) {
return expr;
Expand Down Expand Up @@ -23119,7 +23115,8 @@ namespace ts {
if (resultType === unreachableNeverType || reference.parent && reference.parent.kind === SyntaxKind.NonNullExpression && !(resultType.flags & TypeFlags.Never) && getTypeWithFacts(resultType, TypeFacts.NEUndefinedOrNull).flags & TypeFlags.Never) {
return declaredType;
}
return resultType;
// The non-null unknown type should never escape control flow analysis.
return resultType === nonNullUnknownType ? unknownType : resultType;

function getOrSetCacheKey() {
if (isKeySet) {
Expand Down Expand Up @@ -23607,7 +23604,8 @@ namespace ts {

function narrowTypeByTruthiness(type: Type, expr: Expression, assumeTrue: boolean): Type {
if (isMatchingReference(reference, expr)) {
return getTypeWithFacts(type, assumeTrue ? TypeFacts.Truthy : TypeFacts.Falsy);
return type.flags & TypeFlags.Unknown && assumeTrue ? nonNullUnknownType :
getTypeWithFacts(type, assumeTrue ? TypeFacts.Truthy : TypeFacts.Falsy);
}
if (strictNullChecks && assumeTrue && optionalChainContainsReference(expr, reference)) {
type = getTypeWithFacts(type, TypeFacts.NEUndefinedOrNull);
Expand Down Expand Up @@ -23765,7 +23763,7 @@ namespace ts {
valueType.flags & TypeFlags.Null ?
assumeTrue ? TypeFacts.EQNull : TypeFacts.NENull :
assumeTrue ? TypeFacts.EQUndefined : TypeFacts.NEUndefined;
return getTypeWithFacts(type, facts);
return type.flags & TypeFlags.Unknown && facts & (TypeFacts.NENull | TypeFacts.NEUndefinedOrNull) ? nonNullUnknownType : getTypeWithFacts(type, facts);
Copy link
Member

Choose a reason for hiding this comment

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

Can this not just be handled by getTypeWithFacts?

Copy link
Contributor

@ExE-Boss ExE-Boss Aug 26, 2021

Choose a reason for hiding this comment

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

Well, getTypeWithFacts(…) is used in many more places, and nonNullUnknownType should never escape control flow analysis: https://github.com/microsoft/TypeScript/blob/7e231a2ebfce3692663fe2583a19a8cb0c59e457/src/compiler/checker.ts#L23118-L23119

So probably not?

Copy link
Member

Choose a reason for hiding this comment

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

}
if (assumeTrue) {
const filterFn: (t: Type) => boolean = operator === SyntaxKind.EqualsEqualsToken ?
Expand Down Expand Up @@ -23795,15 +23793,10 @@ namespace ts {
return type;
}
if (assumeTrue && type.flags & TypeFlags.Unknown && literal.text === "object") {
// The pattern x && typeof x === 'object', where x is of type unknown, narrows x to type object. We don't
// need to check for the reverse typeof x === 'object' && x since that already narrows correctly.
if (typeOfExpr.parent.parent.kind === SyntaxKind.BinaryExpression) {
const expr = typeOfExpr.parent.parent as BinaryExpression;
if (expr.operatorToken.kind === SyntaxKind.AmpersandAmpersandToken && expr.right === typeOfExpr.parent && containsTruthyCheck(reference, expr.left)) {
return nonPrimitiveType;
}
}
return getUnionType([nonPrimitiveType, nullType]);
// The non-null unknown type is used to track whether a previous narrowing operation has removed the null type
// from the unknown type. For example, the expression `x && typeof x === 'object'` first narrows x to the non-null
// unknown type, and then narrows that to the non-primitive type.
return type === nonNullUnknownType ? nonPrimitiveType : getUnionType([nonPrimitiveType, nullType]);
}
const facts = assumeTrue ?
typeofEQFacts.get(literal.text) || TypeFacts.TypeofEQHostObject :
Expand Down
77 changes: 77 additions & 0 deletions tests/baselines/reference/controlFlowTypeofObject.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
tests/cases/conformance/controlFlow/controlFlowTypeofObject.ts(66,13): error TS2345: Argument of type 'object | null' is not assignable to parameter of type 'object'.
Type 'null' is not assignable to type 'object'.


==== tests/cases/conformance/controlFlow/controlFlowTypeofObject.ts (1 errors) ====
declare function obj(x: object): void;

function f1(x: unknown) {
if (!x) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}

function f2(x: unknown) {
if (x === null) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}

function f3(x: unknown) {
if (x == null) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}

function f4(x: unknown) {
if (x == undefined) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}

function f5(x: unknown) {
if (!!true) {
if (!x) {
return;
}
}
else {
if (x === null) {
return;
}
}
if (typeof x === 'object') {
obj(x);
}
}

function f6(x: unknown) {
if (x === null) {
x;
}
else {
x;
if (typeof x === 'object') {
obj(x);
}
}
if (typeof x === 'object') {
obj(x); // Error
~
!!! error TS2345: Argument of type 'object | null' is not assignable to parameter of type 'object'.
!!! error TS2345: Type 'null' is not assignable to type 'object'.
}
}

144 changes: 144 additions & 0 deletions tests/baselines/reference/controlFlowTypeofObject.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
//// [controlFlowTypeofObject.ts]
declare function obj(x: object): void;

function f1(x: unknown) {
if (!x) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}

function f2(x: unknown) {
if (x === null) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}

function f3(x: unknown) {
if (x == null) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}

function f4(x: unknown) {
if (x == undefined) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}

function f5(x: unknown) {
if (!!true) {
if (!x) {
return;
}
}
else {
if (x === null) {
return;
}
}
if (typeof x === 'object') {
obj(x);
}
}

function f6(x: unknown) {
if (x === null) {
x;
}
else {
x;
if (typeof x === 'object') {
obj(x);
}
}
if (typeof x === 'object') {
obj(x); // Error
}
}


//// [controlFlowTypeofObject.js]
"use strict";
function f1(x) {
if (!x) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}
function f2(x) {
if (x === null) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}
function f3(x) {
if (x == null) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}
function f4(x) {
if (x == undefined) {
return;
}
if (typeof x === 'object') {
obj(x);
}
}
function f5(x) {
if (!!true) {
if (!x) {
return;
}
}
else {
if (x === null) {
return;
}
}
if (typeof x === 'object') {
obj(x);
}
}
function f6(x) {
if (x === null) {
x;
}
else {
x;
if (typeof x === 'object') {
obj(x);
}
}
if (typeof x === 'object') {
obj(x); // Error
}
}


//// [controlFlowTypeofObject.d.ts]
declare function obj(x: object): void;
declare function f1(x: unknown): void;
declare function f2(x: unknown): void;
declare function f3(x: unknown): void;
declare function f4(x: unknown): void;
declare function f5(x: unknown): void;
declare function f6(x: unknown): void;
Loading