Convert to named parameters

[gabritto]

Fixes #23552

Convert to "named parameters" refactor

A function-like declaration with parameters is refactored to receive a single parameter object with properties corresponding to its old parameters. Thus a function's arguments can be named when calling it. This applies to functions, methods, constructors, arrow functions and function expressions.

Example:

function add(a: number, b: number, c: number): number {
    return a + b + c;
}

would be refactored to

function add({ a, b, c }: { a: number, b: number, c: number }): number {
    return a + b + c;
}

Refactor availability

The refactor will be listed as available on:

• functions
• methods
• constructors for class declarations that have names or for class expressions which are assigned to a const variable without a type annotation
• arrow functions and function expressions that initialize a const variable without type annotation
  The refactor will not be available on:
• overloads
• functions with less than 2 parameters
• functions having binding patterns in its parameter declarations
• constructors with parameters that have properties, e.g.

class Foo {
  constructor(private a: string) {}
}

References

This refactor changes a function's signature, so it affects the way a function should be called either directly or indirectly. The refactor also changes assignability to and from a function's type, since its type will change.
Because of that, there are several situations where performing the refactor could cause a breaking change that would in the best scenario result in type errors or, in the worst scenario, runtime errors and wrong behavior.
The current solution to this problem is to adopt a conservative approach: the refactor will perform changes only if those changes are guaranteed not to break the code. Otherwise, it will not perform any change.
The refactor has to look at all the references of a function (through findAllReferences) to determine if changing it is safe. If a constructor is being refactored, the refactor also looks at all the references of the parent class declaration or class expression.
However, finding all references is too slow to be performed when the user is checking what are the available refactors. So the refactor will only look at all references if the user tries to apply it.
Therefore, the refactor can fail to perform changes even if it was listed as available and the user tried to applied it. Currently there is no way to inform a user why a refactor has failed.

• Future Work: create a mechanism for informing the user why a refactor failed.
  Related issue: Extend tsserver interface to allow refactoring failure reporting #28410

Good cases

Direct calls

The refactor can change direct calls to functions to conform to the new function signature. This is done by taking the old argument list and replacing it with an argument object whose properties have values corresponding to the old arguments.
Example:
1.

function foo(a: number, b: number) { }
foo(1, 2);

becomes

function foo({ a, b }: { a: number; b: number; }) { }
foo({ a: 1, b: 2 });

const foo = function(a: number, b: number) { };
foo(1, 2);

becomes

const foo = function({ a, b }: { a: number; b: number; }) { };
foo({ a: 1, b: 2 });

class Foo {
    bar(t: string, s: string): string { }
}
var foo = new Foo();
foo["bar"]("a", "b");
foo.bar("a", "b");

becomes

class Foo {
    bar({ t, s }: { t: string; s: string; }): string {
        return s + t;
    }
}
var foo = new Foo();
foo["bar"]({ t: "a", s: "b" });
foo.bar({ t: "a", s: "b" });

Imports & Exports

Since findAllReferences can track imports/exports across a project, importing/exporting the refactored function should be allowed.

• Future Work: recognize import/export references.
  Update: done in Handle imports and exports in 'convert parameters to destructured object' #30475.

Other good cases

• Future Work: think about what other usages of the refactored function should be allowed.

Good class cases

If a constructor is being refactored, the refactor will also check if all the references to the corresponding class are valid usages.
Currently, for a class declaration (class C { ... }), the following usages are recognized:

1. New calls:

class C { }

const c = new C();

2. Property access and element access:

class C {
    static a: number = 2;
}

const b = C.a;
C["a"] = 3;

3. Type references and heritage clauses

class C { }
let c: C;
function f(c: C) { }
class B extends C { }
class A implements C { }

For a class expression (const c = class { ... }), the following usages are recognized:

1. New calls:

const c = class C { }

const a = new c();

2. Property access and element access:

const c = class C {
    static a: number = 2;
}

const b = c.a;
c["a"] = 3;

JSDoc

Currently the refactor will not do anything with JSDoc, it will treat it as regular comments.

1. Non-inline JSDoc

Example:

/**
 * @param name person's name
 * @param address person's address
 */
function getPerson(name: string, address: string) { }

should become something like

/**
 * @param {object} obj parameter object
 * @param obj.name person's name
 * @param obj.address person's address
 */
function getPerson({ name, address }: { name: string, address: string }) { return name + address; }

