Skip to content

Extract Method #17625

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.

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

Merged
merged 9 commits into from
Aug 11, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@
"problemMatcher": [
"$tsc"
]
},
{
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Allows debugging single tests from VS Code by pressing F5 in the testcase's editor window

Copy link
Member

Choose a reason for hiding this comment

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

That sounds great. How does it do that?

"taskName": "tests",
"showOutput": "silent",
"problemMatcher": [
"$tsc"
]
}
]
}
1 change: 1 addition & 0 deletions Jakefile.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ var harnessSources = harnessCoreSources.concat([
"projectErrors.ts",
"matchFiles.ts",
"initializeTSConfig.ts",
"extractMethods.ts",
"printer.ts",
"textChanges.ts",
"telemetry.ts",
Expand Down
7 changes: 3 additions & 4 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,11 +223,10 @@ namespace ts {
getSuggestionForNonexistentProperty: (node, type) => unescapeLeadingUnderscores(getSuggestionForNonexistentProperty(node, type)),
getSuggestionForNonexistentSymbol: (location, name, meaning) => unescapeLeadingUnderscores(getSuggestionForNonexistentSymbol(location, escapeLeadingUnderscores(name), meaning)),
getBaseConstraintOfType,
getJsxNamespace: () => unescapeLeadingUnderscores(getJsxNamespace()),
resolveNameAtLocation(location: Node, name: string, meaning: SymbolFlags): Symbol | undefined {
location = getParseTreeNode(location);
return resolveName(location, escapeLeadingUnderscores(name), meaning, /*nameNotFoundMessage*/ undefined, escapeLeadingUnderscores(name));
resolveName(name, location, meaning) {
return resolveName(location, escapeLeadingUnderscores(name), meaning, /*nameNotFoundMessage*/ undefined, /*nameArg*/ undefined);
},
getJsxNamespace: () => unescapeLeadingUnderscores(getJsxNamespace()),
};

const tupleTypes: GenericType[] = [];
Expand Down
10 changes: 10 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3677,5 +3677,15 @@
"Convert function '{0}' to class": {
"category": "Message",
"code": 95002
},

"Extract function": {
"category": "Message",
"code": 95003
},

"Extract function into '{0}'": {
"category": "Message",
"code": 95004
}
}
2 changes: 1 addition & 1 deletion src/compiler/transformers/es2015.ts
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ namespace ts {
function shouldVisitNode(node: Node): boolean {
return (node.transformFlags & TransformFlags.ContainsES2015) !== 0
|| convertedLoopState !== undefined
|| (hierarchyFacts & HierarchyFacts.ConstructorWithCapturedSuper && isStatement(node))
|| (hierarchyFacts & HierarchyFacts.ConstructorWithCapturedSuper && (isStatement(node) || (node.kind === SyntaxKind.Block)))
|| (isIterationStatement(node, /*lookInLabeledStatements*/ false) && shouldConvertIterationStatementBody(node))
|| isTypeScriptClassWrapper(node);
}
Expand Down
3 changes: 1 addition & 2 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2631,9 +2631,8 @@ namespace ts {
* Does not include properties of primitive types.
*/
/* @internal */ getAllPossiblePropertiesOfType(type: Type): Symbol[];

/* @internal */ resolveName(name: string, location: Node, meaning: SymbolFlags): Symbol | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

See comment on the implementation.

/* @internal */ getJsxNamespace(): string;
/* @internal */ resolveNameAtLocation(location: Node, name: string, meaning: SymbolFlags): Symbol | undefined;
}

export enum NodeBuilderFlags {
Expand Down
34 changes: 29 additions & 5 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,8 @@ namespace ts {
return isExternalModule(node) || compilerOptions.isolatedModules;
}

function isBlockScope(node: Node, parentNode: Node) {
/* @internal */
export function isBlockScope(node: Node, parentNode: Node) {
switch (node.kind) {
case SyntaxKind.SourceFile:
case SyntaxKind.CaseBlock:
Expand Down Expand Up @@ -1502,8 +1503,8 @@ namespace ts {
parent.parent.kind === SyntaxKind.VariableStatement;
const variableStatementNode =
isInitializerOfVariableDeclarationInStatement ? parent.parent.parent :
isVariableOfVariableDeclarationStatement ? parent.parent :
undefined;
isVariableOfVariableDeclarationStatement ? parent.parent :
undefined;
if (variableStatementNode) {
getJSDocCommentsAndTagsWorker(variableStatementNode);
}
Expand Down Expand Up @@ -1617,7 +1618,7 @@ namespace ts {
if (isInJavaScriptFile(node)) {
if (node.type && node.type.kind === SyntaxKind.JSDocVariadicType ||
forEach(getJSDocParameterTags(node),
t => t.typeExpression && t.typeExpression.type.kind === SyntaxKind.JSDocVariadicType)) {
t => t.typeExpression && t.typeExpression.type.kind === SyntaxKind.JSDocVariadicType)) {
return true;
}
}
Expand Down Expand Up @@ -4979,6 +4980,19 @@ namespace ts {
return isUnaryExpressionKind(skipPartiallyEmittedExpressions(node).kind);
}

/* @internal */
export function isUnaryExpressionWithWrite(expr: Node): expr is PrefixUnaryExpression | PostfixUnaryExpression {
switch (expr.kind) {
case SyntaxKind.PostfixUnaryExpression:
return true;
case SyntaxKind.PrefixUnaryExpression:
return (<PrefixUnaryExpression>expr).operator === SyntaxKind.PlusPlusToken ||
(<PrefixUnaryExpression>expr).operator === SyntaxKind.MinusMinusToken;
default:
return false;
}
}

function isExpressionKind(kind: SyntaxKind) {
return kind === SyntaxKind.ConditionalExpression
|| kind === SyntaxKind.YieldExpression
Expand Down Expand Up @@ -5193,7 +5207,17 @@ namespace ts {
const kind = node.kind;
return isStatementKindButNotDeclarationKind(kind)
|| isDeclarationStatementKind(kind)
|| kind === SyntaxKind.Block;
|| isBlockStatement(node);
}

function isBlockStatement(node: Node): node is Block {
if (node.kind !== SyntaxKind.Block) return false;
if (node.parent !== undefined) {
if (node.parent.kind === SyntaxKind.TryStatement || node.parent.kind === SyntaxKind.CatchClause) {
return false;
}
}
return !isFunctionBlock(node);
}

// Module references
Expand Down
95 changes: 79 additions & 16 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,9 @@ namespace FourSlash {

// The current caret position in the active file
public currentCaretPosition = 0;
// The position of the end of the current selection, or -1 if nothing is selected
public selectionEnd = -1;

public lastKnownMarker = "";

// The file that's currently 'opened'
Expand Down Expand Up @@ -433,11 +436,19 @@ namespace FourSlash {

public goToPosition(pos: number) {
this.currentCaretPosition = pos;
this.selectionEnd = -1;
}

public select(startMarker: string, endMarker: string) {
const start = this.getMarkerByName(startMarker), end = this.getMarkerByName(endMarker);
this.goToPosition(start.position);
this.selectionEnd = end.position;
}

public moveCaretRight(count = 1) {
this.currentCaretPosition += count;
this.currentCaretPosition = Math.min(this.currentCaretPosition, this.getFileContent(this.activeFile.fileName).length);
this.selectionEnd = -1;
}

// Opens a file given its 0-based index or fileName
Expand Down Expand Up @@ -967,9 +978,9 @@ namespace FourSlash {
}

for (const reference of expectedReferences) {
const {fileName, start, end} = reference;
const { fileName, start, end } = reference;
if (reference.marker && reference.marker.data) {
const {isWriteAccess, isDefinition} = reference.marker.data;
const { isWriteAccess, isDefinition } = reference.marker.data;
this.verifyReferencesWorker(actualReferences, fileName, start, end, isWriteAccess, isDefinition);
}
else {
Expand Down Expand Up @@ -1180,7 +1191,7 @@ namespace FourSlash {
displayParts: ts.SymbolDisplayPart[],
documentation: ts.SymbolDisplayPart[],
tags: ts.JSDocTagInfo[]
) {
) {

const actualQuickInfo = this.languageService.getQuickInfoAtPosition(this.activeFile.fileName, this.currentCaretPosition);
assert.equal(actualQuickInfo.kind, kind, this.messageAtLastKnownMarker("QuickInfo kind"));
Expand Down Expand Up @@ -1776,19 +1787,16 @@ namespace FourSlash {
// We get back a set of edits, but langSvc.editScript only accepts one at a time. Use this to keep track
// of the incremental offset from each edit to the next. We assume these edit ranges don't overlap

edits = edits.sort((a, b) => a.span.start - b.span.start);
for (let i = 0; i < edits.length - 1; i++) {
const firstEditSpan = edits[i].span;
const firstEditEnd = firstEditSpan.start + firstEditSpan.length;
assert.isTrue(firstEditEnd <= edits[i + 1].span.start);
}
// Copy this so we don't ruin someone else's copy
edits = JSON.parse(JSON.stringify(edits));
Copy link
Member

@amcasey amcasey Aug 9, 2017

Choose a reason for hiding this comment

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

JSON.parse(JSON.stringify(edits)); [](start = 20, length = 34)

This seems like a particularly expensive way to accomplish the copy. #WontFix

Copy link
Member

Choose a reason for hiding this comment

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

Never mind - didn't see that this was test code.


In reply to: 132299991 [](ancestors = 132299991)


// Get a snapshot of the content of the file so we can make sure any formatting edits didn't destroy non-whitespace characters
const oldContent = this.getFileContent(fileName);
let runningOffset = 0;

for (const edit of edits) {
const offsetStart = edit.span.start + runningOffset;
for (let i = 0; i < edits.length; i++) {
const edit = edits[i];
const offsetStart = edit.span.start;
const offsetEnd = offsetStart + edit.span.length;
this.editScriptAndUpdateMarkers(fileName, offsetStart, offsetEnd, edit.newText);
const editDelta = edit.newText.length - edit.span.length;
Expand All @@ -1803,8 +1811,13 @@ namespace FourSlash {
}
}
runningOffset += editDelta;
// TODO: Consider doing this at least some of the time for higher fidelity. Currently causes a failure (bug 707150)
// this.languageService.getScriptLexicalStructure(fileName);

// Update positions of any future edits affected by this change
for (let j = i + 1; j < edits.length; j++) {
if (edits[j].span.start >= edits[i].span.start) {
edits[j].span.start += editDelta;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we avoid this work by sorting the edits in descending order of start position? (That would also make it easy to restore the no-overlap assertion.)

Copy link
Member

Choose a reason for hiding this comment

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

Given that this is test code, I don't particularly care about the extra work, but having the assert seems worthwhile.


In reply to: 132300325 [](ancestors = 132300325)

}

if (isFormattingEdit) {
Expand Down Expand Up @@ -1888,7 +1901,7 @@ namespace FourSlash {
this.goToPosition(len);
}

public goToRangeStart({fileName, start}: Range) {
public goToRangeStart({ fileName, start }: Range) {
this.openFile(fileName);
this.goToPosition(start);
}
Expand Down Expand Up @@ -2062,7 +2075,7 @@ namespace FourSlash {
return result;
}

private rangeText({fileName, start, end}: Range): string {
private rangeText({ fileName, start, end }: Range): string {
return this.getFileContent(fileName).slice(start, end);
}

Expand Down Expand Up @@ -2348,7 +2361,7 @@ namespace FourSlash {
private applyCodeActions(actions: ts.CodeAction[], index?: number): void {
if (index === undefined) {
if (!(actions && actions.length === 1)) {
this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `${Harness.IO.newLine()} "${a.description}"`) : "" }`);
this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `${Harness.IO.newLine()} "${a.description}"`) : ""}`);
}
index = 0;
}
Expand Down Expand Up @@ -2723,6 +2736,30 @@ namespace FourSlash {
}
}

private getSelection() {
return ({
pos: this.currentCaretPosition,
end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd
});
}

public verifyRefactorAvailable(negative: boolean, name?: string, subName?: string) {
const selection = this.getSelection();

let refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, selection) || [];
if (name) {
refactors = refactors.filter(r => r.name === name && (subName === undefined || r.actions.some(a => a.name === subName)));
}
const isAvailable = refactors.length > 0;

if (negative && isAvailable) {
this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some: ${refactors.map(r => r.name).join(", ")}`);
}
else if (!negative && !isAvailable) {
this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected a refactor but found none.`);
}
}

public verifyApplicableRefactorAvailableForRange(negative: boolean) {
const ranges = this.getRanges();
if (!(ranges && ranges.length === 1)) {
Expand All @@ -2739,6 +2776,20 @@ namespace FourSlash {
}
}

public applyRefactor(refactorName: string, actionName: string) {
const range = this.getSelection();
const refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, range);
const refactor = ts.find(refactors, r => r.name === refactorName);
if (!refactor) {
this.raiseError(`The expected refactor: ${refactorName} is not available at the marker location.`);
}

const editInfo = this.languageService.getEditsForRefactor(this.activeFile.fileName, this.formatCodeSettings, range, refactorName, actionName);
for (const edit of editInfo.edits) {
this.applyEdits(edit.fileName, edit.textChanges, /*isFormattingEdit*/ false);
}
}

public verifyFileAfterApplyingRefactorAtMarker(
markerName: string,
expectedContent: string,
Expand Down Expand Up @@ -3483,6 +3534,10 @@ namespace FourSlashInterface {
public file(indexOrName: any, content?: string, scriptKindName?: string): void {
this.state.openFile(indexOrName, content, scriptKindName);
}

public select(startMarker: string, endMarker: string) {
this.state.select(startMarker, endMarker);
}
}

export class VerifyNegatable {
Expand Down Expand Up @@ -3604,6 +3659,10 @@ namespace FourSlashInterface {
public applicableRefactorAvailableForRange() {
this.state.verifyApplicableRefactorAvailableForRange(this.negative);
}

public refactorAvailable(name?: string, subName?: string) {
this.state.verifyRefactorAvailable(this.negative, name, subName);
}
}

export class Verify extends VerifyNegatable {
Expand Down Expand Up @@ -3999,6 +4058,10 @@ namespace FourSlashInterface {
public disableFormatting() {
this.state.enableFormatting = false;
}

public applyRefactor(refactorName: string, actionName: string) {
this.state.applyRefactor(refactorName, actionName);
}
}

export class Debug {
Expand Down
Loading