-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 1 commit
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
- Loading branch information
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 | ||
|
@@ -4480,6 +4477,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 +4506,19 @@ namespace ts { | |
if (source === numberType && target.flags & TypeFlags.Enum) return Ternary.True; | ||
} | ||
} | ||
|
||
if (relation === assignableRelation && source.flags & TypeFlags.ObjectLiteral && source.flags & TypeFlags.FreshObjectLiteral) { | ||
if (hasExcessProperties(<ObjectType>source, target, reportErrors)) { | ||
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. Cast to 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. Ok |
||
if (reportErrors) { | ||
reportRelationError(headMessage, source, target); | ||
} | ||
return Ternary.False; | ||
} | ||
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 +4595,28 @@ 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: ObjectType, target: Type, reportErrors: boolean): boolean { | ||
if (target.flags & TypeFlags.ObjectType) { | ||
var resolved = resolveStructuredTypeMembers(target); | ||
if (resolved.properties.length > 0 && !resolved.stringIndexType && !resolved.numberIndexType) { | ||
for (let prop of getPropertiesOfObjectType(source)) { | ||
if (!getPropertyOfType(target, prop.name)) { | ||
if (reportErrors) { | ||
reportError(Diagnostics.Property_0_does_not_exist_on_type_1, symbolToString(prop), typeToString(target)); | ||
} | ||
return true; | ||
} | ||
} | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
function eachTypeRelatedToSomeType(source: UnionOrIntersectionType, target: UnionOrIntersectionType): Ternary { | ||
let result = Ternary.True; | ||
let sourceTypes = source.types; | ||
|
@@ -5255,6 +5284,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 = {}; | ||
|
@@ -6830,18 +6877,6 @@ namespace ts { | |
return links.resolvedType; | ||
} | ||
|
||
function isPermittedProperty(contextualType: Type, propName: string): boolean { | ||
if (contextualType.flags & TypeFlags.ObjectType) { | ||
let resolved = resolveStructuredTypeMembers(<ObjectType>contextualType); | ||
return !!(resolved.properties.length === 0 || resolved.stringIndexType || | ||
resolved.numberIndexType || getPropertyOfObjectType(contextualType, propName)); | ||
} | ||
if (contextualType.flags & TypeFlags.UnionOrIntersection) { | ||
return !forEach((<UnionOrIntersectionType>contextualType).types, type => !isPermittedProperty(type, propName)); | ||
} | ||
return true; | ||
} | ||
|
||
function checkObjectLiteral(node: ObjectLiteralExpression, contextualMapper?: TypeMapper): Type { | ||
// Grammar checking | ||
checkGrammarObjectLiteralExpression(node); | ||
|
@@ -6891,17 +6926,14 @@ namespace ts { | |
|
||
if (!hasDynamicName(memberDecl)) { | ||
propertiesTable[member.name] = member; | ||
if (contextualType && !isPermittedProperty(contextualType, member.name)) { | ||
error(memberDecl.name, Diagnostics.Property_0_does_not_exist_in_contextual_type_1, member.name, typeToString(contextualType)); | ||
} | ||
} | ||
propertiesArray.push(member); | ||
} | ||
|
||
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) { | ||
|
@@ -8782,7 +8814,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); | ||
|
@@ -9559,7 +9591,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,11 @@ namespace ts { | |
numberIndexType?: Type; // Numeric index type | ||
} | ||
|
||
/* @internal */ | ||
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 +2196,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; | ||
|
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.
Why check ObjectLiteral if all FreshObjectLiterals are ObjectLiterals? In fact, why not make the FreshObjectLiteral mask include the ObjectLiteral mask?
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.
It seems strange that we only do this for assignability. Prior to this, every pair of types that return true for subtype also return true for assignability. That is no longer true, and in fact, you will observe this in overload resolution.
This will fail, but if you add a second overload, it suddenly succeeds:
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.
Yeah,
TypeFlags.FreshObjectLiteral
already implies both, so it's enough to just check for that.Agreed, we should do the check for subtype as well.