Skip to content

Commit 3c32f6e

Browse files
andrewbranchtypescript-botarmanio123
authored
Fix preserveSourceNewlines sibling node comparison (fixes extra newlines in organize imports) (#42630)
* Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * Update package-lock.json * More sophisticated check for source position comparability * Fix organize imports by looking at the nodes positions * Rollback formatting changes * Added tests, fixed organizeImports algorithm * Fix autoformatting again * Make sibling node comparison work for all lists * Don’t run siblingNodePositionsAreComparable at all unless `preserveSourceNewlines` is true * Move getNodeAtPosition back * Optimize * Use node array index check instead of tree walk * Revert unneeded change Co-authored-by: TypeScript Bot <typescriptbot@microsoft.com> Co-authored-by: Armando Aguirre <armanio123@outlook.com>
1 parent 68b0323 commit 3c32f6e

File tree

5 files changed

+164
-10
lines changed

5 files changed

+164
-10
lines changed

‎src/compiler/emitter.ts

+31-10
Original file line numberDiff line numberDiff line change
@@ -910,7 +910,7 @@ namespace ts {
910910
let containerEnd = -1;
911911
let declarationListContainerEnd = -1;
912912
let currentLineMap: readonly number[] | undefined;
913-
let detachedCommentsInfo: { nodePos: number, detachedCommentEndPos: number}[] | undefined;
913+
let detachedCommentsInfo: { nodePos: number, detachedCommentEndPos: number }[] | undefined;
914914
let hasWrittenComment = false;
915915
let commentsDisabled = !!printerOptions.removeComments;
916916
const { enter: enterComment, exit: exitComment } = performance.createTimerIf(extendedDiagnostics, "commentTime", "beforeComment", "afterComment");
@@ -4469,15 +4469,15 @@ namespace ts {
44694469
// JsxText will be written with its leading whitespace, so don't add more manually.
44704470
return 0;
44714471
}
4472-
else if (!nodeIsSynthesized(previousNode) && !nodeIsSynthesized(nextNode) && (!previousNode.parent || !nextNode.parent || previousNode.parent === nextNode.parent)) {
4473-
if (preserveSourceNewlines && previousNode.parent && nextNode.parent) {
4474-
return getEffectiveLines(
4475-
includeComments => getLinesBetweenRangeEndAndRangeStart(
4476-
previousNode,
4477-
nextNode,
4478-
currentSourceFile!,
4479-
includeComments));
4480-
}
4472+
else if (preserveSourceNewlines && siblingNodePositionsAreComparable(previousNode, nextNode)) {
4473+
return getEffectiveLines(
4474+
includeComments => getLinesBetweenRangeEndAndRangeStart(
4475+
previousNode,
4476+
nextNode,
4477+
currentSourceFile!,
4478+
includeComments));
4479+
}
4480+
else if (!preserveSourceNewlines && !nodeIsSynthesized(previousNode) && !nodeIsSynthesized(nextNode)) {
44814481
return rangeEndIsOnSameLineAsRangeStart(previousNode, nextNode, currentSourceFile!) ? 0 : 1;
44824482
}
44834483
else if (synthesizedNodeStartsOnNewLine(previousNode, format) || synthesizedNodeStartsOnNewLine(nextNode, format)) {
@@ -5173,6 +5173,27 @@ namespace ts {
51735173

51745174
}
51755175

5176+
function siblingNodePositionsAreComparable(previousNode: Node, nextNode: Node) {
5177+
if (nodeIsSynthesized(previousNode) || nodeIsSynthesized(nextNode)) {
5178+
return false;
5179+
}
5180+
5181+
if (nextNode.pos < previousNode.end) {
5182+
return false;
5183+
}
5184+
5185+
previousNode = getOriginalNode(previousNode);
5186+
nextNode = getOriginalNode(nextNode);
5187+
const parent = previousNode.parent;
5188+
if (!parent || parent !== nextNode.parent) {
5189+
return false;
5190+
}
5191+
5192+
const parentNodeArray = getContainingNodeArray(previousNode);
5193+
const prevNodeIndex = parentNodeArray?.indexOf(previousNode);
5194+
return prevNodeIndex !== undefined && prevNodeIndex > -1 && parentNodeArray!.indexOf(nextNode) === prevNodeIndex + 1;
5195+
}
5196+
51765197
function emitLeadingComments(pos: number, isEmittedNode: boolean) {
51775198
hasWrittenComment = false;
51785199

‎src/compiler/utilities.ts

+67
Original file line numberDiff line numberDiff line change
@@ -7097,4 +7097,71 @@ namespace ts {
70977097
export function containsIgnoredPath(path: string) {
70987098
return some(ignoredPaths, p => stringContains(path, p));
70997099
}
7100+
7101+
export function getContainingNodeArray(node: Node): NodeArray<Node> | undefined {
7102+
if (!node.parent) return undefined;
7103+
switch (node.kind) {
7104+
case SyntaxKind.TypeParameter:
7105+
const { parent } = node as TypeParameterDeclaration;
7106+
return parent.kind === SyntaxKind.InferType ? undefined : parent.typeParameters;
7107+
case SyntaxKind.Parameter:
7108+
return (node as ParameterDeclaration).parent.parameters;
7109+
case SyntaxKind.TemplateLiteralTypeSpan:
7110+
return (node as TemplateLiteralTypeSpan).parent.templateSpans;
7111+
case SyntaxKind.TemplateSpan:
7112+
return (node as TemplateSpan).parent.templateSpans;
7113+
case SyntaxKind.Decorator:
7114+
return (node as Decorator).parent.decorators;
7115+
case SyntaxKind.HeritageClause:
7116+
return (node as HeritageClause).parent.heritageClauses;
7117+
}
7118+
7119+
const { parent } = node;
7120+
if (isJSDocTag(node)) {
7121+
return isJSDocTypeLiteral(node.parent) ? undefined : node.parent.tags;
7122+
}
7123+
7124+
switch (parent.kind) {
7125+
case SyntaxKind.TypeLiteral:
7126+
case SyntaxKind.InterfaceDeclaration:
7127+
return isTypeElement(node) ? (parent as TypeLiteralNode | InterfaceDeclaration).members : undefined;
7128+
case SyntaxKind.UnionType:
7129+
case SyntaxKind.IntersectionType:
7130+
return (parent as UnionOrIntersectionTypeNode).types;
7131+
case SyntaxKind.TupleType:
7132+
case SyntaxKind.ArrayLiteralExpression:
7133+
case SyntaxKind.CommaListExpression:
7134+
case SyntaxKind.NamedImports:
7135+
case SyntaxKind.NamedExports:
7136+
return (parent as TupleTypeNode | ArrayLiteralExpression | CommaListExpression | NamedImports | NamedExports).elements;
7137+
case SyntaxKind.ObjectLiteralExpression:
7138+
case SyntaxKind.JsxAttributes:
7139+
return (parent as ObjectLiteralExpressionBase<ObjectLiteralElement>).properties;
7140+
case SyntaxKind.CallExpression:
7141+
case SyntaxKind.NewExpression:
7142+
return isTypeNode(node) ? (parent as CallExpression | NewExpression).typeArguments :
7143+
(parent as CallExpression | NewExpression).expression === node ? undefined :
7144+
(parent as CallExpression | NewExpression).arguments;
7145+
case SyntaxKind.JsxElement:
7146+
case SyntaxKind.JsxFragment:
7147+
return isJsxChild(node) ? (parent as JsxElement | JsxFragment).children : undefined;
7148+
case SyntaxKind.JsxOpeningElement:
7149+
case SyntaxKind.JsxSelfClosingElement:
7150+
return isTypeNode(node) ? (parent as JsxOpeningElement | JsxSelfClosingElement).typeArguments : undefined;
7151+
case SyntaxKind.Block:
7152+
case SyntaxKind.CaseClause:
7153+
case SyntaxKind.DefaultClause:
7154+
case SyntaxKind.ModuleBlock:
7155+
return (parent as Block | CaseOrDefaultClause | ModuleBlock).statements;
7156+
case SyntaxKind.CaseBlock:
7157+
return (parent as CaseBlock).clauses;
7158+
case SyntaxKind.ClassDeclaration:
7159+
case SyntaxKind.ClassExpression:
7160+
return isClassElement(node) ? (parent as ClassLikeDeclaration).members : undefined;
7161+
case SyntaxKind.EnumDeclaration:
7162+
return isEnumMember(node) ? (parent as EnumDeclaration).members : undefined;
7163+
case SyntaxKind.SourceFile:
7164+
return (parent as SourceFile).statements;
7165+
}
7166+
}
71007167
}

‎tests/cases/fourslash/organizeImports1.ts

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// Regression test for bug #41417
4+
5+
//// import {
6+
//// d, d as D,
7+
//// c,
8+
//// c as C, b,
9+
//// b as B, a
10+
//// } from './foo';
11+
//// import {
12+
//// h, h as H,
13+
//// g,
14+
//// g as G, f,
15+
//// f as F, e
16+
//// } from './foo';
17+
////
18+
//// console.log(a, B, b, c, C, d, D);
19+
//// console.log(e, f, F, g, G, H, h);
20+
21+
verify.organizeImports(
22+
`import {
23+
a, b,
24+
b as B, c,
25+
c as C, d, d as D, e, f,
26+
f as F, g,
27+
g as G, h, h as H
28+
} from './foo';
29+
30+
console.log(a, B, b, c, C, d, D);
31+
console.log(e, f, F, g, G, H, h);`
32+
);

‎tests/cases/fourslash/organizeImports2.ts

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// Regression test for bug #41417
4+
5+
//// import {
6+
//// Foo
7+
//// , Bar
8+
//// } from "foo"
9+
////
10+
//// console.log(Foo, Bar);
11+
12+
verify.organizeImports(
13+
`import { Bar, Foo } from "foo";
14+
15+
console.log(Foo, Bar);`
16+
);

‎tests/cases/fourslash/organizeImports3.ts

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// Regression test for bug #41417
4+
5+
//// import {
6+
//// Bar
7+
//// , Foo
8+
//// } from "foo"
9+
////
10+
//// console.log(Foo, Bar);
11+
12+
verify.organizeImports(
13+
`import {
14+
Bar,
15+
Foo
16+
} from "foo";
17+
18+
console.log(Foo, Bar);`);

0 commit comments

Comments
 (0)