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.

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

Weak type detection #16047

Merged
merged 20 commits into from
Jun 2, 2017
Merged
Show file tree
Hide file tree
Changes from 17 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
69 changes: 59 additions & 10 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2285,7 +2285,7 @@ namespace ts {
Debug.assert(typeNode !== undefined, "should always get typenode?");
const options = { removeComments: true };
const writer = createTextWriter("");
const printer = createPrinter(options, writer);
const printer = createPrinter(options);
const sourceFile = enclosingDeclaration && getSourceFileOfNode(enclosingDeclaration);
printer.writeNode(EmitHint.Unspecified, typeNode, /*sourceFile*/ sourceFile, writer);
const result = writer.getText();
Expand Down Expand Up @@ -8701,6 +8701,7 @@ namespace ts {
let expandingFlags: number;
let depth = 0;
let overflow = false;
let disableWeakTypeErrors = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

talk offline :: this should be renamed 🌵


Debug.assert(relation !== identityRelation || !errorNode, "no error reporting in identity checking");

Expand Down Expand Up @@ -8977,17 +8978,35 @@ namespace ts {
}
}

function typeRelatedToEachType(source: Type, target: UnionOrIntersectionType, reportErrors: boolean): Ternary {
function typeRelatedToEachType(source: Type, target: IntersectionType, reportErrors: boolean): Ternary {
let result = Ternary.True;
const targetTypes = target.types;
const saveDisableWeakTypeErrors = disableWeakTypeErrors;
disableWeakTypeErrors = true;
for (const targetType of targetTypes) {
const related = isRelatedTo(source, targetType, reportErrors);
if (!related) {
disableWeakTypeErrors = saveDisableWeakTypeErrors;
return Ternary.False;
}
result &= related;
}
return result;
disableWeakTypeErrors = saveDisableWeakTypeErrors;
return reportAssignmentToWeakIntersection(source, target, reportErrors) ? Ternary.False : result;
Copy link
Member

@DanielRosenwasser DanielRosenwasser May 25, 2017

Choose a reason for hiding this comment

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

Comment on what you're doing here and below - the intent is obvious now, but it won't be to a random team member in 3 months. 😄

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 refactored this into a separate function and a named boolean. I think the two new names should serve as adequate documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I may miss something here ->why would we call "reportAssignmentToWeakIntersection" without checking first that disableWeakTypeErrors is false

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 assume that you always want to check the combined intersection type. Put another way, I assume that the only reason that disableWeakTypeErrors would be false is to prevent checking of individual intersection constituents, which doesn't apply to reportAssignmentToWeakIntersection.

tl;dr It doesn't need to check disableWeakTypeErrors because it doesn't apply.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oic. Jsut to make sure I understand this correctly because reportAaassignmentToWeakIntersection does the check in line 8999 and 9000 -> checking that all constituents are weak type ? .....would you mind add this comment in? it wasn't obvious jsut from looking at the caller

}

function reportAssignmentToWeakIntersection(source: Type, target: IntersectionType, reportErrors: boolean) {
const needsWeakTypeCheck = source !== globalObjectType && getPropertiesOfType(source).length > 0 && every(target.types, isWeakType);
if (!needsWeakTypeCheck) {
return false;
}
const hasSharedProperty = forEach(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please add comment on why you don't use isrelatedTo

getPropertiesOfType(source),
p => isKnownProperty(target, p.name, /*isComparingJsxAttributes*/ false));
if (!hasSharedProperty && reportErrors) {
reportError(Diagnostics.Weak_type_0_has_no_properties_in_common_with_1, typeToString(target), typeToString(source));
}
return !hasSharedProperty;
}

function someTypeRelatedToType(source: UnionOrIntersectionType, target: Type, reportErrors: boolean): Ternary {
Expand Down Expand Up @@ -9277,8 +9296,12 @@ namespace ts {
let result = Ternary.True;
const properties = getPropertiesOfObjectType(target);
const requireOptionalProperties = relation === subtypeRelation && !(getObjectFlags(source) & ObjectFlags.ObjectLiteral);
let foundMatchingProperty = !isWeakType(target);
for (const targetProp of properties) {
const sourceProp = getPropertyOfType(source, targetProp.name);
if (sourceProp) {
foundMatchingProperty = true;
}

if (sourceProp !== targetProp) {
if (!sourceProp) {
Expand Down Expand Up @@ -9329,7 +9352,10 @@ namespace ts {
}
return Ternary.False;
}
const saveDisableWeakTypeErrors = disableWeakTypeErrors;
disableWeakTypeErrors = false;
const related = isRelatedTo(getTypeOfSymbol(sourceProp), getTypeOfSymbol(targetProp), reportErrors);
disableWeakTypeErrors = saveDisableWeakTypeErrors;
if (!related) {
if (reportErrors) {
reportError(Diagnostics.Types_of_property_0_are_incompatible, symbolToString(targetProp));
Expand All @@ -9355,9 +9381,30 @@ namespace ts {
}
}
}
if (!foundMatchingProperty && !disableWeakTypeErrors && source !== globalObjectType && getPropertiesOfType(source).length > 0) {
if (reportErrors) {
reportError(Diagnostics.Weak_type_0_has_no_properties_in_common_with_1, typeToString(target), typeToString(source));
}
return Ternary.False;
}
return result;
}

/**
* A type is 'weak' if it is an object type with at least one optional property
* and no required properties, call/construct signatures or index signatures
*/
function isWeakType(type: Type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could you rename as isWeakType?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

const props = getPropertiesOfType(type);
return type.flags & TypeFlags.Object &&
props.length > 0 &&
every(props, p => !!(p.flags & SymbolFlags.Optional)) &&
!getSignaturesOfType(type, SignatureKind.Call).length &&
!getSignaturesOfType(type, SignatureKind.Construct).length &&
!getIndexTypeOfType(type, IndexKind.String) &&
!getIndexTypeOfType(type, IndexKind.Number);
}

function propertiesIdenticalTo(source: Type, target: Type): Ternary {
if (!(source.flags & TypeFlags.Object && target.flags & TypeFlags.Object)) {
return Ternary.False;
Expand Down Expand Up @@ -14121,8 +14168,10 @@ namespace ts {
function isKnownProperty(targetType: Type, name: string, isComparingJsxAttributes: boolean): boolean {
if (targetType.flags & TypeFlags.Object) {
const resolved = resolveStructuredTypeMembers(<ObjectType>targetType);
if (resolved.stringIndexInfo || resolved.numberIndexInfo && isNumericLiteralName(name) ||
getPropertyOfType(targetType, name) || isComparingJsxAttributes && !isUnhyphenatedJsxName(name)) {
if (resolved.stringIndexInfo ||
resolved.numberIndexInfo && isNumericLiteralName(name) ||
getPropertyOfType(targetType, name) ||
isComparingJsxAttributes && !isUnhyphenatedJsxName(name)) {
// For JSXAttributes, if the attribute has a hyphenated name, consider that the attribute to be known.
return true;
}
Expand Down Expand Up @@ -22671,12 +22720,12 @@ namespace ts {
return symbols;
}
else if (symbol.flags & SymbolFlags.Transient) {
if ((symbol as SymbolLinks).leftSpread) {
const links = symbol as SymbolLinks;
return [...getRootSymbols(links.leftSpread), ...getRootSymbols(links.rightSpread)];
const transient = symbol as TransientSymbol;
if (transient.leftSpread) {
return [...getRootSymbols(transient.leftSpread), ...getRootSymbols(transient.rightSpread)];
}
if ((symbol as SymbolLinks).syntheticOrigin) {
return getRootSymbols((symbol as SymbolLinks).syntheticOrigin);
if (transient.syntheticOrigin) {
return getRootSymbols(transient.syntheticOrigin);
}

let target: Symbol;
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1879,6 +1879,10 @@
"category": "Error",
"code": 2558
},
"Weak type '{0}' has no properties in common with '{1}'.": {
"category": "Error",
"code": 2559
},
"JSX element attributes type '{0}' may not be a union type.": {
"category": "Error",
"code": 2600
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/APISample_watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ declare var fs: {
existsSync(path: string): boolean;
readdirSync(path: string): string[];
readFileSync(filename: string, encoding?: string): string;
writeFileSync(filename: string, data: any, options?: { encoding?: string; mode?: number; flag?: string; }): void;
writeFileSync(filename: string, data: any, options?: { encoding?: string; mode?: number; flag?: string; } | string): void;
watchFile(filename: string, options: { persistent?: boolean; interval?: number; }, listener: (curr: { mtime: Date }, prev: { mtime: Date }) => void): void;
};
declare var path: any;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,18 @@
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(33,5): error TS2322: Type 'D' is not assignable to type 'C'.
Weak type 'C' has no properties in common with 'D'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(34,5): error TS2322: Type 'E' is not assignable to type 'C'.
Weak type 'C' has no properties in common with 'E'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(35,5): error TS2322: Type 'F' is not assignable to type 'C'.
Weak type 'C' has no properties in common with 'F'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(36,5): error TS2322: Type 'D' is not assignable to type '{ opt?: Base; }'.
Weak type '{ opt?: Base; }' has no properties in common with 'D'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(37,5): error TS2322: Type 'E' is not assignable to type '{ opt?: Base; }'.
Weak type '{ opt?: Base; }' has no properties in common with 'E'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(38,5): error TS2322: Type 'F' is not assignable to type '{ opt?: Base; }'.
Weak type '{ opt?: Base; }' has no properties in common with 'F'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(39,5): error TS2322: Type 'D' is not assignable to type '{ opt?: Base; }'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(40,5): error TS2322: Type 'E' is not assignable to type '{ opt?: Base; }'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(41,5): error TS2322: Type 'F' is not assignable to type '{ opt?: Base; }'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(74,5): error TS2322: Type 'D' is not assignable to type 'C'.
Property 'opt' is missing in type 'D'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts(75,5): error TS2322: Type 'E' is not assignable to type 'C'.
Expand All @@ -18,7 +33,7 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
Property 'opt' is missing in type 'F'.


==== tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts (9 errors) ====
==== tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithObjectMembersOptionality2.ts (18 errors) ====
// M is optional and S contains no property with the same name as M
// N is optional and T contains no property with the same name as N

Expand Down Expand Up @@ -50,20 +65,44 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
var e: E;
var f: F;

// all ok
// disallowed by weak type checking
c = d;
~
!!! error TS2322: Type 'D' is not assignable to type 'C'.
!!! error TS2322: Weak type 'C' has no properties in common with 'D'.
c = e;
~
!!! error TS2322: Type 'E' is not assignable to type 'C'.
!!! error TS2322: Weak type 'C' has no properties in common with 'E'.
c = f;
c = a;

~
!!! error TS2322: Type 'F' is not assignable to type 'C'.
!!! error TS2322: Weak type 'C' has no properties in common with 'F'.
a = d;
~
!!! error TS2322: Type 'D' is not assignable to type '{ opt?: Base; }'.
!!! error TS2322: Weak type '{ opt?: Base; }' has no properties in common with 'D'.
a = e;
~
!!! error TS2322: Type 'E' is not assignable to type '{ opt?: Base; }'.
!!! error TS2322: Weak type '{ opt?: Base; }' has no properties in common with 'E'.
a = f;
a = c;

~
!!! error TS2322: Type 'F' is not assignable to type '{ opt?: Base; }'.
!!! error TS2322: Weak type '{ opt?: Base; }' has no properties in common with 'F'.
b = d;
~
!!! error TS2322: Type 'D' is not assignable to type '{ opt?: Base; }'.
b = e;
~
!!! error TS2322: Type 'E' is not assignable to type '{ opt?: Base; }'.
b = f;
~
!!! error TS2322: Type 'F' is not assignable to type '{ opt?: Base; }'.

// ok
c = a;
a = c;
b = a;
b = c;
}
Expand Down Expand Up @@ -134,4 +173,5 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
!!! error TS2322: Property 'opt' is missing in type 'F'.
b = a; // ok
b = c; // ok
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,20 @@ module TargetHasOptional {
var e: E;
var f: F;

// all ok
// disallowed by weak type checking
c = d;
c = e;
c = f;
c = a;

a = d;
a = e;
a = f;
a = c;

b = d;
b = e;
b = f;

// ok
c = a;
a = c;
b = a;
b = c;
}
Expand Down Expand Up @@ -87,7 +87,8 @@ module SourceHasOptional {
b = f; // error
b = a; // ok
b = c; // ok
}
}


//// [assignmentCompatWithObjectMembersOptionality2.js]
// M is optional and S contains no property with the same name as M
Expand Down Expand Up @@ -129,18 +130,19 @@ var TargetHasOptional;
var d;
var e;
var f;
// all ok
// disallowed by weak type checking
c = d;
c = e;
c = f;
c = a;
a = d;
a = e;
a = f;
a = c;
b = d;
b = e;
b = f;
// ok
c = a;
a = c;
b = a;
b = c;
})(TargetHasOptional || (TargetHasOptional = {}));
Expand Down
12 changes: 6 additions & 6 deletions tests/baselines/reference/checkJsxChildrenProperty1.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ interface Prop {

children: string | JSX.Element
>children : Symbol(Prop.children, Decl(file.tsx, 4, 14))
>JSX : Symbol(JSX, Decl(react.d.ts, 2352, 1))
>Element : Symbol(JSX.Element, Decl(react.d.ts, 2355, 27))
>JSX : Symbol(JSX, Decl(react.d.ts, 2353, 1))
>Element : Symbol(JSX.Element, Decl(react.d.ts, 2356, 27))
}

function Comp(p: Prop) {
Expand All @@ -23,11 +23,11 @@ function Comp(p: Prop) {
>Prop : Symbol(Prop, Decl(file.tsx, 0, 32))

return <div>{p.b}</div>;
>div : Symbol(JSX.IntrinsicElements.div, Decl(react.d.ts, 2399, 45))
>div : Symbol(JSX.IntrinsicElements.div, Decl(react.d.ts, 2400, 45))
>p.b : Symbol(Prop.b, Decl(file.tsx, 3, 14))
>p : Symbol(p, Decl(file.tsx, 8, 14))
>b : Symbol(Prop.b, Decl(file.tsx, 3, 14))
>div : Symbol(JSX.IntrinsicElements.div, Decl(react.d.ts, 2399, 45))
>div : Symbol(JSX.IntrinsicElements.div, Decl(react.d.ts, 2400, 45))
}

// OK
Expand Down Expand Up @@ -59,8 +59,8 @@ let k2 =
>b : Symbol(b, Decl(file.tsx, 19, 16))

<div>hi hi hi!</div>
>div : Symbol(JSX.IntrinsicElements.div, Decl(react.d.ts, 2399, 45))
>div : Symbol(JSX.IntrinsicElements.div, Decl(react.d.ts, 2399, 45))
>div : Symbol(JSX.IntrinsicElements.div, Decl(react.d.ts, 2400, 45))
>div : Symbol(JSX.IntrinsicElements.div, Decl(react.d.ts, 2400, 45))

</Comp>;
>Comp : Symbol(Comp, Decl(file.tsx, 6, 1))
Expand Down
Loading