-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Strict object literal assignment checking #3823
Changes from 5 commits
cd0d3ba
11aecee
3fe7591
f57991e
aa26980
d78fa18
2eca3d5
c7b0732
c42b8f7
d34557a
1a4252d
7dbb69a
c8423d3
eeeb05b
acd8c77
a05ebc4
3cbc3db
592319d
2913cb0
155ee4b
967df39
57f1a99
5f7bc51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1966,15 +1966,12 @@ namespace ts { | |
} | ||
|
||
return _displayBuilder || (_displayBuilder = { | ||
symbolToString: symbolToString, | ||
typeToString: typeToString, | ||
buildSymbolDisplay: buildSymbolDisplay, | ||
buildTypeDisplay: buildTypeDisplay, | ||
buildTypeParameterDisplay: buildTypeParameterDisplay, | ||
buildParameterDisplay: buildParameterDisplay, | ||
buildDisplayForParametersAndDelimiters: buildDisplayForParametersAndDelimiters, | ||
buildDisplayForTypeParametersAndDelimiters: buildDisplayForTypeParametersAndDelimiters, | ||
buildDisplayForTypeArgumentsAndDelimiters: buildDisplayForTypeArgumentsAndDelimiters, | ||
buildTypeParameterDisplayFromSymbol: buildTypeParameterDisplayFromSymbol, | ||
buildSignatureDisplay: buildSignatureDisplay, | ||
buildReturnTypeDisplay: buildReturnTypeDisplay | ||
|
@@ -3355,6 +3352,29 @@ namespace ts { | |
return undefined; | ||
} | ||
|
||
// Check if a property with the given name is known anywhere in the given type. In an object | ||
// type, a property is considered known if the object type is empty, if it has any index | ||
// signatures, or if the property is actually declared in the type. In a union or intersection | ||
// type, a property is considered known if it is known in any constituent type. | ||
function isKnownProperty(type: Type, name: string): boolean { | ||
if (type.flags & TypeFlags.ObjectType) { | ||
var resolved = resolveStructuredTypeMembers(type); | ||
return !!(resolved.properties.length === 0 || | ||
resolved.stringIndexType || | ||
resolved.numberIndexType || | ||
getPropertyOfType(type, name)); | ||
} | ||
if (type.flags & TypeFlags.UnionOrIntersection) { | ||
for (let t of (<UnionOrIntersectionType>type).types) { | ||
if (isKnownProperty(t, name)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this loop is doing the right thing for intersections. For unions, I would not say that a property is known if some constituent has it. Doesn't it seem more correct to demand that all union constituents have the property? Actually, this does make sense. Really you're trying to figure out if you have heard of this property anywhere in the target. If you have, then it would not be considered excess in the source. So it's not that the target is known to have this property, it's that this property was mentioned as a potential property of the target. This also why optional property are "known" even though they might not actually be present on the target. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. |
||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
function getSignaturesOfStructuredType(type: Type, kind: SignatureKind): Signature[] { | ||
if (type.flags & TypeFlags.StructuredType) { | ||
let resolved = resolveStructuredTypeMembers(<ObjectType>type); | ||
|
@@ -4480,6 +4500,16 @@ namespace ts { | |
errorInfo = chainDiagnosticMessages(errorInfo, message, arg0, arg1, arg2); | ||
} | ||
|
||
function reportRelationError(message: DiagnosticMessage, source: Type, target: Type) { | ||
let sourceType = typeToString(source); | ||
let targetType = typeToString(target); | ||
if (sourceType === targetType) { | ||
sourceType = typeToString(source, /*enclosingDeclaration*/ undefined, TypeFormatFlags.UseFullyQualifiedType); | ||
targetType = typeToString(target, /*enclosingDeclaration*/ undefined, TypeFormatFlags.UseFullyQualifiedType); | ||
} | ||
reportError(message || Diagnostics.Type_0_is_not_assignable_to_type_1, sourceType, targetType); | ||
} | ||
|
||
// Compare two types and return | ||
// Ternary.True if they are related with no assumptions, | ||
// Ternary.Maybe if they are related with assumptions of other relationships, or | ||
|
@@ -4499,7 +4529,23 @@ namespace ts { | |
if (source === numberType && target.flags & TypeFlags.Enum) return Ternary.True; | ||
} | ||
} | ||
|
||
if (relation !== identityRelation && source.flags & TypeFlags.FreshObjectLiteral) { | ||
if (hasExcessProperties(<FreshObjectLiteralType>source, target, reportErrors)) { | ||
if (reportErrors) { | ||
reportRelationError(headMessage, source, target); | ||
} | ||
return Ternary.False; | ||
} | ||
// Above we check for excess properties with respect to the entire target type. When union | ||
// and intersection types are further deconstructed on the target side, we don't want to | ||
// make the check again (as it might fail for a partial target type). Therefore we obtain | ||
// the regular source type and proceed with that. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. This makes sense now |
||
source = getRegularTypeOfObjectLiteral(source); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it necessary to do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we otherwise will fail in cases like this: interface A { a: number }
interface B { b: number }
var x: A & B = { a: 1, b: 2 }; We make the check upfront for the entire target type, but then as we descend into the structure of the target type we no longer want to make the check again (as it would now fail). |
||
} | ||
|
||
let saveErrorInfo = errorInfo; | ||
|
||
if (source.flags & TypeFlags.Reference && target.flags & TypeFlags.Reference && (<TypeReference>source).target === (<TypeReference>target).target) { | ||
// We have type references to same target type, see if relationship holds for all type arguments | ||
if (result = typesRelatedTo((<TypeReference>source).typeArguments, (<TypeReference>target).typeArguments, reportErrors)) { | ||
|
@@ -4576,18 +4622,22 @@ namespace ts { | |
} | ||
|
||
if (reportErrors) { | ||
headMessage = headMessage || Diagnostics.Type_0_is_not_assignable_to_type_1; | ||
let sourceType = typeToString(source); | ||
let targetType = typeToString(target); | ||
if (sourceType === targetType) { | ||
sourceType = typeToString(source, /*enclosingDeclaration*/ undefined, TypeFormatFlags.UseFullyQualifiedType); | ||
targetType = typeToString(target, /*enclosingDeclaration*/ undefined, TypeFormatFlags.UseFullyQualifiedType); | ||
} | ||
reportError(headMessage, sourceType, targetType); | ||
reportRelationError(headMessage, source, target); | ||
} | ||
return Ternary.False; | ||
} | ||
|
||
function hasExcessProperties(source: FreshObjectLiteralType, target: Type, reportErrors: boolean): boolean { | ||
for (let prop of getPropertiesOfObjectType(source)) { | ||
if (!isKnownProperty(target, prop.name)) { | ||
if (reportErrors) { | ||
reportError(Diagnostics.Property_0_does_not_exist_on_type_1, symbolToString(prop), typeToString(target)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we already have the specific codepath and context here to give a better error message I'd really like to see something more specific and distinct from a normal assignability error. I fully expect to see bug reports/Stack Overflow questions on this feature if the error simply looks like any other assignability error that changes when you add/remove temps in your chain of assignments. Minimally, a unique enough message that you could put in a search engine and get a distinct StackOverflow answer. Ideally, a message that actually communicates why assignments of this form differ from a regular assignment to a sufficient degree that you don't need to switch to StackOverflow to understand the error you just made. |
||
} | ||
return true; | ||
} | ||
} | ||
} | ||
|
||
function eachTypeRelatedToSomeType(source: UnionOrIntersectionType, target: UnionOrIntersectionType): Ternary { | ||
let result = Ternary.True; | ||
let sourceTypes = source.types; | ||
|
@@ -5255,6 +5305,24 @@ namespace ts { | |
return (type.flags & TypeFlags.Tuple) && !!(<TupleType>type).elementTypes; | ||
} | ||
|
||
function getRegularTypeOfObjectLiteral(type: Type): Type { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just make this take a FreshObjectLiteralType There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the point of the method is to remove "freshness" from the type regardless of the kind of type. I suppose we could call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, that sounds fine |
||
if (type.flags & TypeFlags.FreshObjectLiteral) { | ||
let regularType = (<FreshObjectLiteralType>type).regularType; | ||
if (!regularType) { | ||
regularType = <ResolvedType>createType((<ResolvedType>type).flags & ~TypeFlags.FreshObjectLiteral); | ||
regularType.symbol = (<ResolvedType>type).symbol; | ||
regularType.members = (<ResolvedType>type).members; | ||
regularType.properties = (<ResolvedType>type).properties; | ||
regularType.callSignatures = (<ResolvedType>type).callSignatures; | ||
regularType.constructSignatures = (<ResolvedType>type).constructSignatures; | ||
regularType.stringIndexType = (<ResolvedType>type).stringIndexType; | ||
regularType.numberIndexType = (<ResolvedType>type).numberIndexType; | ||
} | ||
return regularType; | ||
} | ||
return type; | ||
} | ||
|
||
function getWidenedTypeOfObjectLiteral(type: Type): Type { | ||
let properties = getPropertiesOfObjectType(type); | ||
let members: SymbolTable = {}; | ||
|
@@ -6886,7 +6954,7 @@ namespace ts { | |
let stringIndexType = getIndexType(IndexKind.String); | ||
let numberIndexType = getIndexType(IndexKind.Number); | ||
let result = createAnonymousType(node.symbol, propertiesTable, emptyArray, emptyArray, stringIndexType, numberIndexType); | ||
result.flags |= TypeFlags.ObjectLiteral | TypeFlags.ContainsObjectLiteral | (typeFlags & TypeFlags.ContainsUndefinedOrNull); | ||
result.flags |= TypeFlags.ObjectLiteral | TypeFlags.FreshObjectLiteral | TypeFlags.ContainsObjectLiteral | (typeFlags & TypeFlags.ContainsUndefinedOrNull); | ||
return result; | ||
|
||
function getIndexType(kind: IndexKind) { | ||
|
@@ -8767,7 +8835,7 @@ namespace ts { | |
} | ||
|
||
function checkAssertion(node: AssertionExpression) { | ||
let exprType = checkExpression(node.expression); | ||
let exprType = getRegularTypeOfObjectLiteral(checkExpression(node.expression)); | ||
let targetType = getTypeFromTypeNode(node.type); | ||
if (produceDiagnostics && targetType !== unknownType) { | ||
let widenedType = getWidenedType(exprType); | ||
|
@@ -9544,7 +9612,7 @@ namespace ts { | |
return getUnionType([leftType, rightType]); | ||
case SyntaxKind.EqualsToken: | ||
checkAssignmentOperator(rightType); | ||
return rightType; | ||
return getRegularTypeOfObjectLiteral(rightType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does an assignment expression give the regular type of the right as opposed to the original type? I thought the type that gets assigned to the left is the regular type, but the type of the expression should be the original right type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this because of code similar to the following in tsserver: interface A { a: number }
interface B extends A { b: number }
var last: B;
function foo(): A {
return last = { a: 1, b: 2 };
} Once the object literal is successfully assigned to a variable it seems pedantic to insist that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My reasoning was that assigning it to Maybe we could rationalize it by saying that if we tried to assign it to Using the same A and B that you've defined: declare function foo(param: A): A;
declare function foo(param: B): B;
var last: B;
foo({a: 1, b: 2 }); // returns B
foo(last = {a: 1, b: 2 }); // returns A I don't think taking the regular type here makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. Contextual typing only comes from the first assignment target, so we're currently consistent with that. I suppose you could argue we should check against a union of all assignment targets, i.e. as long as each property is known in some assignment target we're good. But that would add a bunch of complexity that I don't think is justified. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we would need to explicitly check against the union of all assignment targets. Each assignment target does an assignability check, and if a certain source type passes all the assignability checks, it's good. So the correct behavior should just fall out if we remove the call to getRegularTypeOfObjectLiteral, no? It's true that contextual typing only comes from the first target, but I'm not sure why contextual typing is relevant. We chose to build this check into a mechanism other than contextual typing, so I don't see why we would consider contextual typing a factor here. |
||
case SyntaxKind.CommaToken: | ||
return rightType; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1743,10 +1743,12 @@ namespace ts { | |
FromSignature = 0x00040000, // Created for signature assignment check | ||
ObjectLiteral = 0x00080000, // Originates in an object literal | ||
/* @internal */ | ||
ContainsUndefinedOrNull = 0x00100000, // Type is or contains Undefined or Null type | ||
FreshObjectLiteral = 0x00100000, // Fresh object literal type | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please define fresh object literal in the comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need a new flag here? This is set in the same place as ObjectLiteral. And when you get the regular type, you turn off FreshObjectLiteral, but not ObjectLiteral, and I don't really understand why. So a regular type is allowed to have excess properties, but it still does not need to have all the optional properties of the target in a subtype check? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it's related to the issue of removing freshness after the first assignment, but still remembering that the source was an object literal. It may be that we can combine the two if we give up on the first assignment bit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, I think we should give up the first assignment bit. I think it is a strange rule. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I don't think we can get rid of the new flag. We use That said, we could still drop the assignment rule. They're really orthogonal issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, let's drop the assignment rule. I understand what you mean about So I think it is safe to remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To elaborate about the type assertion case, widening would essentially do two things:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I admit it's still possible that I missed something, but if I did, I'd like to understand it before allowing us to have two flags that sound really similar and are easy to confuse. |
||
/* @internal */ | ||
ContainsObjectLiteral = 0x00200000, // Type is or contains object literal type | ||
ESSymbol = 0x00400000, // Type of symbol primitive introduced in ES6 | ||
ContainsUndefinedOrNull = 0x00200000, // Type is or contains Undefined or Null type | ||
/* @internal */ | ||
ContainsObjectLiteral = 0x00400000, // Type is or contains object literal type | ||
ESSymbol = 0x00800000, // Type of symbol primitive introduced in ES6 | ||
|
||
/* @internal */ | ||
Intrinsic = Any | String | Number | Boolean | ESSymbol | Void | Undefined | Null, | ||
|
@@ -1839,6 +1841,14 @@ namespace ts { | |
numberIndexType?: Type; // Numeric index type | ||
} | ||
|
||
/* @internal */ | ||
// Object literals are initially marked fresh. Freshness disappears following an assignment, | ||
// before a type assertion, or when when an object literal's type is widened. The regular | ||
// version of a fresh type is identical except for the TypeFlags.FreshObjectLiteral flag. | ||
export interface FreshObjectLiteralType extends ResolvedType { | ||
regularType: ResolvedType; // Regular version of fresh type | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does regular mean not fresh? Are they opposites? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are exactly the same, except the TypeFlags.FreshObjectLiteral flag is set in the fresh version. |
||
} | ||
|
||
// Just a place to cache element types of iterables and iterators | ||
/* @internal */ | ||
export interface IterableOrIteratorType extends ObjectType, UnionType { | ||
|
@@ -2189,6 +2199,7 @@ namespace ts { | |
|
||
export interface CompilerHost { | ||
getSourceFile(fileName: string, languageVersion: ScriptTarget, onError?: (message: string) => void): SourceFile; | ||
getCancellationToken?(): CancellationToken; | ||
getDefaultLibFileName(options: CompilerOptions): string; | ||
writeFile: WriteFileCallback; | ||
getCurrentDirectory(): string; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
tests/cases/compiler/arrayLiteralTypeInference.ts(13,5): error TS2322: Type '({ id: number; trueness: boolean; } | { id: number; name: string; })[]' is not assignable to type 'Action[]'. | ||
Type '{ id: number; trueness: boolean; } | { id: number; name: string; }' is not assignable to type 'Action'. | ||
Type '{ id: number; trueness: boolean; }' is not assignable to type 'Action'. | ||
Property 'trueness' does not exist on type 'Action'. | ||
tests/cases/compiler/arrayLiteralTypeInference.ts(29,5): error TS2322: Type '({ id: number; trueness: boolean; } | { id: number; name: string; })[]' is not assignable to type '{ id: number; }[]'. | ||
Type '{ id: number; trueness: boolean; } | { id: number; name: string; }' is not assignable to type '{ id: number; }'. | ||
Type '{ id: number; trueness: boolean; }' is not assignable to type '{ id: number; }'. | ||
Property 'trueness' does not exist on type '{ id: number; }'. | ||
|
||
|
||
==== tests/cases/compiler/arrayLiteralTypeInference.ts (2 errors) ==== | ||
class Action { | ||
id: number; | ||
} | ||
|
||
class ActionA extends Action { | ||
value: string; | ||
} | ||
|
||
class ActionB extends Action { | ||
trueNess: boolean; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
var x1: Action[] = [ | ||
~~ | ||
!!! error TS2322: Type '({ id: number; trueness: boolean; } | { id: number; name: string; })[]' is not assignable to type 'Action[]'. | ||
!!! error TS2322: Type '{ id: number; trueness: boolean; } | { id: number; name: string; }' is not assignable to type 'Action'. | ||
!!! error TS2322: Type '{ id: number; trueness: boolean; }' is not assignable to type 'Action'. | ||
!!! error TS2322: Property 'trueness' does not exist on type 'Action'. | ||
{ id: 2, trueness: false }, | ||
{ id: 3, name: "three" } | ||
] | ||
|
||
var x2: Action[] = [ | ||
new ActionA(), | ||
new ActionB() | ||
] | ||
|
||
var x3: Action[] = [ | ||
new Action(), | ||
new ActionA(), | ||
new ActionB() | ||
] | ||
|
||
var z1: { id: number }[] = | ||
~~ | ||
!!! error TS2322: Type '({ id: number; trueness: boolean; } | { id: number; name: string; })[]' is not assignable to type '{ id: number; }[]'. | ||
!!! error TS2322: Type '{ id: number; trueness: boolean; } | { id: number; name: string; }' is not assignable to type '{ id: number; }'. | ||
!!! error TS2322: Type '{ id: number; trueness: boolean; }' is not assignable to type '{ id: number; }'. | ||
!!! error TS2322: Property 'trueness' does not exist on type '{ id: number; }'. | ||
[ | ||
{ id: 2, trueness: false }, | ||
{ id: 3, name: "three" } | ||
] | ||
|
||
var z2: { id: number }[] = | ||
[ | ||
new ActionA(), | ||
new ActionB() | ||
] | ||
|
||
var z3: { id: number }[] = | ||
[ | ||
new Action(), | ||
new ActionA(), | ||
new ActionB() | ||
] | ||
|
||
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so you do these checks on the target side as opposed to when contextually typing the object literal. Actually, this is probably a good thing, because not every assignability check is guaranteed to be accompanied by contextual typing. So if the object did not get contextually typed, the assignment would still be allowed, which is good I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I would add some comments explaining the rationale behind these heuristics.