Skip to content

feat(45679): support 'did you mean' diagnostics for string literal union#45723

Merged
sandersn merged 3 commits intomicrosoft:mainfrom
hi-ogawa:45679-did-you-mean-string-literal-union
Sep 14, 2021
Merged

feat(45679): support 'did you mean' diagnostics for string literal union#45723
sandersn merged 3 commits intomicrosoft:mainfrom
hi-ogawa:45679-did-you-mean-string-literal-union

Conversation

@hi-ogawa
Copy link
Copy Markdown
Contributor

@hi-ogawa hi-ogawa commented Sep 4, 2021

Fixes #45679

This PR adds a new diagnostic message for assignment type error with "did you mean" suggestion.
I made a new if-branch in reportRelationError for the case when source is StringLiteralType and target is UnionType and it tries to find suggestion with the existing utility function getSpellingSuggestion.
So far I only tested with simple examples I can think of, but if more complicated cases are desired, I'm happy to add more.
So please let me know if that's the case.
Thanks a lot!

EDIT:
I thought I could also try to implement codefix service for this "did you mean" suggestion, but it doesn't look so easy since there seems no way of obtaining target type information at the time of getCodeActions in fixSpelling.ts. I might be missing something here, so I would appreciate if there's some advice on this.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 4, 2021
@orta orta self-assigned this Sep 7, 2021
@orta
Copy link
Copy Markdown
Contributor

orta commented Sep 7, 2021

Looks pretty solid to me.

I think for the codefix you should be able to use checker.getTypeAtLocation(node) to get the type which corresponds to source which should be the union.

Then you should have enough information to re-run getSuggestedTypeForNonexistentStringLiteralType (expose it in the Checker type and make it @internal to be available at the services later)

@hi-ogawa
Copy link
Copy Markdown
Contributor Author

hi-ogawa commented Sep 7, 2021

Thanks for the comment and tips!
As I look further into codefix, however, I think another stumbling block is that currently diagnostic range is not the mis-spelled string literal but the assigned identifier (or the property key), as in the test case:

    type T1 = "string" | "number" | "boolean";
    const t1: T1 = "strong";
          ~~
!!! error TS2820: Type '"strong"' is not assignable to type 'T1'. Did you mean '"string"'?

So, unfortunately, implementing this properly would be probably beyond my capacity...

Copy link
Copy Markdown
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I like what you have here. Two minor requests.

I looked at the other spelling codefixes. They all work on right-hand sides (rhs) for which the value is misspelled, not the type. It's possible to inspect the assignment's rhs and offer the fix only in the case when it is a value with a unit type. But I don't think that needs to happen in this PR. Let me know whether you'd like to try that.

Comment thread src/compiler/checker.ts Outdated

function getSuggestedTypeForNonexistentStringLiteralType(source: StringLiteralType, target: UnionType): StringLiteralType | undefined {
const candidates = target.types.filter((type): type is StringLiteralType => !!(type.flags & TypeFlags.StringLiteral));
return getSpellingSuggestion(source.value, candidates, (type) => type.value);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

very minor: no parens around arrow parameter

Suggested change
return getSpellingSuggestion(source.value, candidates, (type) => type.value);
return getSpellingSuggestion(source.value, candidates, type => type.value);

Comment thread src/compiler/checker.ts Outdated
if (source.flags & TypeFlags.StringLiteral && target.flags & TypeFlags.Union) {
const suggestedType = getSuggestedTypeForNonexistentStringLiteralType(source as StringLiteralType, target as UnionType);
if (suggestedType) {
reportError(Diagnostics.Type_0_is_not_assignable_to_type_1_Did_you_mean_2, sourceType, targetType, typeToString(suggestedType));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Currently, generalizedSourceType must === sourceType in this case, but it would be better to use generalizedSourceType here, like the other reportErrors, in case we add some additional processing at the beginning of the function.

Suggested change
reportError(Diagnostics.Type_0_is_not_assignable_to_type_1_Did_you_mean_2, sourceType, targetType, typeToString(suggestedType));
reportError(Diagnostics.Type_0_is_not_assignable_to_type_1_Did_you_mean_2, generalizedSourceType, targetType, typeToString(suggestedType));

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 14, 2021
@hi-ogawa
Copy link
Copy Markdown
Contributor Author

@sandersn Thanks for the review! I addressed your feedback in 3dba734.

Thanks for suggestion about codefix as well. But for the codefix feature, I cannot really estimate how it's feasible for me to implement right now, so as you mention, I'd like it to be separate PR from this one if I would even try it. Other contributors might be able to pick it up, so maybe it's better in that way.

If there's anything else I can do on this PR, please let me know. Thank you so much!

@sandersn sandersn merged commit 4f8aa52 into microsoft:main Sep 14, 2021
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Backlog Bug PRs that fix a backlog bug

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Support 'did you mean' for assignments to unions with string literals

4 participants