From f957429efd68bfe5089348b361c30663bac6e65b Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 10 Aug 2017 16:23:17 -0700 Subject: [PATCH] Style fixups --- src/compiler/transformers/es2015.ts | 2 +- src/compiler/utilities.ts | 8 --- src/harness/fourslash.ts | 2 +- src/harness/unittests/extractMethods.ts | 4 +- src/services/refactors/extractMethod.ts | 93 +++++++++++++------------ 5 files changed, 51 insertions(+), 58 deletions(-) diff --git a/src/compiler/transformers/es2015.ts b/src/compiler/transformers/es2015.ts index da827d7b63e..6c8f0a75bdb 100644 --- a/src/compiler/transformers/es2015.ts +++ b/src/compiler/transformers/es2015.ts @@ -394,7 +394,7 @@ namespace ts { function shouldVisitNode(node: Node): boolean { return (node.transformFlags & TransformFlags.ContainsES2015) !== 0 || convertedLoopState !== undefined - || (hierarchyFacts & HierarchyFacts.ConstructorWithCapturedSuper && isStatementOrBlock(node)) + || (hierarchyFacts & HierarchyFacts.ConstructorWithCapturedSuper && (isStatement(node) || (node.kind === SyntaxKind.Block))) || (isIterationStatement(node, /*lookInLabeledStatements*/ false) && shouldConvertIterationStatementBody(node)) || isTypeScriptClassWrapper(node); } diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 336b6b8da0a..78d7dfa7290 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -5353,14 +5353,6 @@ namespace ts { || isBlockStatement(node); } - /* @internal */ - export function isStatementOrBlock(node: Node): node is Statement { - const kind = node.kind; - return isStatementKindButNotDeclarationKind(kind) - || isDeclarationStatementKind(kind) - || node.kind === SyntaxKind.Block; - } - function isBlockStatement(node: Node): node is Block { if (node.kind !== SyntaxKind.Block) return false; if (node.parent !== undefined) { diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 7f8ded3e259..784c354b13b 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2766,7 +2766,7 @@ namespace FourSlash { const isAvailable = refactors.length > 0; if (negative && isAvailable) { - this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some.`); + 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.`); diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts index 4b5d1911b49..d096cd3d375 100644 --- a/src/harness/unittests/extractMethods.ts +++ b/src/harness/unittests/extractMethods.ts @@ -575,12 +575,12 @@ namespace A { }; const result = refactor.extractMethod.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end)); assert.equal(result.errors, undefined, "expect no errors"); - const results = refactor.extractMethod.extractRange(result.targetRange, context); + const results = refactor.extractMethod.getPossibleExtractions(result.targetRange, context); const data: string[] = []; data.push(`==ORIGINAL==`); data.push(sourceFile.text); for (const r of results) { - const changes = refactor.extractMethod.extractRange(result.targetRange, context, results.indexOf(r))[0].changes; + const changes = refactor.extractMethod.getPossibleExtractions(result.targetRange, context, results.indexOf(r))[0].changes; data.push(`==SCOPE::${r.scopeDescription}==`); data.push(textChanges.applyChanges(sourceFile.text, changes[0].textChanges)); } diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 0f58da4ef1d..4315092e192 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -21,7 +21,7 @@ namespace ts.refactor.extractMethod { return undefined; } - const extractions = extractRange(targetRange, context); + const extractions = getPossibleExtractions(targetRange, context); if (extractions === undefined) { // No extractions possible return undefined; @@ -37,15 +37,19 @@ namespace ts.refactor.extractMethod { continue; } - // Don't issue refactorings with duplicated names + // Don't issue refactorings with duplicated names. + // Scopes come back in "innermost first" order, so extractions will + // preferentially go into nearer scopes const description = formatStringFromArgs(Diagnostics.Extract_function_into_0.message, [extr.scopeDescription]); - if (!usedNames.get(description)) { + if (!usedNames.has(description)) { usedNames.set(description, true); actions.push({ description, name: `scope_${i}` }); } + // *do* increment i anyway because we'll look for the i-th scope + // later when actually doing the refactoring if the user requests it i++; } @@ -66,21 +70,14 @@ namespace ts.refactor.extractMethod { const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length }); const targetRange: TargetRange = rangeToExtract.targetRange; - const parsedIndexMatch = /scope_(\d+)/.exec(actionName); - if (!parsedIndexMatch) { - throw new Error("Expected to match the regexp"); - } + const parsedIndexMatch = /^scope_(\d+)$/.exec(actionName); + Debug.assert(!!parsedIndexMatch, "Scope name should have matched the regexp"); const index = +parsedIndexMatch[1]; - if (!isFinite(index)) { - throw new Error("Expected to parse a number from the scope index"); - } + Debug.assert(isFinite(index), "Expected to parse a finite number from the scope index"); - const extractions = extractRange(targetRange, context, index); - if (extractions === undefined) { - // Scope is no longer valid from when the user issued the refactor - // return undefined; - return undefined; - } + const extractions = getPossibleExtractions(targetRange, context, index); + // Scope is no longer valid from when the user issued the refactor (??) + Debug.assert(extractions !== undefined, "The extraction went missing? How?"); return ({ edits: extractions[0].changes }); } @@ -229,7 +226,7 @@ namespace ts.refactor.extractMethod { return { targetRange: { range: statements, facts: rangeFacts, declarations } }; } else { - // We have a single expression (start) + // We have a single node (start) const errors = checkRootNode(start) || checkNode(start); if (errors) { return { errors }; @@ -243,7 +240,7 @@ namespace ts.refactor.extractMethod { ? [start] : start.parent && start.parent.kind === SyntaxKind.ExpressionStatement ? [start.parent as Statement] - : start; + : start as Expression; return { targetRange: { range, facts: rangeFacts, declarations } }; } @@ -259,6 +256,31 @@ namespace ts.refactor.extractMethod { return undefined; } + function checkForStaticContext(nodeToCheck: Node, containingClass: Node) { + let current: Node = nodeToCheck; + while (current !== containingClass) { + if (current.kind === SyntaxKind.PropertyDeclaration) { + if (hasModifier(current, ModifierFlags.Static)) { + rangeFacts |= RangeFacts.InStaticRegion; + } + break; + } + else if (current.kind === SyntaxKind.Parameter) { + const ctorOrMethod = getContainingFunction(current); + if (ctorOrMethod.kind === SyntaxKind.Constructor) { + rangeFacts |= RangeFacts.InStaticRegion; + } + break; + } + else if (current.kind === SyntaxKind.MethodDeclaration) { + if (hasModifier(current, ModifierFlags.Static)) { + rangeFacts |= RangeFacts.InStaticRegion; + } + } + current = current.parent; + } + } + // Verifies whether we can actually extract this node or not. function checkNode(nodeToCheck: Node): Diagnostic[] | undefined { const enum PermittedJumps { @@ -275,31 +297,10 @@ namespace ts.refactor.extractMethod { return [createDiagnosticForNode(nodeToCheck, Messages.CannotExtractAmbientBlock)]; } - // If we're in a class, see if we're in a static region (static property initializer, static method, class constructor parameter default) or not - const stoppingPoint: Node = getContainingClass(nodeToCheck); - if (stoppingPoint) { - let current: Node = nodeToCheck; - while (current !== stoppingPoint) { - if (current.kind === SyntaxKind.PropertyDeclaration) { - if (hasModifier(current, ModifierFlags.Static)) { - rangeFacts |= RangeFacts.InStaticRegion; - } - break; - } - else if (current.kind === SyntaxKind.Parameter) { - const ctorOrMethod = getContainingFunction(current); - if (ctorOrMethod.kind === SyntaxKind.Constructor) { - rangeFacts |= RangeFacts.InStaticRegion; - } - break; - } - else if (current.kind === SyntaxKind.MethodDeclaration) { - if (hasModifier(current, ModifierFlags.Static)) { - rangeFacts |= RangeFacts.InStaticRegion; - } - } - current = current.parent; - } + // If we're in a class, see whether we're in a static region (static property initializer, static method, class constructor parameter default) + const containingClass: Node = getContainingClass(nodeToCheck); + if (containingClass) { + checkForStaticContext(nodeToCheck, containingClass); } let errors: Diagnostic[]; @@ -319,7 +320,7 @@ namespace ts.refactor.extractMethod { if (isDeclaration(node)) { const declaringNode = (node.kind === SyntaxKind.VariableDeclaration) ? node.parent.parent : node; if (hasModifier(declaringNode, ModifierFlags.Export)) { - (errors || (errors = []).push(createDiagnosticForNode(node, Messages.CannotExtractExportedEntity))); + (errors || (errors = [])).push(createDiagnosticForNode(node, Messages.CannotExtractExportedEntity)); return true; } declarations.push(node.symbol); @@ -494,7 +495,7 @@ namespace ts.refactor.extractMethod { // A function parameter's initializer is actually in the outer scope, not the function declaration if (current && current.parent && current.parent.kind === SyntaxKind.Parameter) { // Skip all the way to the outer scope of the function that declared this parameter - current = current.parent.parent.parent; + current = findAncestor(current, parent => isFunctionLike(parent)).parent; } else { current = current.parent; @@ -509,7 +510,7 @@ namespace ts.refactor.extractMethod { * Each returned ExtractResultForScope corresponds to a possible target scope and is either a set of changes * or an error explaining why we can't extract into that scope. */ - export function extractRange(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number = undefined): ReadonlyArray | undefined { + export function getPossibleExtractions(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number = undefined): ReadonlyArray | undefined { const { file: sourceFile } = context; if (targetRange === undefined) {