• Future Work: non-inline JSDoc on the refactored function should be reorganized as above to reflect the parameter change.
  There are also issues (JSDoc support for destructured parameters #11859, JSDoc comment for destructuring param: description text not displayed #24746) that should be resolved to support the JSDoc syntax with destructuring in both JS and TS.

2. Inline JSDoc

Example:

function getPerson(/** person's name*/ name: string, /** person's address*/ address: string) { }

Since the parameter names above will become binding elements in a destructuring pattern, we cannot inline the existing JSDoc,
so the refactor will break inline JSDoc.

• Future Work: think about how the refactor should behave in this case.
  Question: Should it just break the JSDoc? Try to convert it to non-inline JSDoc?

Rest parameters

The refactor works for rest parameters as illustrated below. The rest parameter will be converted to an optional property of the new parameter object, and the corresponding variable will be initialized to an empty array. This ensures that the variable is not undefined in case no rest arguments are provided, which reflects how rest parameters behave.

Example:

function buildName(firstName: string, middleName: string, ...restOfName: string[]) {
    return firstName + " " + middleName + " " + restOfName.join(" ");
}

let employeeName = buildName("Joseph", "Samuel", "Lucas", "MacKinzie");

becomes

function buildName({ firstName, middleName, restOfName = [] }: { firstName: string, middleName: string, restOfName?: string[] }) {
    return firstName + " " + middleName + " " + restOfName.join(" ");
}

let employeeName = buildName({ firstName: "Joseph", middleName: "Samuel", restOfName: ["Lucas", "MacKinzie"]);

This parameter

A function can have a this parameter as its first parameter. In this case, the refactor should ignore this parameter and only modify the other parameters.

Example:

function f(this: void, a: string, b: string) { }
f("a", "b");

becomes

function f(this: void, { a, b }: { a: string, b: string }) { }
f({ a: "a", b: "b" });

Parameters with initializers

A parameter with an initializer is considered an optional member of the new parameter type. If a parameter has an initializer but has no type annotation, its type is inferred (using checker.getTypeAtLocation(node)).

Example:

function add(a: number, b = 1) {
    return a + b;
}

should be

function add({ a, b = 1 }: { a: number, b?: number }) {
    return a + b;
}

Optional parameters

When a parameter is optional (when it is followed by a ? or has an initializer), the corresponding property in the generated parameter object type should also be optional.
In addition, if all parameters are optional, the new parameter object should be optional. This is achieved by adding an empty object initializer.

Example:

function foo(a: number, b?: number) { return a; }
foo(1);

becomes

function foo({ a, b }: { a: number; b?: number; }) { return a; }
foo({ a: 1 });

and

function bar(a?: string, b?: string) { }
bar();

becomes

function bar({ a, b }: { a?: string; b?: string; } = {}) { }
bar();

JavaScript

Currently the refactor is not available on a JavaScript file, but we would like the refactor to work on JavaScript as well. Essentially the difference in the refactor would be the omission of type annotations.

Example:

function foo(a, b) { return a + b; }

becomes

function foo({ a, b }) { return a + b; }

JS code can be harder to analyze and to validate the refactor's changes.
So, if the refactor does something wrong, it would be harder for the user to notice.
If the refactored function has annotations, it becomes easy to apply the refactor.

Some bad cases

Aliasing

As shown above, aliasing makes it not possible to track down all calls and usages of the refactored function.

Example:

const foo = (a: number, b: number) => a + b;
foo(1, 2);
var otherFoo = foo;
otherFoo(1, 2);

after the refactor:

const foo = ({ a, b }: { a: number; b: number; }) => a + b;
foo({ a: 1, b: 2 });
var otherFoo = foo;
otherFoo(1, 2); // Error: Expected 1 arguments, but got 2.

Other example:

class Foo {
    constructor(s: string, t: string) { }
}
var c = Foo;
var f = new c("a", "b");

after refactoring the constructor:

class Foo {
    constructor({ s, t }: { s: string; t: string; }) { }
}
var c = Foo;
var f = new c("a", "b"); // Error: Expected 1 arguments, but got 2.

Class expression example:

const foo = class {
    constructor(a: number, b: number) { }
}
class Bar extends foo {
    constructor() {
        super(1, 2);
    }
}

after refactoring the constructor:

const foo = class {
    constructor({ a, b }: { a: number; b: number; }) { }
}
class Bar extends foo {
    constructor() {
        super(1, 2); // Error: Expected 1 arguments, but got 2.
    }
}

Types

Since the refactor changes the type of the function, there might be type errors.
Example:

function add(a: number, b: number) { return a + b; }
function sum(fn: (a: number, b: number) => number, ...numbers: number[]) {
    let total = 0;
    for (let n of numbers) {
        total += n;
    }
    return total;
}
var x: (a: number, b: number) => number = add;
var y = sum(add, 1, 2, 3);

becomes:

function add({ a, b }: { a: number, b: number }) { return a + b; }
function sum(fn: (a: number, b: number) => number, ...numbers: number[]) {
    let total = 0;
    for (let n of numbers) {
        total += n;
    }
    return total;
}
var x: (a: number, b: number) => number = add; // error on x: type ... is not assignable to type ...
var y = sum(add, 1, 2, 3); // error on add: argument of type ... is not assignable to parameter of type ...

Nameless declarations (anonymous functions/classes)

Anonymous functions and classes are tricky when looking for references.
Example:

var foo = (a: number, b: number) => a + b;
foo(1, 2);
// ... more code ...
foo = (a: number, b: number) => a - b; 
foo(1, 2);

In the example above, if we refactor the arrow function assigned to foo on line 1, then we should refactor the call on line 2. But later on foo is reassigned, so this will cause a type error. The same goes for constructors of class expressions.

bind/call/apply calls

Calling bind, call or apply on a function that has been refactored might go wrong.
Example:

function add(a: number, b: number, c: number) { return a + b + c; }
let add1 = add.bind(null, 1);
let z = add1(2, 3);
let x = add.call(null, 1, 2, 3);
let y = add.apply(null, [1, 2, 3]);

becomes

function add({ a, b, c }: { a: number, b: number, c: number }) { return a + b + c; }
let add1 = add.bind(null, 1);
let z = add1(2, 3); // NaN or Argument not assignable to parameter
let x = add.call(null, 1, 2, 3); // NaN or Expected 2 arguments, but got 4
let y = add.apply(null, [1, 2, 3]); // NaN or Type 'number' is not assignable to type '{ a: ... }'

The compiler will emit the errors above only if strictBindCallApply is on.

Overloads

A function which has overloads is tricky to refactor. Changing a function overload or a function implementation which has overloads can be a breaking change. The overloads and the implementation should be considered together when refactoring. It is not clear what the refactor should do with overloads, so the refactor will not be available in that case.

Example:

function foo(a: number, b: number): number;
function foo(a: string, b: number): string;
function foo(a: string | number, b: number): string | number {
    // implementation
    return 0;
}

might become

function foo(a: number, b: number): number; // Overload signature is not compatible with function implementation
function foo(a: string, b: number): string;
function foo({ a, b }: { a: string | number, b: number }): string | number {
    // implementation
    return 0;
}

or

function foo(a: number, b: number): number;
function foo({ a, b }: { a: string, b: number }): string; // Overload signature is not compatible with function implementation
function foo(a: string | number, b: number): string | number {
    // implementation
    return 0;
}

Overrides

As with overloads, refactoring overrided methods can also be a breaking change. Refactoring a method which overrides another can mean that the refactored method's type is no longer assignable to the type of the method it overrides. Similarly, changing a method which is overriden by another can mean that the overriding methods' types are no longer assignable to the refactored method.

Example:

class A { 
    foo(a: string, b: string) { }
}
class B extends A { 
    foo(c: string, d: string) { }
}

would become

class A { 
    foo({ a, b }: { a: string, b: string }) { }
}
class B extends A { 
    foo(c: string, d: string) { } // error: property 'foo' in type B is not assignable to the same property in base type 'A'
}

That scenario is currently avoided by the refactor. Once it checks all the references and finds a declaration different than the one where the refactor should be applied, it does not perform any changes.

Inference

When a function is refactored to have an object parameter instead of a list of parameters, inference can no longer proceed left-to-right on the parameter list, so the compiler no longer reuses inferences from previous parameters for the next parameter.
Because of that, inference can become worse and cause breaks in rare cases (see #29791).

Known issues

Formatting

In some cases involving comments and line breaks, the refactor generates an argument object that is ill-formatted.
The refactorConverToNamedParameters_callComments2.ts test is currently wrong because of that.

Inherited constructors

Currently findAllReferences will not track usage of an inherited constructor.

Example:

class Foo {
    constructor(t: string, s: string) { }
}
class Bar extends Foo { }
var bar = new Bar("a", "b");

refactoring Foo's constructor, becomes:

class Foo {
    constructor({ t, s }: { t: string; s: string; }) { }
}
class Bar extends Foo { }
var bar = new Bar("a", "b"); // Error: Expected 1 arguments, but got 2.

The refactorConvertToNamedParameters_inheritedConstructor.ts test is currently wrong because of that.
However, findAllReferences tracks inherited methods, which causes the refactor to behave differently in similar situations when it should behave the same. This leads me to believe that findAllReferences should be able to find references to inherited constructors.
Update: solved in #30514.

Type errors when refactoring a method

When a class/object literal's method is refactored, its type changes.
Because of that, we can get type errors where the new class/object literal type is not compatible with the expected type.
Example:

class Bad {
    fn(a: number, b: number) {
        return a + b;
    }
}

interface Expected {
    fn(a: number, b: number): number;
}

declare var e: Expected;
declare var b: Bad;

e = b;
b = e;

After refactoring fn on Bad:

class Bad {
    fn({ a, b }: { a: number; b: number; }) {
        return a + b;
    }
}

interface Expected {
    fn(a: number, b: number): number;
}

declare var e: Expected;
declare var b: Bad;

e = b; // Error: Type 'Bad' is not assignable to type 'Expected'.
b = e; // Error: Type 'Expected' is not assignable to type 'Bad'.

Open questions

Type Literal or Interface

Should the refactor generate a type literal for the new parameter object or an interface?
In the case where this refactor generates a type literal,
this refactor should be composed with a refactor for extracting a type literal into an interface.

Example:

function add(a: number, b: number, c: number): number {
    return a + b + c;
}

could be refactored to

function add({ a, b, c }: { a: number, b: number, c: number }): number {
    return a + b + c;
}

or

interface AddOptions {
    a: number;
    b: number;
    c: number;
}

function add({ a, b, c }: AddOptions) {
    return a + b + c;
}

Name

What should the refactor be called?
Currently it is 'named parameters', but 'parameter object' was suggested.
Update: done in #30401.

Dealing with bad cases

The refactor's current approach is to not apply changes when it finds a reference it does not recognize.
This approach comes from the idea that a refactor should not break user's code, i.e. it should 'do the right thing'.
So, when the refactor does not know the right thing to do, it does nothing.
Some cons:

• This approach is very restrictive and will cause the refactor to not do anything in many cases, possibly to the user's frustration.
• It requires that we recognize what the "good usages" are and add code for each one.
  The question is: is this the best approach?
  In some situations, the refactor could break the user's code, and the breaking changes can be detected by the type checker or not.
  But if the user then has type errors that indicate all the parts of the code that have to be fixed,
  could it be better to apply the refactor and have the user do the remaining necessary changes?

Good cases

For the current approach, what are other usages of the refactored function that the refactor should recognize?

JavaScript

If the JavaScript code is not type annotated, what should we do?

[amcasey]

This is definitely optional, but it would be very nice if we simplified x: x to x. I think passing variables with the same name as parameters is a fairly common pattern.

[yortus]

Another 'bad case' to note: #29791. For generic functions, tsc currently currently makes assumptions about parameter order to infer the parameter types. With a named params object this ordering is lost, so type inference may fail on the refactored version.

[amcasey]

Questions about textChanges

[amcasey]

This is the only place I can see TrailingTriviaOption.IncludeIfLineBreak being consumed and I don't understand how it relates to expressions.

[gabritto]

I don't know why it is necessary to check if node is an expression, but for the behavior I needed for this function (when the TrailingTriviaOption.Include is used), it should not matter if node is an expression. So I just tried not to break existing code. The same goes for the isLineBreak check below, I don't know what is the reasoning behind it.

[amcasey]

I think you got rid of IncludeIfLineBreak.

[amcasey]

Comments about utilities.ts.

[amcasey]

Initial comments on convertToNamedParameters.

[amcasey]

How did you decide which helper functions would be nested and which would be siblings?

[RyanCavanaugh]

The ones that don't close over anything should be moved outside to avoid excess closure allocations.

[amcasey]

Didn't getDeclarationNames return the class name? I would have thought it was already incorporated into references.

[gabritto]

I separated getDeclarationNames into getClassNames and getFunctionNames, and now there's a single pass through the references which checks for both function and (if necessary) class references, so the code flow should be more clear now.

[sandersn]

Looks good so far. I've only reviewed convertToNamedParameters so far.

[sandersn]

Can you explain why we need to suppress trivia and then copy comments instead of just using newThisParameter.name as-is?

[gabritto]

If we don't suppress trivia, trailing comments will be emitted but leading comments will disappear. In essence, I do not know what behavior to expect from trivia when reusing nodes. So I am suppressing both leading and trailing and explicitly copying the comments.

[amcasey]

I believe you're still polishing but it generally LGTM.

[amcasey]

Why do we need the class name if there's a variable name? Is that for ctor calls within the class itself?

[gabritto]

yes, that's for checking the references to the class within the class expression itself.

[amcasey]

I'm not sure I understand why there would be duplicates.

[gabritto]

when refactoring the constructor of a class expression, the variable name to which the class is assigned is both a function name and a class name.

[sandersn]

is this tracked by a bug? It's a good idea to file one after merging this PR.

[RyanCavanaugh]

Looks great overall. Polling the issue on naming so hold off on changing that, but please do move the functions out one level where appropriate.

[RyanCavanaugh]

I think we agreed on "Convert to parameters object" ?

[RyanCavanaugh]

The ones that don't close over anything should be moved outside to avoid excess closure allocations.