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

Perform excess property checking on intersection and union members #30853

Merged
merged 8 commits into from
Apr 17, 2019
Merged

Perform excess property checking on intersection and union members #30853

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b3f9bc0
Perform excess property checking on intersection and union members
weswigham Apr 10, 2019
667329d
Allow partial union props to contain the undefined type
weswigham Apr 10, 2019
ac0af46
Add test case from #30771
weswigham Apr 10, 2019
f87ba3f
Un-terse getPossiblePropertiesOfUnionType side-effecting code
weswigham Apr 11, 2019
8dd510b
Fix bug exposed in RWC
weswigham Apr 12, 2019
7a4761c
Cache results of getPossiblePropertiesOfUnionType
weswigham Apr 12, 2019
b7f9326
Fix whitespace
weswigham Apr 16, 2019
f54e5ab
Merge branch 'master' into fix-intersection-target-excess-props-checks
weswigham Apr 16, 2019
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
96 changes: 56 additions & 40 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7473,6 +7473,25 @@ namespace ts {
return type.resolvedProperties;
}

function getPossiblePropertiesOfUnionType(type: UnionType): Symbol[] {
if (type.possiblePropertyCache) {
return type.possiblePropertyCache.size ? arrayFrom(type.possiblePropertyCache.values()) : emptyArray;
weswigham marked this conversation as resolved.
Show resolved Hide resolved
}
type.possiblePropertyCache = createSymbolTable();
for (const t of type.types) {
for (const p of getPropertiesOfType(t)) {
if (!type.possiblePropertyCache.has(p.escapedName)) {
const prop = getUnionOrIntersectionProperty(type, p.escapedName);
if (prop) {
type.possiblePropertyCache.set(p.escapedName, prop);
}
}
}
}
// We can't simply use the normal property cache here, since that will contain cached apparent type members :(
return type.possiblePropertyCache.size ? arrayFrom(type.possiblePropertyCache.values()) : emptyArray;
}

