Skip to content

Commit

Permalink
Fix unassignable properties by adding undefined with exactOptionalPro…
Browse files Browse the repository at this point in the history 
…pertyTypes (#45032)

* Simple first version

Doesn't cover or test any complicated variations.

* Lots of cases work

Destructuring does not. But

- skipping node_modules and lib.* does.
- call expressions does
- property access, including with private identifiers, does

* Support variable declarations, property assignments, destructuring

As long as it's not nested

* More cleanup

* skip all d.ts, not just node_modules/lib

* Offer a codefix for a lot more cases

* remove incorrect tuple check

* Use getSymbolId instead of converting to string

Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>

* add test + switch to tracking number symbol ids

* Address PR comments

* Exclude tuples from suggestion

* Better way to get error node

Plus add a check that errorNode is an argument to the call, not the
call's expression.

* fix semicolon lint

* fix another crash

* Simplify: add undefined to all optional propertie

whether or not somebody tried to assign undefined to them in the
erroneous assignment

* remove fix-all

Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>
  • Loading branch information
@sandersn @andrewbranch
sandersn and andrewbranch committed Aug 10, 2021
1 parent 92e7fb5 commit 8d4fe5a
Show file tree
Hide file tree
Showing 31 changed files with 992 additions and 100 deletions.
63 changes: 49 additions & 14 deletions src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,7 @@ namespace ts {
isTupleType,
isArrayLikeType,
isTypeInvalidDueToUnionDiscriminant,
getExactOptionalProperties,
getAllPossiblePropertiesOfTypes,
getSuggestedSymbolForNonexistentProperty,
getSuggestionForNonexistentProperty,
Expand Down Expand Up @@ -16768,24 +16769,29 @@ namespace ts {
let sourcePropType = getIndexedAccessTypeOrUndefined(source, nameType);
if (!sourcePropType) continue;
const propName = getPropertyNameFromIndex(nameType, /*accessNode*/ undefined);
const targetIsOptional = !!(propName && (getPropertyOfType(target, propName) || unknownSymbol).flags & SymbolFlags.Optional);
const sourceIsOptional = !!(propName && (getPropertyOfType(source, propName) || unknownSymbol).flags & SymbolFlags.Optional);
targetPropType = removeMissingType(targetPropType, targetIsOptional);
sourcePropType = removeMissingType(sourcePropType, targetIsOptional && sourceIsOptional);
if (!checkTypeRelatedTo(sourcePropType, targetPropType, relation, /*errorNode*/ undefined)) {
const elaborated = next && elaborateError(next, sourcePropType, targetPropType, relation, /*headMessage*/ undefined, containingMessageChain, errorOutputContainer);
if (elaborated) {
reportedError = true;
}
else {
reportedError = true;
if (!elaborated) {
// Issue error on the prop itself, since the prop couldn't elaborate the error
const resultObj: { errors?: Diagnostic[] } = errorOutputContainer || {};
// Use the expression type, if available
const specificSource = next ? checkExpressionForMutableLocationWithContextualType(next, sourcePropType) : sourcePropType;
const result = checkTypeRelatedTo(specificSource, targetPropType, relation, prop, errorMessage, containingMessageChain, resultObj);
if (result && specificSource !== sourcePropType) {
// If for whatever reason the expression type doesn't yield an error, make sure we still issue an error on the sourcePropType
checkTypeRelatedTo(sourcePropType, targetPropType, relation, prop, errorMessage, containingMessageChain, resultObj);
if (exactOptionalPropertyTypes && isExactOptionalPropertyMismatch(specificSource, targetPropType)) {
const diag = createDiagnosticForNode(prop, Diagnostics.Type_0_is_not_assignable_to_type_1_with_exactOptionalPropertyTypes_Colon_true_Consider_adding_undefined_to_the_type_of_the_target, typeToString(specificSource), typeToString(targetPropType));
diagnostics.add(diag);
resultObj.errors = [diag];
}
else {
const targetIsOptional = !!(propName && (getPropertyOfType(target, propName) || unknownSymbol).flags & SymbolFlags.Optional);
const sourceIsOptional = !!(propName && (getPropertyOfType(source, propName) || unknownSymbol).flags & SymbolFlags.Optional);
targetPropType = removeMissingType(targetPropType, targetIsOptional);
sourcePropType = removeMissingType(sourcePropType, targetIsOptional && sourceIsOptional);
const result = checkTypeRelatedTo(specificSource, targetPropType, relation, prop, errorMessage, containingMessageChain, resultObj);
if (result && specificSource !== sourcePropType) {
// If for whatever reason the expression type doesn't yield an error, make sure we still issue an error on the sourcePropType
checkTypeRelatedTo(sourcePropType, targetPropType, relation, prop, errorMessage, containingMessageChain, resultObj);
}
}
if (resultObj.errors) {
const reportedDiag = resultObj.errors[resultObj.errors.length - 1];
Expand Down Expand Up @@ -16813,7 +16819,6 @@ namespace ts {
}
}
}
reportedError = true;
}
}
}
Expand Down Expand Up @@ -17679,10 +17684,18 @@ namespace ts {
else if (sourceType === targetType) {
message = Diagnostics.Type_0_is_not_assignable_to_type_1_Two_different_types_with_this_name_exist_but_they_are_unrelated;
}
else if (exactOptionalPropertyTypes && getExactOptionalUnassignableProperties(source, target).length) {
message = Diagnostics.Type_0_is_not_assignable_to_type_1_with_exactOptionalPropertyTypes_Colon_true_Consider_adding_undefined_to_the_types_of_the_target_s_properties;
}
else {
message = Diagnostics.Type_0_is_not_assignable_to_type_1;
}
}
else if (message === Diagnostics.Argument_of_type_0_is_not_assignable_to_parameter_of_type_1
&& exactOptionalPropertyTypes
&& getExactOptionalUnassignableProperties(source, target).length) {
message = Diagnostics.Argument_of_type_0_is_not_assignable_to_parameter_of_type_1_with_exactOptionalPropertyTypes_Colon_true_Consider_adding_undefined_to_the_types_of_the_target_s_properties;
}

reportError(message, generalizedSourceType, targetType);
}
Expand Down Expand Up @@ -19598,6 +19611,20 @@ namespace ts {
return isUnitType(type) || !!(type.flags & TypeFlags.TemplateLiteral);
}

function getExactOptionalUnassignableProperties(source: Type, target: Type) {
if (isTupleType(source) && isTupleType(target)) return emptyArray;
return getPropertiesOfType(target)
.filter(targetProp => isExactOptionalPropertyMismatch(getTypeOfPropertyOfType(source, targetProp.escapedName), getTypeOfSymbol(targetProp)));
}

function isExactOptionalPropertyMismatch(source: Type | undefined, target: Type | undefined) {
return !!source && !!target && maybeTypeOfKind(source, TypeFlags.Undefined) && !!containsMissingType(target);
}

function getExactOptionalProperties(type: Type) {
return getPropertiesOfType(type).filter(targetProp => containsMissingType(getTypeOfSymbol(targetProp)));
}

function getBestMatchingType(source: Type, target: UnionOrIntersectionType, isRelatedTo = compareTypesAssignable) {
return findMatchingDiscriminantType(source, target, isRelatedTo, /*skipPartial*/ true) ||
findMatchingTypeReferenceOrTypeAliasReference(source, target) ||
Expand Down Expand Up @@ -32479,8 +32506,16 @@ namespace ts {
Diagnostics.The_left_hand_side_of_an_assignment_expression_must_be_a_variable_or_a_property_access,
Diagnostics.The_left_hand_side_of_an_assignment_expression_may_not_be_an_optional_property_access)
&& (!isIdentifier(left) || unescapeLeadingUnderscores(left.escapedText) !== "exports")) {

let headMessage: DiagnosticMessage | undefined;
if (exactOptionalPropertyTypes && isPropertyAccessExpression(left) && maybeTypeOfKind(valueType, TypeFlags.Undefined)) {
const target = getTypeOfPropertyOfType(getTypeOfExpression(left.expression), left.name.escapedText);
if (isExactOptionalPropertyMismatch(valueType, target)) {
headMessage = Diagnostics.Type_0_is_not_assignable_to_type_1_with_exactOptionalPropertyTypes_Colon_true_Consider_adding_undefined_to_the_type_of_the_target;
}
}
// to avoid cascading errors check assignability only if 'isReference' check succeeded and no errors were reported
checkTypeAssignableToAndOptionallyElaborate(valueType, leftType, left, right);
checkTypeAssignableToAndOptionallyElaborate(valueType, leftType, left, right, headMessage);
}
}
}
Expand Down
16 changes: 16 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1738,6 +1738,10 @@
"category": "Error",
"code": 2374
},
"Type '{0}' is not assignable to type '{1}' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.": {
"category": "Error",
"code": 2375
},
"A 'super' call must be the first statement in the constructor when a class contains initialized properties, parameter properties, or private identifiers.": {
"category": "Error",
"code": 2376
Expand All @@ -1750,6 +1754,10 @@
"category": "Error",
"code": 2378
},
"Argument of type '{0}' is not assignable to parameter of type '{1}' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.": {
"category": "Error",
"code": 2379
},
"The return type of a 'get' accessor must be assignable to its 'set' accessor type": {
"category": "Error",
"code": 2380
Expand Down Expand Up @@ -1874,6 +1882,10 @@
"category": "Error",
"code": 2410
},
"Type '{0}' is not assignable to type '{1}' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the type of the target.": {
"category": "Error",
"code": 2412
},
"Property '{0}' of type '{1}' is not assignable to '{2}' index type '{3}'.": {
"category": "Error",
"code": 2411
Expand Down Expand Up @@ -7118,6 +7130,10 @@
"category": "Message",
"code": 95168
},
"Add 'undefined' to optional property type": {
"category": "Message",
"code": 95169
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
"category": "Error",
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4308,6 +4308,7 @@ namespace ts {
* e.g. it specifies `kind: "a"` and obj has `kind: "b"`.
*/
/* @internal */ isTypeInvalidDueToUnionDiscriminant(contextualType: Type, obj: ObjectLiteralExpression | JsxAttributes): boolean;
/* @internal */ getExactOptionalProperties(type: Type): Symbol[];
/**
* For a union, will include a property if it's defined in *any* of the member types.
* So for `{ a } | { b }`, this will include both `a` and `b`.
Expand Down
3 changes: 2 additions & 1 deletion src/harness/fourslashImpl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,8 @@ namespace FourSlash {
if (errors.length) {
this.printErrorLog(/*expectErrors*/ false, errors);
const error = errors[0];
this.raiseError(`Found an error: ${this.formatPosition(error.file!, error.start!)}: ${error.messageText}`);
const message = typeof error.messageText === "string" ? error.messageText : error.messageText.messageText;
this.raiseError(`Found an error: ${this.formatPosition(error.file!, error.start!)}: ${message}`);
}
});
}
Expand Down
28 changes: 9 additions & 19 deletions src/services/codefixes/addMissingAwait.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace ts.codefix {
errorCodes,
getCodeActions: context => {
const { sourceFile, errorCode, span, cancellationToken, program } = context;
const expression = getFixableErrorSpanExpression(sourceFile, errorCode, span, cancellationToken, program);
const expression = getAwaitErrorSpanExpression(sourceFile, errorCode, span, cancellationToken, program);
if (!expression) {
return;
}
Expand All @@ -48,7 +48,7 @@ namespace ts.codefix {
const checker = context.program.getTypeChecker();
const fixedDeclarations = new Set<number>();
return codeFixAll(context, errorCodes, (t, diagnostic) => {
const expression = getFixableErrorSpanExpression(sourceFile, diagnostic.code, diagnostic, cancellationToken, program);
const expression = getAwaitErrorSpanExpression(sourceFile, diagnostic.code, diagnostic, cancellationToken, program);
if (!expression) {
return;
}
Expand All @@ -59,6 +59,13 @@ namespace ts.codefix {
},
});

function getAwaitErrorSpanExpression(sourceFile: SourceFile, errorCode: number, span: TextSpan, cancellationToken: CancellationToken, program: Program) {
const expression = getFixableErrorSpanExpression(sourceFile, span);
return expression
&& isMissingAwaitError(sourceFile, errorCode, span, cancellationToken, program)
&& isInsideAwaitableBody(expression) ? expression : undefined;
}

function getDeclarationSiteFix(context: CodeFixContext | CodeFixAllContext, expression: Expression, errorCode: number, checker: TypeChecker, trackChanges: ContextualTrackChangesFunction, fixedDeclarations?: Set<number>) {
const { sourceFile, program, cancellationToken } = context;
const awaitableInitializers = findAwaitableInitializers(expression, sourceFile, cancellationToken, program, checker);
Expand Down Expand Up @@ -95,23 +102,6 @@ namespace ts.codefix {
some(relatedInformation, related => related.code === Diagnostics.Did_you_forget_to_use_await.code));
}

function getFixableErrorSpanExpression(sourceFile: SourceFile, errorCode: number, span: TextSpan, cancellationToken: CancellationToken, program: Program): Expression | undefined {
const token = getTokenAtPosition(sourceFile, span.start);
// Checker has already done work to determine that await might be possible, and has attached
// related info to the node, so start by finding the expression that exactly matches up
// with the diagnostic range.
const expression = findAncestor(token, node => {
if (node.getStart(sourceFile) < span.start || node.getEnd() > textSpanEnd(span)) {
return "quit";
}
return isExpression(node) && textSpansEqual(span, createTextSpanFromNode(node, sourceFile));
}) as Expression | undefined;

return expression
&& isMissingAwaitError(sourceFile, errorCode, span, cancellationToken, program)
&& isInsideAwaitableBody(expression) ? expression : undefined;
}

interface AwaitableInitializer {
expression: Expression;
declarationSymbol: Symbol;
Expand Down

0 comments on commit 8d4fe5a

Please sign in to comment.