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
Better errors for indexing gettable/settable values #26446
Conversation
@@ -3945,6 +3945,10 @@ | |||
"category": "Error", | |||
"code": 7042 | |||
}, | |||
"Element implicitly has an 'any' type because type '{0}' has no index signature. Did you mean to call '{1}' ?": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the stray space at the end.
src/compiler/checker.ts
Outdated
const s = getSingleCallSignature(getTypeOfSymbol(prop)); | ||
if (s && getMinArgumentCount(s) === 1 && typeToString(getTypeAtPosition(s, 0)) === "string") { | ||
const suggestion = symbolToString(objectType.symbol) + "." + symbolToString(prop); | ||
suggestions = (!suggestions) ? suggestion : suggestions.concat(" or " + suggestion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "or " is not localizable (i.e. just concatenating it into a diagnostic means that it might not translate correctly in another language.
src/compiler/checker.ts
Outdated
getPropertyOfObjectType(objectType, <__String>"set") | ||
]; | ||
|
||
for (const prop of props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is a little too general. You really only want to recommend "set"
when the element access is on the left hand side of some mutating operator (e.g. =
, +=
, ++
, etc.). You want to try to recommend "get"
otherwise.
}; | ||
d['hello']; | ||
|
||
// Should give suggestion 'e.get or e.set' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try to avoid (and remove) these comments regarding error messages in general. The baselines should communicate that on their own.
Thanks for being patient! This is a good start! I left some feedback to iterate on. |
b02e898
to
0ae6376
Compare
Hi @DanielRosenwasser, I've made the requested changes. Feel free to take another look at this, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay! Hopefully my feedback is still useful.
src/compiler/checker.ts
Outdated
const prop = getPropertyOfObjectType(objectType, <__String>name); | ||
if (prop) { | ||
const s = getSingleCallSignature(getTypeOfSymbol(prop)); | ||
if (s && getMinArgumentCount(s) === 1 && typeToString(getTypeAtPosition(s, 0)) === "string") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arg count is incorrect for set
methods (it should probably be 2).
src/compiler/checker.ts
Outdated
|
||
if (isAssignmentTarget(expr)) { | ||
if (hasProp("set")) { | ||
suggestion = symbolToString(objectType.symbol) + ".set"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately symbolToString
might not work if the expression isn't a simple binding. You'd be better off with something like
const suggestedMethod = isAssignmentTarget(expr) ? "set" : "get";
if (!hasProp(suggestedMethod)) {
return undefined;
}
let suggestion = tryGetPropertyAccessOrIdentifierToString(expr);
if (suggestion === undefined) {
suggestion = suggestedMethod;
}
else {
suggestion += "." + suggestedMethod;
}
where you could define tryGetPropertyAccessOrIdentifierToString
in utilities.ts
.
function tryGetPropertyAccessOrIdentifierToString(expr: Expression): string | undefined {
if (isPropertyAccessExpression(expr)) {
return tryGetPropertyAccessOrIdentifierToString(expr.expression) + "." + x.name;
}
if (isIdentifier(expr)) {
return unescapeLeadingUnderscores(expr.escapedText);
}
return undefined;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! Let me do this in a few. Thank you.
src/compiler/checker.ts
Outdated
@@ -9476,7 +9476,13 @@ namespace ts { | |||
} | |||
} | |||
else { | |||
error(accessExpression, Diagnostics.Element_implicitly_has_an_any_type_because_type_0_has_no_index_signature, typeToString(objectType)); | |||
const suggestion = getSuggestionForNonexistentIndexSignature(objectType, accessExpression); | |||
if (suggestion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other nit - use suggestion !== undefined
.
@collin5 would be good to merge this after the last round of comments + merge conflicts are addressed. Thanks! |
Updated. Thank you. |
Gives suggestions where a type has no index signature but has
get
orset
properties.Fixes #26098