function getPropertiesOfType(type: Type): Symbol[] {
type = getApparentType(type);
return type.flags & TypeFlags.UnionOrIntersection ?
Expand Down Expand Up @@ -7832,7 +7851,7 @@ namespace ts {
const isUnion = containingType.flags & TypeFlags.Union;
const excludeModifiers = isUnion ? ModifierFlags.NonPublicAccessibilityModifier : 0;
// Flags we want to propagate to the result if they exist in all source symbols
let commonFlags = isUnion ? SymbolFlags.None : SymbolFlags.Optional;
let optionalFlag = isUnion ? SymbolFlags.None : SymbolFlags.Optional;
let syntheticFlag = CheckFlags.SyntheticMethod;
let checkFlags = 0;
for (const current of containingType.types) {
Expand All @@ -7841,7 +7860,12 @@ namespace ts {
const prop = getPropertyOfType(type, name);
const modifiers = prop ? getDeclarationModifierFlagsFromSymbol(prop) : 0;
if (prop && !(modifiers & excludeModifiers)) {
commonFlags &= prop.flags;
if (isUnion) {
optionalFlag |= (prop.flags & SymbolFlags.Optional);
}
else {
optionalFlag &= prop.flags;
}
const id = "" + getSymbolId(prop);
if (!propSet.has(id)) {
propSet.set(id, prop);
Expand All @@ -7859,10 +7883,11 @@ namespace ts {
const indexInfo = !isLateBoundName(name) && (isNumericLiteralName(name) && getIndexInfoOfType(type, IndexKind.Number) || getIndexInfoOfType(type, IndexKind.String));
if (indexInfo) {
checkFlags |= indexInfo.isReadonly ? CheckFlags.Readonly : 0;
checkFlags |= CheckFlags.WritePartial;
indexTypes = append(indexTypes, isTupleType(type) ? getRestTypeOfTupleType(type) || undefinedType : indexInfo.type);
}
else {
checkFlags |= CheckFlags.Partial;
checkFlags |= CheckFlags.ReadPartial;
}
}
}
Expand All @@ -7871,7 +7896,7 @@ namespace ts {
return undefined;
}
const props = arrayFrom(propSet.values());
if (props.length === 1 && !(checkFlags & CheckFlags.Partial) && !indexTypes) {
if (props.length === 1 && !(checkFlags & CheckFlags.ReadPartial) && !indexTypes) {
return props[0];
}
let declarations: Declaration[] | undefined;
Expand Down Expand Up @@ -7902,7 +7927,7 @@ namespace ts {
propTypes.push(type);
}
addRange(propTypes, indexTypes);
const result = createSymbol(SymbolFlags.Property | commonFlags, name, syntheticFlag | checkFlags);
const result = createSymbol(SymbolFlags.Property | optionalFlag, name, syntheticFlag | checkFlags);
result.containingType = containingType;
if (!hasNonUniformValueDeclaration && firstValueDeclaration) {
result.valueDeclaration = firstValueDeclaration;
Expand Down Expand Up @@ -7939,7 +7964,7 @@ namespace ts {
function getPropertyOfUnionOrIntersectionType(type: UnionOrIntersectionType, name: __String): Symbol | undefined {
const property = getUnionOrIntersectionProperty(type, name);
// We need to filter out partial properties in union types
return property && !(getCheckFlags(property) & CheckFlags.Partial) ? property : undefined;
return property && !(getCheckFlags(property) & CheckFlags.ReadPartial) ? property : undefined;
}

/**
Expand Down Expand Up @@ -12204,25 +12229,6 @@ namespace ts {
}
}

function isUnionOrIntersectionTypeWithoutNullableConstituents(type: Type): boolean {
if (!(type.flags & TypeFlags.UnionOrIntersection)) {
return false;
}
// at this point we know that this is union or intersection type possibly with nullable constituents.
// check if we still will have compound type if we ignore nullable components.
let seenNonNullable = false;
for (const t of (<UnionOrIntersectionType>type).types) {
if (t.flags & TypeFlags.Nullable) {
continue;
}
if (seenNonNullable) {
return true;
}
seenNonNullable = true;
}
return false;
}

/**
* Compare two types and return
* * Ternary.True if they are related with no assumptions,
Expand Down Expand Up @@ -12277,21 +12283,15 @@ namespace ts {
isSimpleTypeRelatedTo(source, target, relation, reportErrors ? reportError : undefined)) return Ternary.True;

const isComparingJsxAttributes = !!(getObjectFlags(source) & ObjectFlags.JsxAttributes);
if (isObjectLiteralType(source) && getObjectFlags(source) & ObjectFlags.FreshLiteral) {
const isPerformingExcessPropertyChecks = (isObjectLiteralType(source) && getObjectFlags(source) & ObjectFlags.FreshLiteral);
if (isPerformingExcessPropertyChecks) {
const discriminantType = target.flags & TypeFlags.Union ? findMatchingDiscriminantType(source, target as UnionType) : undefined;
if (hasExcessProperties(<FreshObjectLiteralType>source, target, discriminantType, 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.
if (isUnionOrIntersectionTypeWithoutNullableConstituents(target) && !discriminantType) {
source = getRegularTypeOfObjectLiteral(source);
}
}

if (relation !== comparableRelation && !isApparentIntersectionConstituent &&
Expand Down Expand Up @@ -12327,11 +12327,24 @@ namespace ts {
}
else {
if (target.flags & TypeFlags.Union) {
result = typeRelatedToSomeType(source, <UnionType>target, reportErrors && !(source.flags & TypeFlags.Primitive) && !(target.flags & TypeFlags.Primitive));
result = typeRelatedToSomeType(getRegularTypeOfObjectLiteral(source), <UnionType>target, reportErrors && !(source.flags & TypeFlags.Primitive) && !(target.flags & TypeFlags.Primitive));
if (result && isPerformingExcessPropertyChecks) {
// Validate against excess props using the original `source`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the propertiesRelatedTo check more expensive than the typeRelatedToEachType check? Does it make sense to do the checks in the other order?

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 think usually, yeah - we typically consider structural decomposition more expensive than algebraic comparison.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, fair enough. I thought that typeRelatedToEachType would end up calling into propertiesRelatedTo for each union component anyway via isRelatedTo.

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 can; but that's the last kind of comparison we make - any other comparison succeeding avoids it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. Ty!

const discriminantType = target.flags & TypeFlags.Union ? findMatchingDiscriminantType(source, target as UnionType) : undefined;
if (!propertiesRelatedTo(source, discriminantType || target, reportErrors)) {
return Ternary.False;
}
}
}
else if (target.flags & TypeFlags.Intersection) {
isIntersectionConstituent = true; // set here to affect the following trio of checks
result = typeRelatedToEachType(source, target as IntersectionType, reportErrors);
result = typeRelatedToEachType(getRegularTypeOfObjectLiteral(source), target as IntersectionType, reportErrors);
if (result && isPerformingExcessPropertyChecks) {
// Validate against excess props using the original `source`
if (!propertiesRelatedTo(source, target, reportErrors)) {
return Ternary.False;
}
}
}
else if (source.flags & TypeFlags.Intersection) {
// Check to see if any constituents of the intersection are immediately related to the target.
Expand Down Expand Up @@ -12431,7 +12444,7 @@ namespace ts {
// check excess properties against discriminant type only, not the entire union
return hasExcessProperties(source, discriminant, /*discriminant*/ undefined, reportErrors);
}
for (const prop of getPropertiesOfObjectType(source)) {
for (const prop of getPropertiesOfType(source)) {
if (shouldCheckAsExcessProperty(prop, source.symbol) && !isKnownProperty(target, prop.escapedName, isComparingJsxAttributes)) {
if (reportErrors) {
// Report error in terms of object types in the target as those are the only ones
Expand Down Expand Up @@ -13150,7 +13163,9 @@ namespace ts {
}
}
}
const properties = getPropertiesOfObjectType(target);
// We only call this for union target types when we're attempting to do excess property checking - in those cases, we want to get _all possible props_
// from the target union, across all members
const properties = target.flags & TypeFlags.Union ? getPossiblePropertiesOfUnionType(target as UnionType) : getPropertiesOfType(target);
for (const targetProp of properties) {
if (!(targetProp.flags & SymbolFlags.Prototype)) {
const sourceProp = getPropertyOfType(source, targetProp.escapedName);
Expand Down Expand Up @@ -13198,7 +13213,8 @@ namespace ts {
}
return Ternary.False;
}
const related = isRelatedTo(getTypeOfSymbol(sourceProp), getTypeOfSymbol(targetProp), reportErrors);
// If the target comes from a partial union prop, allow `undefined` in the target type
const related = isRelatedTo(getTypeOfSymbol(sourceProp), addOptionality(getTypeOfSymbol(targetProp), !!(getCheckFlags(targetProp) & CheckFlags.Partial)), reportErrors);
if (!related) {
if (reportErrors) {
reportError(Diagnostics.Types_of_property_0_are_incompatible, symbolToString(targetProp));
Expand Down Expand Up @@ -14550,9 +14566,9 @@ namespace ts {
}

function* getUnmatchedProperties(source: Type, target: Type, requireOptionalProperties: boolean, matchDiscriminantProperties: boolean) {
const properties = target.flags & TypeFlags.Intersection ? getPropertiesOfUnionOrIntersectionType(<IntersectionType>target) : getPropertiesOfObjectType(target);
const properties = target.flags & TypeFlags.Union ? getPossiblePropertiesOfUnionType(target as UnionType) : getPropertiesOfType(target);
for (const targetProp of properties) {
if (requireOptionalProperties || !(targetProp.flags & SymbolFlags.Optional)) {
if (requireOptionalProperties || !(targetProp.flags & SymbolFlags.Optional || getCheckFlags(targetProp) & CheckFlags.Partial)) {
const sourceProp = getPropertyOfType(source, targetProp.escapedName);
if (!sourceProp) {
yield targetProp;
Expand Down
28 changes: 16 additions & 12 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3746,19 +3746,21 @@ namespace ts {
SyntheticProperty = 1 << 1, // Property in union or intersection type
SyntheticMethod = 1 << 2, // Method in union or intersection type
Readonly = 1 << 3, // Readonly transient symbol
Partial = 1 << 4, // Synthetic property present in some but not all constituents
HasNonUniformType = 1 << 5, // Synthetic property with non-uniform type in constituents
HasLiteralType = 1 << 6, // Synthetic property with at least one literal type in constituents
ContainsPublic = 1 << 7, // Synthetic property with public constituent(s)
ContainsProtected = 1 << 8, // Synthetic property with protected constituent(s)
ContainsPrivate = 1 << 9, // Synthetic property with private constituent(s)
ContainsStatic = 1 << 10, // Synthetic property with static constituent(s)
Late = 1 << 11, // Late-bound symbol for a computed property with a dynamic name
ReverseMapped = 1 << 12, // Property of reverse-inferred homomorphic mapped type
OptionalParameter = 1 << 13, // Optional parameter
RestParameter = 1 << 14, // Rest parameter
ReadPartial = 1 << 4, // Synthetic property present in some but not all constituents
WritePartial = 1 << 5, // Synthetic property present in some but only satisfied by an index signature in others
HasNonUniformType = 1 << 6, // Synthetic property with non-uniform type in constituents
HasLiteralType = 1 << 7, // Synthetic property with at least one literal type in constituents
ContainsPublic = 1 << 8, // Synthetic property with public constituent(s)
ContainsProtected = 1 << 9, // Synthetic property with protected constituent(s)
ContainsPrivate = 1 << 10, // Synthetic property with private constituent(s)
Copy link
Member

Choose a reason for hiding this comment

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

Off-by-one whitespace 😲

ContainsStatic = 1 << 11, // Synthetic property with static constituent(s)
Late = 1 << 12, // Late-bound symbol for a computed property with a dynamic name
ReverseMapped = 1 << 13, // Property of reverse-inferred homomorphic mapped type
OptionalParameter = 1 << 14, // Optional parameter
RestParameter = 1 << 15, // Rest parameter
Synthetic = SyntheticProperty | SyntheticMethod,
Discriminant = HasNonUniformType | HasLiteralType
Discriminant = HasNonUniformType | HasLiteralType,
Partial = ReadPartial | WritePartial
}

/* @internal */
Expand Down Expand Up @@ -4171,6 +4173,8 @@ namespace ts {
}

export interface UnionType extends UnionOrIntersectionType {
/* @internal */
possiblePropertyCache?: SymbolTable; // Cache of _all_ resolved properties less any from aparent members
}

export interface IntersectionType extends UnionOrIntersectionType {
Expand Down
48 changes: 48 additions & 0 deletions tests/baselines/reference/deepExcessPropertyCheckingWhenTargetIsIntersection.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
tests/cases/compiler/deepExcessPropertyCheckingWhenTargetIsIntersection.ts(21,33): error TS2322: Type '{ INVALID_PROP_NAME: string; ariaLabel: string; }' is not assignable to type 'ITestProps'.
Object literal may only specify known properties, and 'INVALID_PROP_NAME' does not exist in type 'ITestProps'.
tests/cases/compiler/deepExcessPropertyCheckingWhenTargetIsIntersection.ts(27,34): error TS2326: Types of property 'icon' are incompatible.
Type '{ props: { INVALID_PROP_NAME: string; ariaLabel: string; }; }' is not assignable to type 'NestedProp<ITestProps>'.
Types of property 'props' are incompatible.
Type '{ INVALID_PROP_NAME: string; ariaLabel: string; }' is not assignable to type 'ITestProps'.
Object literal may only specify known properties, and 'INVALID_PROP_NAME' does not exist in type 'ITestProps'.


==== tests/cases/compiler/deepExcessPropertyCheckingWhenTargetIsIntersection.ts (2 errors) ====
interface StatelessComponent<P = {}> {
(props: P & { children?: number }, context?: any): null;
}

const TestComponent: StatelessComponent<TestProps> = (props) => {
return null;
}

interface ITestProps {
ariaLabel?: string;
}

interface NestedProp<TProps> {
props: TProps;
}

interface TestProps {
icon: NestedProp<ITestProps>;
}

TestComponent({icon: { props: { INVALID_PROP_NAME: 'share', ariaLabel: 'test label' } }});
~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2322: Type '{ INVALID_PROP_NAME: string; ariaLabel: string; }' is not assignable to type 'ITestProps'.
!!! error TS2322: Object literal may only specify known properties, and 'INVALID_PROP_NAME' does not exist in type 'ITestProps'.
!!! related TS6500 tests/cases/compiler/deepExcessPropertyCheckingWhenTargetIsIntersection.ts:14:3: The expected type comes from property 'props' which is declared here on type 'NestedProp<ITestProps>'

const TestComponent2: StatelessComponent<TestProps | {props2: {x: number}}> = (props) => {
return null;
}

TestComponent2({icon: { props: { INVALID_PROP_NAME: 'share', ariaLabel: 'test label' } }});
~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2326: Types of property 'icon' are incompatible.
!!! error TS2326: Type '{ props: { INVALID_PROP_NAME: string; ariaLabel: string; }; }' is not assignable to type 'NestedProp<ITestProps>'.
!!! error TS2326: Types of property 'props' are incompatible.
!!! error TS2326: Type '{ INVALID_PROP_NAME: string; ariaLabel: string; }' is not assignable to type 'ITestProps'.
!!! error TS2326: Object literal may only specify known properties, and 'INVALID_PROP_NAME' does not exist in type 'ITestProps'.

Loading