Convert function into ES2015 class quickfix returns empty result

[mjbvz]

From microsoft/vscode#81409

TypeScript Version: 3.7.0-dev.20190924

Search Terms:

• code action
• quick fix
• convert function to class
• tsserver
• getCodeFixes

Code
For the js

(function() {
    let x = () => {
        this.y = 1;
    };
})();

Try running the suggested convert function into class refactoring

Bug:
Nothing happens.

This is because TS Server returns a response with no changes:

[Trace  - 10:43:25 AM] <semantic> Sending request: getCodeFixes (45). Response expected: yes. Current queue length: 0
Arguments: {
    "file": "/Users/matb/projects/san/index.js",
    "startLine": 1,
    "startOffset": 2,
    "endLine": 1,
    "endOffset": 10,
    "errorCodes": [
        80002
    ]
}
[Trace  - 10:43:25 AM] <semantic> Response received: getCodeFixes (45). Request took 1 ms. Success: true 
Result: [
    {
        "fixName": "convertFunctionToEs6Class",
        "description": "Convert function to an ES2015 class",
        "changes": [],
        "fixId": "convertFunctionToEs6Class",
        "fixAllDescription": "Convert all constructor functions to classes"
    }
]

I believe we can filter this out on the VS Code side but it is odd that this result is included

[mjbvz]

Note that we probably just can't drop this result entirely on the VS Code as the UX would be weird: users will still see the ... suggestion action but no associated code actions will show up if they try to trigger it

[LWChris]

I don't know if it is of any relevance, but I noticed the quick fix isn't suggested without an assigment to a member of this inside x, e. g. if you change this.y = 1; into let y = 1;

[a-tarasyuk]

I suppose the expected fix for this issue is skipping QF suggestion. Is it true? Or need to generate class code?
BTW: In TS file there is no QF for this case 😄.

cc @DanielRosenwasser @orta @mjbvz

[orta]

Looking at tests:

• This seems to be JS only (tests either have allowNonTsExtensions: true or allowJs: true
• The signal is either that the function mutates its this or has it's prototyped edited

e.g. we have a test like this:

// @allowJs: true
// @Filename: /a.js
/////** Doc */
////const C = function() { this.x = 0; }

verify.codeFix({
    description: "Convert function to an ES2015 class",
    newFileContent:
`
/** Doc */
class C {
    constructor() { this.x = 0; }
}
`,
});

and

/// <reference path='fourslash.ts' />

// @allowNonTsExtensions: true
// @Filename: test123.js
////export function /**/MyClass() {
////}
////MyClass.prototype.foo = function() {
////}

verify.codeFix({
    description: "Convert function to an ES2015 class",
    newFileContent:
`export class MyClass {
    constructor() {
    }
    foo() {
    }
}
`,
});

My gut stays maybe the IIFE is making the response not work as expected (not tested) but I'd expect a test to look something like this:

/// <reference path='fourslash.ts' />

// @allowNonTsExtensions: true
// @Filename: test123.js
////(function() {
////    let /**/x = () => {
////        this.y = 1;
////    };
////   x;
////})();

verify.codeFix({
    description: "Convert function to an ES2015 class",
    newFileContent:
`(function() {
    class x {
        constructor() {
            this.y = 1;
        }
    }
    x
})();
`,
});

[a-tarasyuk]

@orta About the last sample, I think we don't need to convert arrow function to class because AF doesn't have its own this and this refers to the parent function.

const /**/x = function () {
    const y = () => {
        this.x = 10;
    }
}

After

class x {
    constructor() {
        const y = () => {
            this.x = 10;
        };
    }
}

The question is, do we need to suggest QF for IIEF which causes the issue. I think no because there is no name that should be used as for the name. What do you think? Does that make sense?

[orta]

Great points!

Yeah, the arrow funds is a good point. I also agree about the IIFE, I think for the sample it shouldn't offer the fixits for that. So the test would be that the sample doesn't return that quick fix IMO