Skip to content

Fix unassignable properties by adding undefined with exactOptionalPropertyTypes #45032

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

Merged
merged 23 commits into from
Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
664d749
Simple first version
sandersn Jul 13, 2021
d1008eb
Merge branch 'main' into fix-exact-optional-unassignable-properties
sandersn Jul 13, 2021
671096b
Lots of cases work
sandersn Jul 14, 2021
a728852
Support variable declarations, property assignments, destructuring
sandersn Jul 14, 2021
715f235
More cleanup
sandersn Jul 14, 2021
cfea6e1
skip all d.ts, not just node_modules/lib
sandersn Jul 15, 2021
d058191
Merge branch 'main' into fix-exact-optional-unassignable-properties
sandersn Jul 15, 2021
334ca65
Merge branch 'main' into fix-exact-optional-unassignable-properties
sandersn Jul 30, 2021
41f0525
Offer a codefix for a lot more cases
sandersn Aug 3, 2021
09c916f
Merge branch 'main' into fix-exact-optional-unassignable-properties
sandersn Aug 3, 2021
cdd40d3
remove incorrect tuple check
sandersn Aug 3, 2021
f018b8e
Use getSymbolId instead of converting to string
sandersn Aug 3, 2021
960e657
add test + switch to tracking number symbol ids
sandersn Aug 3, 2021
cdbe969
Address PR comments
sandersn Aug 3, 2021
22b9e7a
Merge branch 'main' into fix-exact-optional-unassignable-properties
sandersn Aug 4, 2021
e9607f1
Exclude tuples from suggestion
sandersn Aug 4, 2021
2c1982f
Better way to get error node
sandersn Aug 4, 2021
3ffe9db
fix semicolon lint
sandersn Aug 4, 2021
3e70798
fix another crash
sandersn Aug 5, 2021
4b7b3fe
Simplify: add undefined to all optional propertie
sandersn Aug 5, 2021
d535509
remove fix-all
sandersn Aug 5, 2021
e027d1e
Merge branch 'main' into fix-exact-optional-unassignable-properties
sandersn Aug 6, 2021
cc51d23
Merge branch 'main' into fix-exact-optional-unassignable-properties
sandersn Aug 10, 2021
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
59 changes: 45 additions & 14 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,8 @@ namespace ts {
isTupleType,
isArrayLikeType,
isTypeInvalidDueToUnionDiscriminant,
getExactOptionalUnassignableProperties,
isExactOptionalPropertyMismatch,
getAllPossiblePropertiesOfTypes,
getSuggestedSymbolForNonexistentProperty,
getSuggestionForNonexistentProperty,
Expand Down Expand Up @@ -16753,24 +16755,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);
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 moved this verbatim inside the new else

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;
Copy link
Member Author

Choose a reason for hiding this comment

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

driveby fix; reportedError was always set to true in this block, regardless of whether elaborated is true, and it's not checked anywhere in this block, so it's fine to set it at the top.

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 @@ -16798,7 +16805,6 @@ namespace ts {
}
}
}
reportedError = true;
}
}
}
Expand Down Expand Up @@ -17664,10 +17670,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 @@ -19583,6 +19597,15 @@ namespace ts {
return isUnitType(type) || !!(type.flags & TypeFlags.TemplateLiteral);
}

