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

Jump to bottom

Improve checking of in operator #50666

Merged
merged 17 commits into from
Sep 19, 2022
Merged

Improve checking of in operator #50666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
88927b5
Improve checking of `in` operator
ahejlsberg Sep 6, 2022
9ed6357
Accept new baselines
ahejlsberg Sep 6, 2022
335d7c8
Add tests
ahejlsberg Sep 7, 2022
d178ea4
Merge branch 'main' into fix21732
ahejlsberg Sep 7, 2022
5d31ba8
Delete old and accept new baselines
ahejlsberg Sep 7, 2022
498d9b5
Disallow right operand of type '{}'
ahejlsberg Sep 8, 2022
7d1564f
Accept new baselines
ahejlsberg Sep 8, 2022
4fe1ab3
Support number and symbol literals
ahejlsberg Sep 8, 2022
0db3935
Add tests
ahejlsberg Sep 8, 2022
a681878
Disallow {} typed right operand only in strictNullChecks mode
ahejlsberg Sep 9, 2022
00b32d5
Accept new baselines
ahejlsberg Sep 9, 2022
cba7203
Detect {} resulting from intersections
ahejlsberg Sep 14, 2022
1f673d6
Accept new baselines
ahejlsberg Sep 14, 2022
19f4407
Merge branch 'main' into fix21732
ahejlsberg Sep 16, 2022
3dea326
Don't attempt to reduce intersections with Record<K, unknown>
ahejlsberg Sep 17, 2022
3efc2d6
Accept new baselines
ahejlsberg Sep 17, 2022
5bc9f95
Return undefined instead of unknownSymbol from getGlobalRecordSymbol()
ahejlsberg Sep 17, 2022
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
93 changes: 44 additions & 49 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,8 @@ namespace ts {
emptyTypeLiteralSymbol.members = createSymbolTable();
const emptyTypeLiteralType = createAnonymousType(emptyTypeLiteralSymbol, emptySymbols, emptyArray, emptyArray, emptyArray);

const unknownUnionType = strictNullChecks ? getUnionType([undefinedType, nullType, createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, emptyArray)]) : unknownType;
const unknownEmptyObjectType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, emptyArray);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't there already a nonNullUnknown marker type in use somewhere that's suspiciously similar to this? Maybe it's the unknown shape rather than the {} shape? Just feels like we could reuse the marker for this, too. Cut down on potential type identities.

Copy link
Member Author

@ahejlsberg ahejlsberg Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, there is a nonNullUnknown type, but it is actually a TypeFlags.Unknown, so it can't be substituted here.

const unknownUnionType = strictNullChecks ? getUnionType([undefinedType, nullType, unknownEmptyObjectType]) : unknownType;

const emptyGenericType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, emptyArray) as ObjectType as GenericType;
emptyGenericType.instantiations = new Map<string, TypeReference>();
Expand Down Expand Up @@ -997,6 +998,7 @@ namespace ts {
let deferredGlobalOmitSymbol: Symbol | undefined;
let deferredGlobalAwaitedSymbol: Symbol | undefined;
let deferredGlobalBigIntType: ObjectType | undefined;
let deferredGlobalRecordSymbol: Symbol | undefined;

const allPotentiallyUnusedIdentifiers = new Map<Path, PotentiallyUnusedIdentifier[]>(); // key is file name

Expand Down Expand Up @@ -14308,6 +14310,11 @@ namespace ts {
return (deferredGlobalBigIntType ||= getGlobalType("BigInt" as __String, /*arity*/ 0, /*reportErrors*/ false)) || emptyObjectType;
}

function getGlobalRecordSymbol(): Symbol | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

Part of me is not entirely comfortable long-term with using Record - especially with records and tuples. But maybe I can get past it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, Record isn't going anywhere given how much usage it has in existing code.

deferredGlobalRecordSymbol ||= getGlobalTypeAliasSymbol("Record" as __String, /*arity*/ 2, /*reportErrors*/ true) || unknownSymbol;
Copy link
Member

Choose a reason for hiding this comment

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

unknownSymbol is declared with SymbolFlags.Property - is this the right thing to use?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just make this possibly return undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Elsewhere we have assumptions about unknownSymbol being SymbolFlags.Property. Plus it has to be something.

return deferredGlobalRecordSymbol === unknownSymbol ? undefined : deferredGlobalRecordSymbol;
}

