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

Feat/quick fix for types #42126

Merged
merged 15 commits into from
Oct 5, 2021
Merged

Conversation

chenjigeng
Copy link
Contributor

@chenjigeng chenjigeng commented Dec 27, 2020

Fixes #40314

It's easy to fix the code below.

before:

declare let a: numbers;
declare let b: Numbers;
declare let c: objects;
declare let d: Objects;
declare let e: RegEx;

namespace yadda {
  export type Thing = string;
}
let f: yaddas.Thing;

after:

declare let a: Number;
declare let b: Number;
declare let c: Object;
declare let d: Object;
declare let e: RegExp;

namespace yadda {
  export type Thing = string;
}
let f: yadda.Thing;

@DanielRosenwasser please help see this

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 27, 2020
@sandersn sandersn added this to Not started in PR Backlog Jan 9, 2021
PR Backlog automation moved this from Not started to Waiting on author Jan 9, 2021
Copy link
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.

It's impressive how much those two lines of code are doing. However, I only want to offer the codefix for the case where the did-you-mean applies and is offered. That means adding did-you-mean errors for Cannot_find_name_0 and hooking them up properly to type resolution.

@chenjigeng
Copy link
Contributor Author

chenjigeng commented Jan 9, 2021

I think what you said is reasonable, I can have a try

@chenjigeng
Copy link
Contributor Author

It has been modified. If you are free, please help to check it @sandersn

@sandersn sandersn moved this from Waiting on author to Needs review in PR Backlog Jan 16, 2021
Copy link
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.

Sorry for the long wait. I have two suggestions:

  1. Unconditionally issue suggestions in resolveName if errors are requested.
  2. Make sure that string is suggested before String, as well as for the other primitive types.

I realise I dropped the ball on this, so if you don't have time or energy to finish this PR, I can take over.

fixId: "fixSpelling",
fixAllDescription: "Fix all detected spelling errors",
newFileContent:
`declare let a: Number;
Copy link
Member

Choose a reason for hiding this comment

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

Strange that this doesn't correct to number after #39060. Good to look at for future work.

@@ -1,12 +1,13 @@
/a.js(3,15): error TS2304: Cannot find name 'sting'.
/a.js(3,15): error TS2552: Cannot find name 'sting'. Did you mean 'String'?
Copy link
Member

Choose a reason for hiding this comment

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

hmm, this also needs to suggest string not String. We might need to improve the suggestion code, perhaps with a special case for string/number/boolean/object (and any others?).

Copy link
Member

@sandersn sandersn Sep 30, 2021

Choose a reason for hiding this comment

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

Looks like string/number/boolean/object/bigint etc are missing from the list of global suggestions.
Edit: That's because the intrinsic types are resolved via direct lookup and don't have symbols.

@@ -3080,7 +3080,7 @@ namespace ts {
/**
* Resolves a qualified name and any involved aliases.
*/
function resolveEntityName(name: EntityNameOrEntityNameExpression, meaning: SymbolFlags, ignoreErrors?: boolean, dontResolveAlias?: boolean, location?: Node): Symbol | undefined {
function resolveEntityName(name: EntityNameOrEntityNameExpression, meaning: SymbolFlags, ignoreErrors?: boolean, dontResolveAlias?: boolean, location?: Node, needSuggestedNameNotFoundMessage?: boolean): Symbol | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

what happens if you don't add needSuggestedNameNotFoundMessage and always request suggestions? It only makes a difference for resolveName calls that pass the combination nameNotFoundMessage = undefined and issueSuggestions = true. I'll go see if any calls like that exist.

Edit: I checked. The only new places that would issue suggestions is resolution for globals and JSX built-in symbols, only if they already request errors. This is fine. You can delete needSuggestedNameNotFoundMessage and issueSuggestions and unconditionally issue suggestions in resolveName if message is defined.

suggestedNameNotFoundMessage = Diagnostics.Cannot_find_name_0_Did_you_mean_1;
}
}
symbol = getMergedSymbol(resolveName(location || name, name.escapedText, meaning, ignoreErrors || symbolFromJSPrototype ? undefined : message, name, /*isUse*/ true, false, suggestedNameNotFoundMessage));
Copy link
Member

Choose a reason for hiding this comment

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

resolveName now takes a boolean issueSuggestions instead of a message. I think it's probably fine to move your new code to choose the message inside resolveName.

PR Backlog automation moved this from Waiting on reviewers to Waiting on author Sep 29, 2021
@sandersn
Copy link
Member

sandersn commented Sep 30, 2021

Update: I tried unconditionally suggesting errors and it works fine. I pushed the change with changed baselines to the branch always-suggest-spelling-correction

sandersn added a commit that referenced this pull request Sep 30, 2021
Even for types. Based on #42126, but without the namespace-specific
error message.
@chenjigeng
Copy link
Contributor Author

@sandersn If you have time, please help me to complete this PR, I have been busy recently

PR Backlog automation moved this from Waiting on author to Needs merge Oct 5, 2021
@sandersn sandersn merged commit d60747f into microsoft:main Oct 5, 2021
PR Backlog automation moved this from Needs merge to Done Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

No spelling suggestion/quick fix for types
3 participants