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

Perform excess property checking on intersection and union members #30853

Merged
Show file tree
Hide file tree
Changes from 3 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
84 changes: 44 additions & 40 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7473,6 +7473,13 @@ namespace ts {
return type.resolvedProperties;
}

function getPossiblePropertiesOfUnionType(type: UnionType): Symbol[] {
// The following is for effects - getUnionOrIntersectionProperty will cache all the possible union properties into `type`
void map(flatMap(type.types, getPropertiesOfType), p => getUnionOrIntersectionProperty(type, p.escapedName));
weswigham marked this conversation as resolved.
Show resolved Hide resolved
// And we can then (uniquely) fetch them out of the cache, instead of as a result of the above call.
return !type.propertyCache ? emptyArray : arrayFrom(type.propertyCache.values());
}

function getPropertiesOfType(type: Type): Symbol[] {
type = getApparentType(type);
return type.flags & TypeFlags.UnionOrIntersection ?
Expand Down Expand Up @@ -7832,7 +7839,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 +7848,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 +7871,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 +7884,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 +7915,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 +7952,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 +12217,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 +12271,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 +12315,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`
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)) {
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!

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 +12432,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 +13151,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 +13201,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 +14554,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
26 changes: 14 additions & 12 deletions src/compiler/types.ts
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
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'.

Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//// [deepExcessPropertyCheckingWhenTargetIsIntersection.ts]
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' } }});

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

TestComponent2({icon: { props: { INVALID_PROP_NAME: 'share', ariaLabel: 'test label' } }});


//// [deepExcessPropertyCheckingWhenTargetIsIntersection.js]
var TestComponent = function (props) {
return null;
};
TestComponent({ icon: { props: { INVALID_PROP_NAME: 'share', ariaLabel: 'test label' } } });
var TestComponent2 = function (props) {
return null;
};
TestComponent2({ icon: { props: { INVALID_PROP_NAME: 'share', ariaLabel: 'test label' } } });
Loading