/**
* Instantiates a global type that is generic with some element type, and returns that instantiation.
*/
Expand Down Expand Up @@ -25170,19 +25177,27 @@ namespace ts {

function isTypePresencePossible(type: Type, propName: __String, assumeTrue: boolean) {
const prop = getPropertyOfType(type, propName);
if (prop) {
return prop.flags & SymbolFlags.Optional ? true : assumeTrue;
}
return getApplicableIndexInfoForName(type, propName) ? true : !assumeTrue;
return prop ?
!!(prop.flags & SymbolFlags.Optional) || assumeTrue :
!!getApplicableIndexInfoForName(type, propName) || !assumeTrue;
}

function narrowByInKeyword(type: Type, name: __String, assumeTrue: boolean) {
if (type.flags & TypeFlags.Union
|| type.flags & TypeFlags.Object && declaredType !== type && !(declaredType === unknownType && isEmptyAnonymousObjectType(type))
|| isThisTypeParameter(type)
|| type.flags & TypeFlags.Intersection && every((type as IntersectionType).types, t => t.symbol !== globalThisSymbol)) {
function narrowByInKeyword(type: Type, nameType: StringLiteralType | NumberLiteralType | UniqueESSymbolType, assumeTrue: boolean) {
const name = getPropertyNameFromType(nameType);
const isKnownProperty = someType(type, t => isTypePresencePossible(t, name, /*assumeTrue*/ true));
if (isKnownProperty) {
// If the check is for a known property (i.e. a property declared in some constituent of
// the target type), we filter the target type by presence of absence of the property.
return filterType(type, t => isTypePresencePossible(t, name, assumeTrue));
}
if (assumeTrue) {
// If the check is for an unknown property, we intersect the target type with `Record<X, unknown>`,
// where X is the name of the property.
const recordSymbol = getGlobalRecordSymbol();
if (recordSymbol) {
return getIntersectionType([type, getTypeAliasInstantiation(recordSymbol, [nameType, unknownType])]);
Copy link
Member

Choose a reason for hiding this comment

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

This makes object into Record<"whatever", unknown>, but object & {} into object & {} & Record<"whatever", unknown>. Isn't the difference in behavior a bit odd? Should this just be checking if object is assignable to t, rather than the type flags?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's basically just a cosmetic check to turn object & Record<"foo", unknown> into Record<"foo", unknown>, given that the difference rarely if ever matters. I'm inclined to just remove it and leave the object & Record<"foo, unknown> alone.

}
}
return type;
}

Expand Down Expand Up @@ -25242,15 +25257,14 @@ namespace ts {
return narrowTypeByPrivateIdentifierInInExpression(type, expr, assumeTrue);
}
const target = getReferenceCandidate(expr.right);
const leftType = getTypeOfNode(expr.left);
if (leftType.flags & TypeFlags.StringLiteral) {
const name = escapeLeadingUnderscores((leftType as StringLiteralType).value);
const leftType = getTypeOfExpression(expr.left);
if (leftType.flags & TypeFlags.StringOrNumberLiteralOrUnique) {
if (containsMissingType(type) && isAccessExpression(reference) && isMatchingReference(reference.expression, target) &&
getAccessedPropertyName(reference) === name) {
getAccessedPropertyName(reference) === getPropertyNameFromType(leftType as StringLiteralType | NumberLiteralType | UniqueESSymbolType)) {
return getTypeWithFacts(type, assumeTrue ? TypeFacts.NEUndefined : TypeFacts.EQUndefined);
}
if (isMatchingReference(reference, target)) {
return narrowByInKeyword(type, name, assumeTrue);
return narrowByInKeyword(type, leftType as StringLiteralType | NumberLiteralType | UniqueESSymbolType, assumeTrue);
}
}
break;
Expand Down Expand Up @@ -33818,6 +33832,10 @@ namespace ts {
return booleanType;
}

function hasEmptyObjectIntersection(type: Type): boolean {
return someType(type, t => t === unknownEmptyObjectType || !!(t.flags & TypeFlags.Intersection) && some((t as IntersectionType).types, isEmptyAnonymousObjectType));
}

function checkInExpression(left: Expression, right: Expression, leftType: Type, rightType: Type): Type {
if (leftType === silentNeverType || rightType === silentNeverType) {
return silentNeverType;
Expand All @@ -33834,43 +33852,20 @@ namespace ts {
}
}
else {
leftType = checkNonNullType(leftType, left);
// TypeScript 1.0 spec (April 2014): 4.15.5
// Require the left operand to be of type Any, the String primitive type, or the Number primitive type.
if (!(allTypesAssignableToKind(leftType, TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.ESSymbolLike) ||
isTypeAssignableToKind(leftType, TypeFlags.Index | TypeFlags.TemplateLiteral | TypeFlags.StringMapping | TypeFlags.TypeParameter))) {
error(left, Diagnostics.The_left_hand_side_of_an_in_expression_must_be_a_private_identifier_or_of_type_any_string_number_or_symbol);
// The type of the lef operand must be assignable to string, number, or symbol.
checkTypeAssignableTo(checkNonNullType(leftType, left), stringNumberSymbolType, left);
}
// The type of the right operand must be assignable to 'object'.
if (checkTypeAssignableTo(checkNonNullType(rightType, right), nonPrimitiveType, right)) {
// The {} type is assignable to the object type, yet {} might represent a primitive type. Here we
// detect and error on {} that results from narrowing the unknown type, as well as intersections
// that include {} (we know that the other types in such intersections are assignable to object
// since we already checked for that).
if (hasEmptyObjectIntersection(rightType)) {
error(right, Diagnostics.Type_0_may_represent_a_primitive_value_which_is_not_permitted_as_the_right_operand_of_the_in_operator, typeToString(rightType));
}
}
rightType = checkNonNullType(rightType, right);
// TypeScript 1.0 spec (April 2014): 4.15.5
// The in operator requires the right operand to be
//
// 1. assignable to the non-primitive type,
// 2. an unconstrained type parameter,
// 3. a union or intersection including one or more type parameters, whose constituents are all assignable to the
// the non-primitive type, or are unconstrainted type parameters, or have constraints assignable to the
// non-primitive type, or
// 4. a type parameter whose constraint is
// i. an object type,
// ii. the non-primitive type, or
// iii. a union or intersection with at least one constituent assignable to an object or non-primitive type.
//
// The divergent behavior for type parameters and unions containing type parameters is a workaround for type
// parameters not being narrowable. If the right operand is a concrete type, we can error if there is any chance
// it is a primitive. But if the operand is a type parameter, it cannot be narrowed, so we don't issue an error
// unless *all* instantiations would result in an error.
//
// The result is always of the Boolean primitive type.
const rightTypeConstraint = getConstraintOfType(rightType);
if (!allTypesAssignableToKind(rightType, TypeFlags.NonPrimitive | TypeFlags.InstantiableNonPrimitive) ||
rightTypeConstraint && (
isTypeAssignableToKind(rightType, TypeFlags.UnionOrIntersection) && !allTypesAssignableToKind(rightTypeConstraint, TypeFlags.NonPrimitive | TypeFlags.InstantiableNonPrimitive) ||
!maybeTypeOfKind(rightTypeConstraint, TypeFlags.NonPrimitive | TypeFlags.InstantiableNonPrimitive | TypeFlags.Object)
)
) {
error(right, Diagnostics.The_right_hand_side_of_an_in_expression_must_not_be_a_primitive);
}
return booleanType;
}

Expand Down
12 changes: 4 additions & 8 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1844,14 +1844,6 @@
"category": "Error",
"code": 2359
},
"The left-hand side of an 'in' expression must be a private identifier or of type 'any', 'string', 'number', or 'symbol'.": {
"category": "Error",
"code": 2360
},
"The right-hand side of an 'in' expression must not be a primitive.": {
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 losing these error messages is actually worse.

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 disagree. We'd tell you it can't be a primitive, but we wouldn't tell you why it could be. There was never any elaboration. Now there is. The fact that the error applies to the right hand of an in expression is obvious from context so it just seems like useless information.

"category": "Error",
"code": 2361
},
"The left-hand side of an arithmetic operation must be of type 'any', 'number', 'bigint' or an enum type.": {
"category": "Error",
"code": 2362
Expand Down Expand Up @@ -2845,6 +2837,10 @@
"category": "Error",
"code": 2637
},
"Type '{0}' may represent a primitive value, which is not permitted as the right operand of the 'in' operator.": {
"category": "Error",
"code": 2638
},

"Cannot augment module '{0}' with value exports because it resolves to a non-module entity.": {
"category": "Error",
Expand Down
Loading