Skip to content
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

Expand extractSymbol ranges #42770

Merged
merged 6 commits into from
Feb 17, 2021
Merged

Expand extractSymbol ranges #42770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
74d9b19
allow partial selections of node ranges
jessetrinity Feb 11, 2021
9774b3a
separate tests for better failure investigation
jessetrinity Feb 11, 2021
2ec179b
gate span expansion behind invoked command
jessetrinity Feb 12, 2021
20db66f
add invoked test
jessetrinity Feb 12, 2021
610da7e
comment wording
jessetrinity Feb 16, 2021
8a63048
for test
jessetrinity Feb 17, 2021
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
50 changes: 34 additions & 16 deletions src/services/refactors/extractSymbol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ namespace ts.refactor.extractSymbol {
let i = 0;
for (const { functionExtraction, constantExtraction } of extractions) {
const description = functionExtraction.description;
if(refactorKindBeginsWith(extractFunctionAction.kind, requestedRefactor)){
if (refactorKindBeginsWith(extractFunctionAction.kind, requestedRefactor)) {
if (functionExtraction.errors.length === 0) {
// Don't issue refactorings with duplicated names.
// Scopes come back in "innermost first" order, so extractions will
Expand All @@ -94,8 +94,7 @@ namespace ts.refactor.extractSymbol {
}
}

// Skip these since we don't have a way to report errors yet
if(refactorKindBeginsWith(extractConstantAction.kind, requestedRefactor)) {
if (refactorKindBeginsWith(extractConstantAction.kind, requestedRefactor)) {
if (constantExtraction.errors.length === 0) {
// Don't issue refactorings with duplicated names.
// Scopes come back in "innermost first" order, so extractions will
Expand Down Expand Up @@ -265,24 +264,30 @@ namespace ts.refactor.extractSymbol {
/**
* getRangeToExtract takes a span inside a text file and returns either an expression or an array
* of statements representing the minimum set of nodes needed to extract the entire span. This
* process may fail, in which case a set of errors is returned instead (these are currently
* not shown to the user, but can be used by us diagnostically)
* process may fail, in which case a set of errors is returned instead. These errors are shown to
* users if they have the provideRefactorNotApplicableReason option set.
*/
// exported only for tests
export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan, considerEmptySpans = true): RangeToExtract {
export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan, invoked = true): RangeToExtract {
const { length } = span;
if (length === 0 && !considerEmptySpans) {
if (length === 0 && !invoked) {
return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractEmpty)] };
}
const cursorRequest = length === 0 && considerEmptySpans;
const cursorRequest = length === 0 && invoked;

const startToken = getTokenAtPosition(sourceFile, span.start);
const endToken = findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span));
/* If the refactoring command is invoked through a keyboard action it's safe to assume that the user is actively looking for
refactoring actions at the span location. As they may not know the exact range that will trigger a refactoring, we expand the
searched span to cover a real node range making it more likely that something useful will show up. */
const adjustedSpan = startToken && endToken && invoked ? getAdjustedSpanFromNodes(startToken, endToken, sourceFile) : span;
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved

// Walk up starting from the the start position until we find a non-SourceFile node that subsumes the selected span.
// This may fail (e.g. you select two statements in the root of a source file)
const startToken = getTokenAtPosition(sourceFile, span.start);
const start = cursorRequest ? getExtractableParent(startToken): getParentNodeInSpan(startToken, sourceFile, span);
const start = cursorRequest ? getExtractableParent(startToken): getParentNodeInSpan(startToken, sourceFile, adjustedSpan);

// Do the same for the ending position
const endToken = findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span));
const end = cursorRequest ? start : getParentNodeInSpan(endToken, sourceFile, span);
const end = cursorRequest ? start : getParentNodeInSpan(endToken, sourceFile, adjustedSpan);

const declarations: Symbol[] = [];

Expand All @@ -295,6 +300,10 @@ namespace ts.refactor.extractSymbol {
return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractRange)] };
}

if (isJSDoc(start)) {
amcasey marked this conversation as resolved.
Show resolved Hide resolved
return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractJSDoc)] };
}

if (start.parent !== end.parent) {
// start and end nodes belong to different subtrees
return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractRange)] };
Expand Down Expand Up @@ -332,10 +341,6 @@ namespace ts.refactor.extractSymbol {
return { targetRange: { range: statements, facts: rangeFacts, declarations } };
}