function getExactOptionalUnassignableProperties(source: Type, target: Type) {
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 getBestMatchingType(source: Type, target: UnionOrIntersectionType, isRelatedTo = compareTypesAssignable) {
return findMatchingDiscriminantType(source, target, isRelatedTo, /*skipPartial*/ true) ||
findMatchingTypeReferenceOrTypeAliasReference(source, target) ||
Expand Down Expand Up @@ -32444,8 +32467,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
20 changes: 20 additions & 0 deletions src/compiler/diagnosticMessages.json
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 @@ -7110,6 +7122,14 @@
"category": "Message",
"code": 95166
},
"Add 'undefined' to all optional properties": {
"category": "Message",
"code": 95167
},
"Add 'undefined' to optional property type": {
"category": "Message",
"code": 95168
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
"category": "Error",
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4305,6 +4305,8 @@ namespace ts {
* e.g. it specifies `kind: "a"` and obj has `kind: "b"`.
*/
/* @internal */ isTypeInvalidDueToUnionDiscriminant(contextualType: Type, obj: ObjectLiteralExpression | JsxAttributes): boolean;
/* @internal */ getExactOptionalUnassignableProperties(source: Type, target: Type): Symbol[];
/* @internal */ isExactOptionalPropertyMismatch(source: Type | undefined, target: Type | undefined): boolean;
/**
* 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
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
139 changes: 139 additions & 0 deletions src/services/codefixes/addOptionalPropertyUndefined.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
/* @internal */
namespace ts.codefix {
const addOptionalPropertyUndefined = "addOptionalPropertyUndefined";

const errorCodes = [
Diagnostics.Type_0_is_not_assignable_to_type_1_with_exactOptionalPropertyTypes_Colon_true_Consider_adding_undefined_to_the_type_of_the_target.code,
Diagnostics.Type_0_is_not_assignable_to_type_1_with_exactOptionalPropertyTypes_Colon_true_Consider_adding_undefined_to_the_types_of_the_target_s_properties.code,
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.code,
];

registerCodeFix({
errorCodes,
getCodeActions(context) {
const typeChecker = context.program.getTypeChecker();
const toAdd = getPropertiesToAdd(context.sourceFile, context.span.start, typeChecker);
if (!toAdd.length) {
return undefined;
}
const changes = textChanges.ChangeTracker.with(context, t => addUndefinedToOptionalProperty(t, toAdd));
return [createCodeFixAction(addOptionalPropertyUndefined, changes, Diagnostics.Add_undefined_to_optional_property_type, addOptionalPropertyUndefined, Diagnostics.Add_undefined_to_all_optional_properties)];
},
fixIds: [addOptionalPropertyUndefined],
getAllCodeActions: context => {
const { program } = context;
const checker = program.getTypeChecker();
const seen = new Map<number, true>();
return createCombinedCodeActions(textChanges.ChangeTracker.with(context, changes => {
eachDiagnostic(context, errorCodes, diag => {
const toAdd = getPropertiesToAdd(diag.file, diag.start, checker);
if (!toAdd.length) {
return;
}
let untouched = true;
for (const add of toAdd) {
if (!addToSeen(seen, getSymbolId(add))) {
untouched = false;
}
}
if (untouched) {
addUndefinedToOptionalProperty(changes, toAdd);
}
});
}));
},
});

function getPropertiesToAdd(file: SourceFile, pos: number, checker: TypeChecker): Symbol[] {
const sourceTarget = getSourceTarget(getErrorNode(file, pos), checker);
if (!sourceTarget) {
return emptyArray;
}
const { source: sourceNode, target: targetNode } = sourceTarget;
const target = checker.getTypeAtLocation(targetNode);
const source = checker.getTypeAtLocation(sourceNode);
if (target.symbol?.declarations?.some(d => getSourceFileOfNode(d).fileName.match(/\.d\.ts$/))) {
return emptyArray;
}
const targetPropertyType = getTargetPropertyType(checker, targetNode);
if (targetPropertyType && checker.isExactOptionalPropertyMismatch(source, targetPropertyType)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe find parent type and fix all, like the other cases

const s = checker.getSymbolAtLocation((targetNode as PropertyAccessExpression).name);
return s ? [s] : emptyArray;
}
return checker.getExactOptionalUnassignableProperties(source, target);
}

function getTargetPropertyType(checker: TypeChecker, targetNode: Node) {
if (isPropertySignature(targetNode)) {
return checker.getTypeAtLocation(targetNode.name);
}
else if (isPropertyAccessExpression(targetNode)) {
return checker.getTypeOfPropertyOfType(checker.getTypeAtLocation(targetNode.expression), targetNode.name.text);
}
return undefined;
}
/**
* Get the part of the incorrect assignment that is useful for type-checking
* eg
* this.definite = 1; ---> `this.definite`
* ^^^^
* definite = source ----> `definite`
* ^^^^^^^^
*/
function getErrorNode(file: SourceFile, pos: number): MemberName | PropertyAccessExpression | undefined {
const start = getTokenAtPosition(file, pos);
return isPropertyAccessExpression(start.parent) && start.parent.expression === start ? start.parent
: isIdentifier(start) || isPrivateIdentifier(start) ? start
: undefined;
}

/**
* Find the source and target of the incorrect assignment.
* The call is recursive for property assignments.
*/
function getSourceTarget(errorNode: Node | undefined, checker: TypeChecker): { source: Node, target: Node } | undefined {
if (!errorNode) {
return undefined;
}
else if (isBinaryExpression(errorNode.parent) && errorNode.parent.operatorToken.kind === SyntaxKind.EqualsToken) {
return { source: errorNode.parent.right, target: errorNode.parent.left };
}
else if (isVariableDeclaration(errorNode.parent) && errorNode.parent.initializer) {
return { source: errorNode.parent.initializer, target: errorNode.parent.name };
}
else if (isCallExpression(errorNode.parent)) {
const n = checker.getSymbolAtLocation(errorNode.parent.expression);
if (!n?.valueDeclaration) return undefined;
if (!isExpression(errorNode)) return undefined;
const i = errorNode.parent.arguments.indexOf(errorNode);
const name = (n.valueDeclaration as any as SignatureDeclaration).parameters[i].name;
if (isIdentifier(name)) return { source: errorNode, target: name };
}
else if (isPropertyAssignment(errorNode.parent) && isIdentifier(errorNode.parent.name) ||
isShorthandPropertyAssignment(errorNode.parent)) {
const parentTarget = getSourceTarget(errorNode.parent.parent, checker);
if (!parentTarget) return undefined;
const prop = checker.getPropertyOfType(checker.getTypeAtLocation(parentTarget.target), (errorNode.parent.name as Identifier).text);
const declaration = prop?.declarations?.[0];
if (!declaration) return undefined;
return {
source: isPropertyAssignment(errorNode.parent) ? errorNode.parent.initializer : errorNode.parent.name,
target: declaration
};
}
return undefined;
}

function addUndefinedToOptionalProperty(changes: textChanges.ChangeTracker, toAdd: Symbol[]) {
for (const add of toAdd) {
const d = add.valueDeclaration;
if (d && (isPropertySignature(d) || isPropertyDeclaration(d)) && d.type) {
const t = factory.createUnionTypeNode([
...d.type.kind === SyntaxKind.UnionType ? (d.type as UnionTypeNode).types : [d.type],
factory.createTypeReferenceNode("undefined")
]);
changes.replaceNode(d.getSourceFile(), d.type, t);
}
}
}
}
1 change: 1 addition & 0 deletions src/services/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
"codefixes/addMissingDeclareProperty.ts",
"codefixes/addMissingInvocationForDecorator.ts",
"codefixes/addNameToNamelessParameter.ts",
"codefixes/addOptionalPropertyUndefined.ts",
"codefixes/annotateWithTypeFromJSDoc.ts",
"codefixes/convertFunctionToEs6Class.ts",
"codefixes/convertToAsyncFunction.ts",
Expand Down
Loading