-
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 all 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
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1762,10 +1762,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 | ||
/* @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, | ||
|
@@ -1858,6 +1860,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 { | ||
|
@@ -2211,6 +2221,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 |
---|---|---|
@@ -1,8 +1,8 @@ | ||
tests/cases/conformance/statements/for-ofStatements/ES5For-ofTypeCheck9.ts(2,15): error TS2461: Type 'number | symbol | string[]' is not an array type. | ||
tests/cases/conformance/statements/for-ofStatements/ES5For-ofTypeCheck9.ts(2,15): error TS2461: Type 'string[] | number | symbol' is not an array type. | ||
|
||
|
||
==== tests/cases/conformance/statements/for-ofStatements/ES5For-ofTypeCheck9.ts (1 errors) ==== | ||
var union: string | string[] | number | symbol; | ||
for (let v of union) { } | ||
~~~~~ | ||
!!! error TS2461: Type 'number | symbol | string[]' is not an array type. | ||
!!! error TS2461: Type 'string[] | number | symbol' is not an array type. |
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.
Please define fresh object literal in the comment.
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 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 comment
The 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 comment
The 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 comment
The 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
TypeFlag.ObjectLiteral
to indicate that a type originated in an object literal and may contain null or undefined types. We can't turn off that flag unless we also widen the type. Yet, in type assertions and during subtype reduction we want to turn off freshness but not widen.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 comment
The 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
TypeFlags.ObjectLiteral
, but I still think we can use it. We actually useContainsUndefinedOrNull
andContainsObjectLiteral
for what you are talking about, not ObjectLiteral directly. For type assertions, I think it's fine to widen, we already do in the downcast direction, and I think it's fine to do it for the upcast (it uses assignability so we should be fine). For creation of a union type, we already give itTypeFlags.Union
plus all the widening flags (which do not include ObjectLiteral anyway). So all the flags that getWidenedType checks for would still be there.So I think it is safe to remove it.
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.
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 comment
The 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.