Skip to content

allow more cases to not block convert params to destructured object refactor #59140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

allow more cases to not block convert params to destructured object refactor #59140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a6a2fc
allow more cases to be restructured
iisaduan Jul 4, 2024
dacc5d2
update comment
iisaduan Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 52 additions & 25 deletions src/services/refactors/convertParamsToDestructuredObject.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
getTouchingToken,
getTypeNodeIfAccessible,
Identifier,
isCallExpression,
isCallOrNewExpression,
isClassDeclaration,
isConstructorDeclaration,
Expand Down Expand Up @@ -239,17 +240,21 @@ function getGroupedReferences(functionDeclaration: ValidFunctionDeclaration, pro
continue;
}
const call = entryToFunctionCall(entry);
if (call) {
groupedReferences.functionCalls.push(call);
if (call !== undefined) {
if (call !== true) {
groupedReferences.functionCalls.push(call);
}
continue;
}
}

const contextualSymbol = getSymbolForContextualType(entry.node, checker);
if (contextualSymbol && contains(contextualSymbols, contextualSymbol)) {
const decl = entryToDeclaration(entry);
if (decl) {
groupedReferences.declarations.push(decl);
if (decl !== undefined) {
if (decl !== true) {
groupedReferences.declarations.push(decl);
}
continue;
}
}
Expand All @@ -271,14 +276,18 @@ function getGroupedReferences(functionDeclaration: ValidFunctionDeclaration, pro
continue;
}
const decl = entryToDeclaration(entry);
if (decl) {
groupedReferences.declarations.push(decl);
if (decl !== undefined) {
if (decl !== true) {
groupedReferences.declarations.push(decl);
}
continue;
}

const call = entryToFunctionCall(entry);
if (call) {
groupedReferences.functionCalls.push(call);
if (call !== undefined) {
if (call !== true) {
groupedReferences.functionCalls.push(call);
}
continue;
}
}
Expand All @@ -290,8 +299,10 @@ function getGroupedReferences(functionDeclaration: ValidFunctionDeclaration, pro
}

const decl = entryToDeclaration(entry);
if (decl) {
groupedReferences.declarations.push(decl);
if (decl !== undefined) {
if (decl !== true) {
groupedReferences.declarations.push(decl);
}
continue;
}

Expand Down Expand Up @@ -355,47 +366,63 @@ function entryToImportOrExport(entry: FindAllReferences.NodeEntry): Node | undef
return undefined;
}

function entryToDeclaration(entry: FindAllReferences.NodeEntry): Node | undefined {
function entryToDeclaration(entry: FindAllReferences.NodeEntry): Node | true | undefined {
// returns a callOrNewExpression if the reference needs to be updated during the refactor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think technically this returns a Node whose parent is a declaration.

// returns 'true' if the reference will not be updated by the refactor, but should not block the refactor
// returns 'undefined' if the reference should block the refactor
if (isDeclaration(entry.node.parent)) {
return entry.node;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this also needs to be modified to return true in some cases. For instance, the following test case that is similar to the method assignment one, results in the refactor failing to be applied:

function /*a*/foo/*b*/(actual: number) {
    return actual;
}
const x = foo;

That's because const x = foo is a declaration, so that foo reference will be added to the grouped reference's declarations, but it will later be considered invalid here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what I did, but I thought I had tested that case out and it wasn't blocked! I'll add it.

}
else if (
findAncestor(entry.node.parent, n => {
if (isDeclaration(n)) return true;
if (isPropertyAccessExpression(n) || isElementAccessExpression(n)) return false;
return "quit";
})
) {
return true;
}
return undefined;
}

function entryToFunctionCall(entry: FindAllReferences.NodeEntry): CallExpression | NewExpression | undefined {
function entryToFunctionCall(entry: FindAllReferences.NodeEntry): CallExpression | NewExpression | true | undefined {
// returns a callOrNewExpression if the reference needs to be updated during the refactor
// returns 'true' if the reference will not be updated by the refactor, but should not block the refactor
// returns 'undefined' if the reference should block the refactor
if (entry.node.parent) {
const functionReference = entry.node;
const parent = functionReference.parent;
let callOrNewExpression: CallExpression | NewExpression | undefined;
let matchReference = functionReference;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we assign functionReference to matchReference inside the first case of the switch, to match the other two cases?


switch (parent.kind) {
// foo(...) or super(...) or new Foo(...)
case SyntaxKind.CallExpression:
case SyntaxKind.NewExpression:
const callOrNewExpression = tryCast(parent, isCallOrNewExpression);
if (callOrNewExpression && callOrNewExpression.expression === functionReference) {
return callOrNewExpression;
}
callOrNewExpression = tryCast(parent, isCallOrNewExpression);
break;
// x.foo(...)
case SyntaxKind.PropertyAccessExpression:
const propertyAccessExpression = tryCast(parent, isPropertyAccessExpression);
if (propertyAccessExpression && propertyAccessExpression.parent && propertyAccessExpression.name === functionReference) {
const callOrNewExpression = tryCast(propertyAccessExpression.parent, isCallOrNewExpression);
if (callOrNewExpression && callOrNewExpression.expression === propertyAccessExpression) {
return callOrNewExpression;
}
callOrNewExpression = tryCast(propertyAccessExpression.parent, isCallOrNewExpression);
matchReference = propertyAccessExpression;
}
break;
// x["foo"](...)
case SyntaxKind.ElementAccessExpression:
const elementAccessExpression = tryCast(parent, isElementAccessExpression);
if (elementAccessExpression && elementAccessExpression.parent && elementAccessExpression.argumentExpression === functionReference) {
const callOrNewExpression = tryCast(elementAccessExpression.parent, isCallOrNewExpression);
if (callOrNewExpression && callOrNewExpression.expression === elementAccessExpression) {
return callOrNewExpression;
}
callOrNewExpression = tryCast(elementAccessExpression.parent, isCallOrNewExpression);
matchReference = elementAccessExpression;
}
break;
default:
return undefined;
}

if (callOrNewExpression === undefined) return undefined;
if (callOrNewExpression.expression === matchReference) return callOrNewExpression;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move the comments on the switch cases to here? Something saying that if callOrNewExpression.expression === matchReference, then it means we have one of the cases of e.g. x.foo(...) or x["foo"](...) or foo(...) or super(...) or new Foo(...).

if (isCallExpression(callOrNewExpression) && callOrNewExpression.arguments.some(arg => arg === matchReference)) return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my comment above, could you add comments here saying that if this is true, we have a reference like map(x, foo) or x.map(foo), etc.?

}
return undefined;
}
Expand Down
18 changes: 18 additions & 0 deletions tests/cases/fourslash/refactorConvertParamsToDestructuredObject_callbackFunction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/// <reference path='fourslash.ts' />

//// function /*a*/foo/*b*/(actual: number) {
//// return actual;
//// }
//// [1].map(foo);

goTo.select("a", "b");
edit.applyRefactor({
refactorName: "Convert parameters to destructured object",
actionName: "Convert parameters to destructured object",
actionDescription: "Convert parameters to destructured object",
newContent:
`function foo({ actual }: { actual: number; }) {
return actual;
}
[1].map(foo);`
});
25 changes: 25 additions & 0 deletions tests/cases/fourslash/refactorConvertParamsToDestructuredObject_callbackMethod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/// <reference path='fourslash.ts' />

//// class testClass {
//// /*a*/static foo/*b*/(actual: number) {
//// return actual;
//// }
//// }
//// [1].map(testClass.foo);
//// [2].map(testClass["foo"]);


goTo.select("a", "b");
edit.applyRefactor({
refactorName: "Convert parameters to destructured object",
actionName: "Convert parameters to destructured object",
actionDescription: "Convert parameters to destructured object",
newContent:
`class testClass {
static foo({ actual }: { actual: number; }) {
return actual;
}
}
[1].map(testClass.foo);
[2].map(testClass["foo"]);`
});
32 changes: 32 additions & 0 deletions tests/cases/fourslash/refactorConvertParamsToDestructuredObject_methodAssign.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/// <reference path='fourslash.ts' />

//// class testClass {
//// /*a*/static foo/*b*/(actual: number) {
//// return actual;
//// }
//// }
//// const x = testClass.foo;
//// [1].map(x);
//// const y = testClass["foo"];
//// [2].map(y);





goTo.select("a", "b");
edit.applyRefactor({
refactorName: "Convert parameters to destructured object",
actionName: "Convert parameters to destructured object",
actionDescription: "Convert parameters to destructured object",
newContent:
`class testClass {
static foo({ actual }: { actual: number; }) {
return actual;
}
}
const x = testClass.foo;
[1].map(x);
const y = testClass["foo"];
[2].map(y);`
});