Add reason for a disabled code action #37871

PranavSenthilnathan · 2020-04-09T18:45:54Z

This PR adds the error property on refactors so that the editor can consume it and show the user why a refactor does not apply in the case. The extract symbol refactor already has error messages so that's already wired up but these are the other refactors that need messages:

Refactors that currently have too simple or too complex error messages:

convertParamsToDestructuredObject
moveToNewFile

Starting this as a draft PR because of these 2 questions:

The messages in the extractSymbol.ts have this comment:
// Move these into diagnostic messages if they become user-facing
Is this just moving it into src\compiler\diagnosticMessages.json?
The errors are Diagnostics. Is there a standard way to stringify a Diagnostic?

jessetrinity · 2020-04-09T21:24:19Z

Overall looks good so far.

Do we intend to also include the Diagnostic messages from getRangeToExtract? Not sure if they would be useful, but they might be if we have requests for specific refactors.

jessetrinity · 2020-04-09T21:29:21Z

src/services/refactors/extractSymbol.ts

                if (!usedFunctionNames.has(description)) {
                    usedFunctionNames.set(description, true);
                    functionActions.push({
                        description,
                        name: `function_scope_${i}`
                    });
                }
+            } else if (!innermostErrorFunctionAction) {


I guess the "innermost" error is the only one we care about?

Yep, this is probably the most relevant since it's lexically closest to problem.

For microsoft/TypeScript#37871

mjbvz · 2020-04-11T00:09:11Z

microsoft/vscode@f3ceb33 in VS Code adds support for an .error property on RefactorActionInfo. This should let you test the UX of this in VS Code

You'll just need to make sure that .error is sent along by the TS Server. The next VS Code insiders should be able to interpret that value

andrewbranch · 2020-06-18T23:18:32Z

Are these messages intended to be shown to a user? If so, they need to go in diagnostics.json so they can be localized.

Also, I realize this came from @mjbvz’s proposal so if the editor folks have a strong opinion you can disregard—but the terminology error was a little misleading to me in this context, since it has nothing to do with compiler diagnostic errors and it doesn’t indicate that anything went wrong; it’s not like the refactor threw an error and had to abort.

jessetrinity · 2020-06-19T21:46:23Z

src/compiler/diagnosticMessages.json

+        "category": "Message",
+        "code": 95136
+    },
+    "Property_has_invalid_accessibility": {


The property needs to have any accessibility modifier or static or readonly right? If so, does it make sense to say "Can only convert property with modifier"?

jessetrinity · 2020-06-19T21:56:24Z

src/services/codefixes/generateAccessors.ts

+        }
+
+        if (!(nodeOverlapsWithStartEnd(declaration.name, file, start, end) || cursorRequest)) {
+            return undefined;


This should just check that the span is in a sensible place to offer the refactor. Should we return the "Could not find property" message here?

jessetrinity

I have a couple of comments about the messages themselves but otherwise looks good.

amcasey

Mostly nitpicking

amcasey · 2020-06-22T20:49:40Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

@@ -70,28 +98,45 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
        return { renameFilename: undefined, renameLocation: undefined, edits };
    }