if (isJSDoc(start)) {
return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractJSDoc)] };
}

if (isReturnStatement(start) && !start.expression) {
// Makes no sense to extract an expression-less return statement.
return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractRange)] };
Expand Down Expand Up @@ -605,6 +610,19 @@ namespace ts.refactor.extractSymbol {
}
}

/**
* Includes the final semicolon so that the span covers statements in cases where it would otherwise
* only cover the declaration list.
*/
function getAdjustedSpanFromNodes(startNode: Node, endNode: Node, sourceFile: SourceFile): TextSpan {
const start = startNode.getStart(sourceFile);
let end = endNode.getEnd();
if (sourceFile.text.charCodeAt(end) === CharacterCodes.semicolon) {
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved
end++;
}
return { start, length: end - start };
}

function getStatementOrExpressionRange(node: Node): Statement[] | Expression | undefined {
if (isStatement(node)) {
return [node];
Expand Down
123 changes: 71 additions & 52 deletions src/testRunner/unittests/services/extract/ranges.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,85 +14,100 @@ namespace ts {
});
}

function testExtractRange(s: string): void {
const t = extractTest(s);
const f = createSourceFile("a.ts", t.source, ScriptTarget.Latest, /*setParentNodes*/ true);
const selectionRange = t.ranges.get("selection");
if (!selectionRange) {
throw new Error(`Test ${s} does not specify selection range`);
}
const result = refactor.extractSymbol.getRangeToExtract(f, createTextSpanFromRange(selectionRange));
const expectedRange = t.ranges.get("extracted");
if (expectedRange) {
let pos: number, end: number;
const targetRange = result.targetRange!;
if (isArray(targetRange.range)) {
pos = targetRange.range[0].getStart(f);
end = last(targetRange.range).getEnd();
function testExtractRange(caption: string, s: string) {
return it(caption, () => {
const t = extractTest(s);
const f = createSourceFile("a.ts", t.source, ScriptTarget.Latest, /*setParentNodes*/ true);
const selectionRange = t.ranges.get("selection");
if (!selectionRange) {
throw new Error(`Test ${s} does not specify selection range`);
}
const result = refactor.extractSymbol.getRangeToExtract(f, createTextSpanFromRange(selectionRange));
const expectedRange = t.ranges.get("extracted");
if (expectedRange) {
let pos: number, end: number;
const targetRange = result.targetRange!;
if (isArray(targetRange.range)) {
pos = targetRange.range[0].getStart(f);
end = last(targetRange.range).getEnd();
}
else {
pos = targetRange.range.getStart(f);
end = targetRange.range.getEnd();
}
assert.equal(pos, expectedRange.pos, "incorrect pos of range");
assert.equal(end, expectedRange.end, "incorrect end of range");
}
else {
pos = targetRange.range.getStart(f);
end = targetRange.range.getEnd();
assert.isTrue(!result.targetRange, `expected range to extract to be undefined`);
}
assert.equal(pos, expectedRange.pos, "incorrect pos of range");
assert.equal(end, expectedRange.end, "incorrect end of range");
}
else {
assert.isTrue(!result.targetRange, `expected range to extract to be undefined`);
}
});
}

describe("unittests:: services:: extract:: extractRanges", () => {
it("get extract range from selection", () => {
testExtractRange(`
describe("get extract range from selection", () => {
testExtractRange("extractRange1", `
[#|
[$|var x = 1;
var y = 2;|]|]
`);
testExtractRange(`
[#|
var x = 1;
var y = 2|];
testExtractRange("extractRange2", `
[$|[#|var x = 1;
var y = 2|];|]
`);
testExtractRange(`
[#|var x = 1|];
testExtractRange("extractRange3", `
[#|var x = [$|1|]|];
jessetrinity marked this conversation as resolved.
Show resolved Hide resolved
var y = 2;
`);
testExtractRange(`
testExtractRange("extractRange4", `
var x = [$|10[#|00|]|];
`);
testExtractRange("extractRange5", `
[$|va[#|r foo = 1;
var y = 200|]0;|]
`);
testExtractRange("extractRange6", `
var x = [$|fo[#|o.bar.baz()|]|];
`);
testExtractRange("extractRange7", `
if ([#|[#extracted|a && b && c && d|]|]) {
}
`);
testExtractRange(`
testExtractRange("extractRange8", `
if [#|(a && b && c && d|]) {
}
`);
testExtractRange(`
testExtractRange("extractRange9", `
if ([$|a[#|a && b && c && d|]d|]) {
}
`);
testExtractRange("extractRange10", `
if (a && b && c && d) {
[#| [$|var x = 1;
console.log(x);|] |]
}
`);
testExtractRange(`
testExtractRange("extractRange11", `
[#|
if (a) {
return 100;
} |]
`);
testExtractRange(`
testExtractRange("extractRange12", `
function foo() {
[#| [$|if (a) {
}
return 100|] |]
}
`);
testExtractRange(`
testExtractRange("extractRange13", `
[#|
[$|l1:
if (x) {
break l1;
}|]|]
`);
testExtractRange(`
testExtractRange("extractRange14", `
[#|
[$|l2:
{
Expand All @@ -101,21 +116,21 @@ namespace ts {
break l2;
}|]|]
`);
testExtractRange(`
testExtractRange("extractRange15", `
while (true) {
[#| if(x) {
}
break; |]
}
`);
testExtractRange(`
testExtractRange("extractRange16", `
while (true) {
[#| if(x) {
}
continue; |]
}
`);
testExtractRange(`
testExtractRange("extractRange17", `
l3:
{
[#|
Expand All @@ -124,7 +139,7 @@ namespace ts {
break l3; |]
}
`);
testExtractRange(`
testExtractRange("extractRange18", `
function f() {
while (true) {
[#|
Expand All @@ -134,7 +149,7 @@ namespace ts {
}
}
`);
testExtractRange(`
testExtractRange("extractRange19", `
function f() {
while (true) {
[#|
Expand All @@ -145,13 +160,13 @@ namespace ts {
}
}
`);
testExtractRange(`
testExtractRange("extractRange20", `
function f() {
return [#| [$|1 + 2|] |]+ 3;
}
}
`);
testExtractRange(`
testExtractRange("extractRange21", `
function f(x: number) {
[#|[$|try {
x++;
Expand All @@ -163,17 +178,21 @@ namespace ts {
`);

// Variable statements
testExtractRange(`[#|let x = [$|1|];|]`);
testExtractRange(`[#|let x = [$|1|], y;|]`);
testExtractRange(`[#|[$|let x = 1, y = 1;|]|]`);
testExtractRange("extractRange22", `[#|let x = [$|1|];|]`);
testExtractRange("extractRange23", `[#|let x = [$|1|], y;|]`);
testExtractRange("extractRange24", `[#|[$|let x = 1, y = 1;|]|]`);

// Variable declarations
testExtractRange(`let [#|x = [$|1|]|];`);
testExtractRange(`let [#|x = [$|1|]|], y = 2;`);
testExtractRange(`let x = 1, [#|y = [$|2|]|];`);
testExtractRange("extractRange25", `let [#|x = [$|1|]|];`);
testExtractRange("extractRange26", `let [#|x = [$|1|]|], y = 2;`);
testExtractRange("extractRange27", `let x = 1, [#|y = [$|2|]|];`);

// Return statements
testExtractRange(`[#|return [$|1|];|]`);
testExtractRange("extractRange28", `[#|return [$|1|];|]`);

// For statements
testExtractRange("extractRange29", `for ([#|var i = 1|]; i < 2; i++) {}`);
testExtractRange("extractRange30", `for (var i = [#|[$|1|]|]; i < 2; i++) {}`);
});

testExtractRangeFailed("extractRangeFailed1",
Expand Down
0 ...ourslash/extractSymbolForTriggerReason.ts → ...urslash/extractSymbolForTriggerReason1.ts
View file
Open in desktop
File renamed without changes.
8 changes: 8 additions & 0 deletions tests/cases/fourslash/extractSymbolForTriggerReason2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/// <reference path='fourslash.ts' />

////const foo = ba/*a*/r + b/*b*/az;

// Expand selection to fit nodes if refactors are explicitly requested
goTo.select("a", "b");
verify.not.refactorAvailableForTriggerReason("implicit", "Extract Symbol");
verify.refactorAvailableForTriggerReason("invoked", "Extract Symbol", "constant_scope_0");