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

Better errors for indexing gettable/settable values #26446

Merged
merged 5 commits into from Apr 30, 2019

Conversation

collin5
Copy link
Contributor

@collin5 collin5 commented Aug 14, 2018

Gives suggestions where a type has no index signature but has get or set properties.

Fixes #26098

@msftclas
Copy link

msftclas commented Aug 14, 2018

CLA assistant check
All CLA requirements met.

@@ -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}' ?": {
Copy link
Member

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.

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

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.

getPropertyOfObjectType(objectType, <__String>"set")
];

for (const prop of props) {
Copy link
Member

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'
Copy link
Member

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.

@DanielRosenwasser
Copy link
Member

Thanks for being patient! This is a good start! I left some feedback to iterate on.

@collin5 collin5 force-pushed the b26098 branch 2 times, most recently from b02e898 to 0ae6376 Compare September 12, 2018 00:58
@collin5
Copy link
Contributor Author

collin5 commented Sep 12, 2018

Hi @DanielRosenwasser, I've made the requested changes. Feel free to take another look at this, thanks!

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a 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.

const prop = getPropertyOfObjectType(objectType, <__String>name);
if (prop) {
const s = getSingleCallSignature(getTypeOfSymbol(prop));
if (s && getMinArgumentCount(s) === 1 && typeToString(getTypeAtPosition(s, 0)) === "string") {
Copy link
Member

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).


if (isAssignmentTarget(expr)) {
if (hasProp("set")) {
suggestion = symbolToString(objectType.symbol) + ".set";
Copy link
Member

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;
}

Copy link
Contributor Author

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.

@@ -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) {
Copy link
Member

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.

@RyanCavanaugh
Copy link
Member

@collin5 would be good to merge this after the last round of comments + merge conflicts are addressed. Thanks!

@collin5
Copy link
Contributor Author

collin5 commented Apr 30, 2019

Updated. Thank you.

@RyanCavanaugh RyanCavanaugh merged commit 7016d45 into microsoft:master Apr 30, 2019
@collin5 collin5 deleted the b26098 branch April 30, 2019 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better errors for indexing gettable/settable values
4 participants