GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — noImplicitAny as suggestion by sandersn · Pull Request #27693 · microsoft/TypeScript

Conversation

Nathan Shively-Sanders (sandersn) commented Oct 10, 2018

Copy link

Copy Markdown

Member

With noImplicitAny suggestions, people will see suggestions for the infer-from-usage codefix even with noImplicitAny turned off. This applies to JS users, who are otherwise unlikely to see that codefix.

Notes:

I want to evaluate performance before merging this.
I want to make sure the suggestions are mostly reliable and sensical before merging as well. I'm going to look over our user tests.
Not all noImplicitAny errors turn into suggestions. In
particular:
1. reportErrorsFromWidening does not, because it doesn't log an error that infer-from-usage fixes, and fixing it would require otherwise-unnecessary code.
2. auto types do not have implicit any suggestions, because that would require running control flow in noImplicitAny mode.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          noImplicitAny as suggestion

e6a57f7

Note that not all noImplicitAny errors turn into suggestions. In
particular,

1. reportErrorsFromWidening does not, because it doesn't log an error
that infer-from-usage fixes, and fixing it would require
otherwise-unnecessary code.
2. auto types do not have implicit any suggestions, because that would
require running control flow in noImplicitAny mode.

Nathan Shively-Sanders (sandersn) requested a review from a user

October 10, 2018 22:31

Sheetal Nandi (sheetalkamat) reviewed

Oct 10, 2018

View reviewed changes

Comment thread

src/compiler/checker.ts Outdated

		}

		function reportImplicitAnyError(declaration: Declaration, type: Type) {
		function reportImplicitAnyError(noImplicitAny: boolean, declaration: Declaration, type: Type) {

Sheetal Nandi (sheetalkamat) Oct 10, 2018

Copy link

Copy Markdown

Member

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.

May be rename this to reportImplicitAnyErrorOrSuggestion instead?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sandersn reacted with thumbs up emoji

All reactions

👍 1 reaction

Deleted user (ghost) Oct 10, 2018

Copy link

Copy Markdown

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.

It's not necessary to take noImplicitAny as a parameter since that's already in scope. In every instance of calling this the argument is noImplicitAny; in one case the argument is true but it's within if (noImplicitAny).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 11, 2018

Copy link

Copy Markdown

Member Author

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 chose to shorten it to reportImplicitAny

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 11, 2018

Copy link

Copy Markdown

Member Author

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.

Andy (Andrewkraft) (@Andy-MS) good catch. I'll get rid of the parameter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sheetal Nandi (sheetalkamat) approved these changes

Oct 10, 2018

View reviewed changes

Deleted user (ghost) approved these changes

Oct 10, 2018

View reviewed changes

Deleted user (ghost) left a comment

Copy link

Copy Markdown

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.

Code looks good.
I'm thinking we should avoid reporting these even as suggestions in JS code that doesn't show an intent to be typed, i.e. doesn't use any types in its jsdoc.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) commented Oct 11, 2018

Copy link

Copy Markdown

Member Author

Andy (Andrewkraft) (@Andy-MS) I discussed this with Daniel Rosenwasser (@DanielRosenwasser) and we decided that, for now, the easiest and strongest signal of intent is --checkJS or // @ts-check. Once I improve the infer-from-usage inferences, I think we can re-evaluate showing suggestions for all other JS users.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Nathan Shively-Sanders (sandersn) added 2 commits

October 11, 2018 14:39


          Rename reportImplicitAny+forbid it for non-checkJS

c835e0c

In JS, you only get implicit any errors/suggestions with checkJS or
ts-check turned on.


          Update baselines

d2fad6a

Deleted user (ghost) reviewed

Oct 11, 2018

View reviewed changes

Comment thread

src/compiler/checker.ts Outdated

		function reportImplicitAnyError(noImplicitAny: boolean, declaration: Declaration, type: Type) {
		function reportImplicitAny(noImplicitAny: boolean, declaration: Declaration, type: Type) {
		const typeAsString = typeToString(getWidenedType(type));
		if (isInJSFile(declaration) && !getSourceFileOfNode(declaration).pragmas.has("checkJSDirective") && !compilerOptions.checkJs) {

Deleted user (ghost) Oct 11, 2018

Copy link

Copy Markdown

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.

Do we even get here without checkJS? Also, this must be duplicate code of something?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 11, 2018

Copy link

Copy Markdown

Member Author

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.

Yes, allowJS reports errors as usual, but doesn't actually display them.
Yes, isCheckJsEnabledForFile.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Oct 11, 2018

Copy link

Copy Markdown

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.

So, why go out of our way to not report this error, if no errors will be shown anyway?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 11, 2018

Copy link

Copy Markdown

Member Author

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.

Suggestions will still be shown, since they're stored in a separate array, and errorOrSuggestion at the bottom of the function will create a suggestion instead of an error in an allowJs file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Oct 11, 2018

Copy link

Copy Markdown

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.

#27852

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Use isCheckJsEnabledForFile

c59c4eb

Nathan Shively-Sanders (sandersn) commented Oct 11, 2018

Copy link

Copy Markdown

Member Author

Performance seems fine. I tried on webpack, which is a big project that has tons of implicit anys intermixed with existing jsdoc. I'm pretty sure the codefix calculations are correctly deferred to when the lightbulb is clicked.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Remove noImplicitAny parameter since it's in scope already

eea5553

Nathan Shively-Sanders (sandersn) merged commit ec0e8cb into master

Oct 11, 2018

Nathan Shively-Sanders (sandersn) deleted the noImplicitAny-as-suggestion branch

October 11, 2018 23:15

Klaus Meinhardt (ajafff) mentioned this pull request

Dec 5, 2018

Add rule that mimics --noImplicitAny compiler option fimbullinter/wotan#432

Closed

Microsoft (microsoft) locked as resolved and limited conversation to collaborators

Oct 21, 2025

TypeScript Bot (typescript-bot) assigned Nathan Shively-Sanders (sandersn)

Oct 21, 2025

Reviewers

Sheetal Nandi (sheetalkamat)

sheetalkamat approved these changes

+1 more reviewer

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Nathan Shively-Sanders (sandersn)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.