getApplicableRefactors returns "Install missing types package" quickfix

[mjbvz]

TypeScript Version: 2.6.1-insiders.20171019

Code

In a empty project with a package.json that includes lodash but not @types/lodash. Try writing an import:

import _ from 'lodash'

Request quickfixs on 'lodash'

Bug

getApplicableRefactors is also returning a quick fix here for Install missing types package

Trace  - 11:13:01 AM] Sending request: getApplicableRefactors (51). Response expected: yes. Current queue length: 0
Arguments: {
    "file": "/Users/matb/projects/san/src/test.ts",
    "startLine": 1,
    "startOffset": 15,
    "endLine": 1,
    "endOffset": 23
}
[Trace  - 11:13:01 AM] Response received: getApplicableRefactors (51). Request took 2 ms. Success: true 
Result: [
    {
        "name": "Install missing types package",
        "description": "Install missing types package",
        "actions": [
            {
                "description": "Install '@types/lodash'",
                "name": "install"
            }
        ]
    }
]
[Trace  - 11:13:01 AM] Sending request: getCodeFixes (52). Response expected: yes. Current queue length: 0
Arguments: {
    "file": "/Users/matb/projects/san/src/test.ts",
    "startLine": 1,
    "startOffset": 15,
    "endLine": 1,
    "endOffset": 23,
    "errorCodes": [
        2307
    ]
}
[Trace  - 11:13:01 AM] Response received: getCodeFixes (52). Request took 2 ms. Success: true 
Result: [
    {
        "description": "Install '@types/lodash'",
        "changes": [],
        "commands": [
            {
                "type": "install package",
                "packageName": "@types/lodash"
            }
        ]
    }
]

[mjbvz]

I've added a workaround for this on the VS Code side but believe this should be addressed for 2.6.1. @mhegazy and @andy-ms Do you agree?

[ghost]

We intentionally exposed this both as a refactor and a quick fix. If a user has --noImplicitAny disabled they will see only the refactor because they will have no compile errors. (That might be a good reason to surface implicit-any problems as "info" even if --noImplicitAny is disabled.)
Is it a problem if they both appear? (Might it be related to microsoft/vscode#34481?) Should we add code to the refactor to do nothing if we think this will also show up as a quick fix?

[mjbvz]

Yes, this causes us to show the same code action twice, once from the quickfix for the error and once from the refactoring.

I believe that this is a quickfix and should not be returned as a refactoring. We previously used to send compute quickFix requests to TS even if there were no errors in the currently selected code, but disabled this for performance reasons in TS 2.4. Should we look into enabling this again?

[ghost]

The problem is that it is not a compile error to import from an untyped package unless the user has --noImplicitAny enabled. We want implicit-any users to still be able to take this action even if there aren't any errors to be fixed.

[mjbvz]

Yes that's what I'm trying to say. We currently don't even call getCodeFixes if there are no errors in the selected code:

	public async provideCodeActions(
		document: TextDocument,
		range: Range,
		context: CodeActionContext,
		token: CancellationToken
	): Promise<Command[]> {
		const file = this.client.normalizePath(document.uri);

		// This part was added when we picked up TS 2.4		
		const supportedActions = await this.getSupportedActionsForContext(context);
		if (!supportedActions.size) {
			return [];
		}

		const args: Proto.CodeFixRequestArgs = {
			...vsRangeToTsFileRange(file, range),
			errorCodes: Array.from(supportedActions)
		};
		const response = await this.client.execute('getCodeFixes', args, token);
		return (response.body || []).map(action => this.getCommandForAction(action, file));
	}

We can remove the size check so that we still call getCodeFixes even if there are no error codes.

[ghost]

If you ask for code fixes and there are no errors, we'll return an empty response, since there's nothing to fix. (The TS implementation of code fixes (getCodeFixesAtPosition in services.ts iterates over error codes.)

[mjbvz]

If we want to support the no implicit any case, I propose we change the getCodeFix to support empty error code requests. If that change would be too complex for 2.6, we can ship with the no implicit any case not being supported yet and look into handling it in a future release. However I feel that we need to remove this quick fix from the getApplicableRefactors response for 2.6

[mhegazy]

@mjbvz do you happen to have --strict in your project? or did you get this config file from tsc --init (which adds --strict)?

If so, i think i have a fix for this issue.

[mjbvz]

No, my tsconfig was:

{
    "compilerOptions": {
        "target": "ES6",
        "allowJs": true
    },
    "exclude": [
        "node_modules",
        "**/node_modules/*"
    ]
}

[ghost]

Longer-term solution might be #19392.

[mhegazy]

I am unable to reproduce this locally with the setup mentioned above with 1.18-insiders and latest TS.

@andy-ms , seems from your comments, you understand the issue here. can you elaborate on how i can reproduce this locally?

[ghost]

I don't get the quickfix at all in insiders -- didn't think that feature was in yet? Also, @mjbvz mentioned having added a workaround.
But from what I can tell, he is getting an error at the import for "lodash" because his module resolution is set to "classic" which can't find it; we provide a quickfix since the module import failed, but we probably shouldn't. We also provide a refactor because he doesn't have --noImplicitAny enabled. So we are providing duplicate actions (except, he already added a workaround into vscode).

[mhegazy]

aah.. now i see. why do we have the quick fix associated with Diagnostics.Cannot_find_module_0.code ? it should only be associated with Diagnostics.Could_not_find_a_declaration_file_for_module_0_1_implicitly_has_an_any_type.code.. or am i missing something?

[ghost]

That's exactly what #19394 does.

[mhegazy]

Sorry, was stuck trying to understand the issue and did not look at the change.

[mhegazy]

Fixed by #19394.

[mjbvz]

The original issue here was not addressed and the difference in semantics between quick fixes and refactorings is causing problems on our side (see microsoft/vscode#37288)

I still would like install @types to be returned only as a quick fix. See my comment on the PR for more info: #19394 (comment)

[mhegazy]

We are working on adding a new non-error diagnostic category, and moving all these refactoring to quick fix only. this should address the problem.