-    function getConvertibleArrowFunctionAtPosition(file: SourceFile, startPosition: number, considerFunctionBodies = true): Info | undefined {
+    function getConvertibleArrowFunctionAtPosition(file: SourceFile, startPosition: number, considerFunctionBodies = true): InfoOrError | undefined {


Can this still return undefined? When?

amcasey · 2020-06-22T20:53:34Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

@@ -15,33 +15,61 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
        addBraces: boolean;
    }

+    type InfoOrError = {


Maybe this type should be generic and reused?

amcasey · 2020-06-22T20:54:49Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+        info?: never,
+        error: string
+    };
+
    function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] {
        const { file, startPosition, triggerReason } = context;
        const info = getConvertibleArrowFunctionAtPosition(file, startPosition, triggerReason === "invoked");


Personally, I'd take the hit and rename the outer info so we don't have info.info everywhere. Maybe result?

amcasey · 2020-06-22T20:55:56Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+
+        if (!func) {
+            return {
+                error: getLocaleSpecificMessage(Diagnostics.Could_not_find_a_containing_arrow_function)


Or any other sort of containing function?

amcasey · 2020-06-22T20:58:08Z

src/services/codefixes/generateAccessors.ts

+        error?: never
+    } | {
+        info?: never,
+        error: string


Would DiagnosticMessage be narrower and lazier than string?

amcasey · 2020-06-22T21:00:39Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+                actions: [{
+                    name: addBracesActionName,
+                    description: addBracesActionDescription,
+                    notApplicableReason: info.error


Without having read the code in detail, it seems intuitively unlikely to me that add-braces and remove-braces would fail for the same reason.

amcasey · 2020-06-22T21:02:46Z

src/services/refactors/convertImport.ts

+
+            if (context.preferences.provideRefactorNotApplicableReason) {
+                return [
+                    { name: refactorName, description: Diagnostics.Convert_namespace_import_to_named_imports.message, actions: [{ name: actionNameNamespaceToNamed, description: Diagnostics.Convert_namespace_import_to_named_imports.message, notApplicableReason: i.error }] },


How is .message different from getLocaleSpecificMessage?

amcasey · 2020-06-22T21:03:59Z

src/services/refactors/convertImport.ts

        const { file } = context;
        const span = getRefactorContextSpan(context);
        const token = getTokenAtPosition(file, span.start);
        const importDecl = considerPartialSpans ? findAncestor(token, isImportDeclaration) : getParentNodeInSpan(token, file, span);
-        if (!importDecl || !isImportDeclaration(importDecl) || (importDecl.getEnd() < span.start + span.length)) return undefined;
+        if (!importDecl || !isImportDeclaration(importDecl)) return { error: "Selection is not an import declaration." };
+        if (importDecl.getEnd() < span.start + span.length) return undefined;


Why no error in this case?

amcasey · 2020-06-22T21:07:50Z

src/services/refactors/extractSymbol.ts

            });
        }

        return infos.length ? infos : emptyArray;
+
+        function getStringError(errors: readonly Diagnostic[]) {


What does this do? I thought all refactoring errors were strings.

amcasey · 2020-06-22T21:09:29Z

tests/baselines/reference/api/tsserverlibrary.d.ts

@@ -3789,6 +3789,7 @@ declare namespace ts {
        readonly importModuleSpecifierEnding?: "auto" | "minimal" | "index" | "js";
        readonly allowTextChangesInNewFiles?: boolean;
        readonly providePrefixAndSuffixTextForRename?: boolean;
+        readonly provideRefactorNotApplicableReason?: boolean;


I feel like I'm missing something obvious, but shouldn't there also be a protocol change for this?

* upstream/master: (58 commits) Variadic tuple types (microsoft#39094) chore: resolve suggestions Expand auto-import to all package.json dependencies (microsoft#38923) inline local functions Update bigint declaration file (microsoft#38526) Update user baselines (microsoft#39077) LEGO: check in for master to temporary branch. Add missing index.ts files to user projects (microsoft#39163) Add reason for a disabled code action (microsoft#37871) Minor fix for assertion predicates (microsoft#38710) Update LKG (microsoft#39173) Reparse top level 'await' in modules (microsoft#39084) change chore: more change chore: resolve review chore: save space fix: lint error test: add test for it chore: make isJsxAttr required chore: revert change in checker ... # Conflicts: # src/compiler/binder.ts # src/compiler/checker.ts # src/compiler/parser.ts # src/compiler/types.ts

* add reason for a disabled code action * add addOrRemoveBracesToArrowFunction * use user preferences * add error reason * accept baseline changes * fix lint rules * move messages to diagnosticMessages.json * rename 'error' to 'notApplicableReason' * accept baseline change * address comments

Adopts changes from microsoft/TypeScript#37871

add reason for a disabled code action

bd9f3cf

PranavSenthilnathan requested a review from jessetrinity April 9, 2020 18:46

jessetrinity reviewed Apr 9, 2020

View reviewed changes

mjbvz added a commit to microsoft/vscode that referenced this pull request Apr 11, 2020

Hook up experimental TS refactor error

f3ceb33

For microsoft/TypeScript#37871

PranavSenthilnathan added 4 commits May 1, 2020 12:20

add addOrRemoveBracesToArrowFunction

ecb957d

use user preferences

dd17741

merge

8684d36

add error reason

93723fb

PranavSenthilnathan marked this pull request as ready for review June 18, 2020 20:21

PranavSenthilnathan requested review from andrewbranch, amcasey and sheetalkamat June 18, 2020 20:26

PranavSenthilnathan added 2 commits June 18, 2020 13:58

accept baseline changes

a57100e

fix lint rules

569c6f8

PranavSenthilnathan added 4 commits June 19, 2020 12:33

move messages to diagnosticMessages.json

6b6d970

rename 'error' to 'notApplicableReason'

8ae8304

merge

e86312c

accept baseline change

7734694

jessetrinity reviewed Jun 19, 2020

View reviewed changes

jessetrinity approved these changes Jun 19, 2020

View reviewed changes

address comments

cf992ad

PranavSenthilnathan merged commit 2f79389 into microsoft:master Jun 22, 2020

amcasey reviewed Jun 22, 2020

View reviewed changes

mjbvz added a commit to microsoft/vscode that referenced this pull request Jul 2, 2020

Update VS Code refactoring support for new TS 4.0 api

2ce03ee

Adopts changes from microsoft/TypeScript#37871

jessetrinity mentioned this pull request Sep 21, 2020

fix(40640) add missing notApplicableReason in protocol #40680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reason for a disabled code action #37871

Add reason for a disabled code action #37871

PranavSenthilnathan commented Apr 9, 2020 •

edited

Loading

jessetrinity commented Apr 9, 2020 •

edited

Loading

jessetrinity Apr 9, 2020

PranavSenthilnathan Jun 18, 2020

mjbvz commented Apr 11, 2020

andrewbranch commented Jun 18, 2020

jessetrinity Jun 19, 2020

jessetrinity Jun 19, 2020

jessetrinity left a comment

amcasey left a comment

amcasey Jun 22, 2020

amcasey Jun 22, 2020

amcasey Jun 22, 2020

amcasey Jun 22, 2020

amcasey Jun 22, 2020

amcasey Jun 22, 2020

amcasey Jun 22, 2020

amcasey Jun 22, 2020

amcasey Jun 22, 2020

amcasey Jun 22, 2020

Add reason for a disabled code action #37871

Add reason for a disabled code action #37871

Conversation

PranavSenthilnathan commented Apr 9, 2020 • edited Loading

jessetrinity commented Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjbvz commented Apr 11, 2020

andrewbranch commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessetrinity left a comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PranavSenthilnathan commented Apr 9, 2020 •

edited

Loading

jessetrinity commented Apr 9, 2020 •

edited

Loading