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

Jump to bottom

Do not incorrectly add line separators for non-synthetic nodes when emitting node list #44070

Merged
merged 1 commit into from Jun 4, 2021
Merged

Do not incorrectly add line separators for non-synthetic nodes when emitting node list #44070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 27 additions & 14 deletions src/compiler/emitter.ts
View file
Open in desktop
Expand Up @@ -4564,16 +4564,26 @@ namespace ts {
// JsxText will be written with its leading whitespace, so don't add more manually.
return 0;
}
else if (preserveSourceNewlines && siblingNodePositionsAreComparable(previousNode, nextNode)) {
return getEffectiveLines(
includeComments => getLinesBetweenRangeEndAndRangeStart(
previousNode,
nextNode,
currentSourceFile!,
includeComments));
}
else if (!preserveSourceNewlines && !nodeIsSynthesized(previousNode) && !nodeIsSynthesized(nextNode)) {
return rangeEndIsOnSameLineAsRangeStart(previousNode, nextNode, currentSourceFile!) ? 0 : 1;
else if (!nodeIsSynthesized(previousNode) && !nodeIsSynthesized(nextNode)) {
if (preserveSourceNewlines && siblingNodePositionsAreComparable(previousNode, nextNode)) {
return getEffectiveLines(
includeComments => getLinesBetweenRangeEndAndRangeStart(
previousNode,
nextNode,
currentSourceFile!,
includeComments));
}
// If `preserveSourceNewlines` is `false` we do not intend to preserve the effective lines between the
// previous and next node. Instead we naively check whether nodes are on separate lines within the
// same node parent. If so, we intend to preserve a single line terminator. This is less precise and
// expensive than checking with `preserveSourceNewlines` as above, but the goal is not to preserve the
// effective source lines between two sibling nodes.
else if (!preserveSourceNewlines && originalNodesHaveSameParent(previousNode, nextNode)) {
return rangeEndIsOnSameLineAsRangeStart(previousNode, nextNode, currentSourceFile!) ? 0 : 1;
}
// If the two nodes are not comparable, add a line terminator based on the format that can indicate
// whether new lines are preferred or not.
return format & ListFormat.PreferNewLine ? 1 : 0;
}
else if (synthesizedNodeStartsOnNewLine(previousNode, format) || synthesizedNodeStartsOnNewLine(nextNode, format)) {
return 1;
Expand Down Expand Up @@ -5300,11 +5310,14 @@ namespace ts {

}

function siblingNodePositionsAreComparable(previousNode: Node, nextNode: Node) {
if (nodeIsSynthesized(previousNode) || nodeIsSynthesized(nextNode)) {
return false;
}
function originalNodesHaveSameParent(nodeA: Node, nodeB: Node) {
nodeA = getOriginalNode(nodeA);
// For performance, do not call `getOriginalNode` for `nodeB` if `nodeA` doesn't even
// have a parent node.
return nodeA.parent && nodeA.parent === getOriginalNode(nodeB).parent;
}

function siblingNodePositionsAreComparable(previousNode: Node, nextNode: Node) {
if (nextNode.pos < previousNode.end) {
return false;
}
Expand Down
20 changes: 20 additions & 0 deletions src/testRunner/unittests/transform.ts
View file
Open in desktop
Expand Up @@ -154,6 +154,26 @@ namespace ts {
}).outputText;
});

testBaseline("issue44068", () => {
return transformSourceFile(`
const FirstVar = null;
const SecondVar = null;
`, [
context => file => {
const firstVarName = (file.statements[0] as VariableStatement)
.declarationList.declarations[0].name as Identifier;
const secondVarName = (file.statements[0] as VariableStatement)
.declarationList.declarations[0].name as Identifier;

return context.factory.updateSourceFile(file, file.statements.concat([
context.factory.createExpressionStatement(
context.factory.createArrayLiteralExpression([firstVarName, secondVarName])
),
]));
}
]);
});

testBaseline("rewrittenNamespace", () => {
return transpileModule(`namespace Reflect { const x = 1; }`, {
transformers: {
Expand Down
3 changes: 2 additions & 1 deletion tests/baselines/reference/decoratorOnClassMethodThisParameter.js
View file
Open in desktop
Expand Up @@ -30,7 +30,8 @@ var C2 = /** @class */ (function () {
}
C2.prototype.method = function (allowed) { };
__decorate([
__param(0, dec), __param(1, dec)
__param(0, dec),
__param(1, dec)
], C2.prototype, "method", null);
return C2;
}());
3 changes: 1 addition & 2 deletions tests/baselines/reference/jsxJsxsCjsTransformNestedSelfClosingChild(jsx=react-jsx).js
View file
Open in desktop
Expand Up @@ -26,6 +26,5 @@ console.log(
exports.__esModule = true;
var jsx_runtime_1 = require("react/jsx-runtime");
console.log(jsx_runtime_1.jsx("div", { children: jsx_runtime_1.jsx("div", {}, void 0) }, void 0));
console.log(jsx_runtime_1.jsxs("div", { children: [jsx_runtime_1.jsx("div", {}, void 0),
jsx_runtime_1.jsx("div", {}, void 0)] }, void 0));
console.log(jsx_runtime_1.jsxs("div", { children: [jsx_runtime_1.jsx("div", {}, void 0), jsx_runtime_1.jsx("div", {}, void 0)] }, void 0));
console.log(jsx_runtime_1.jsx("div", { children: [1, 2].map(function (i) { return jsx_runtime_1.jsx("div", { children: i }, i); }) }, void 0));
3 changes: 1 addition & 2 deletions tests/baselines/reference/jsxJsxsCjsTransformNestedSelfClosingChild(jsx=react-jsxdev).js
View file
Open in desktop
Expand Up @@ -28,6 +28,5 @@ exports.__esModule = true;
var jsx_dev_runtime_1 = require("react/jsx-dev-runtime");
var _jsxFileName = "tests/cases/conformance/jsx/jsxs/jsxJsxsCjsTransformNestedSelfClosingChild.tsx";
console.log(jsx_dev_runtime_1.jsxDEV("div", { children: jsx_dev_runtime_1.jsxDEV("div", {}, void 0, false, { fileName: _jsxFileName, lineNumber: 6, columnNumber: 5 }, this) }, void 0, false, { fileName: _jsxFileName, lineNumber: 4, columnNumber: 13 }, this));
console.log(jsx_dev_runtime_1.jsxDEV("div", { children: [jsx_dev_runtime_1.jsxDEV("div", {}, void 0, false, { fileName: _jsxFileName, lineNumber: 12, columnNumber: 5 }, this),
jsx_dev_runtime_1.jsxDEV("div", {}, void 0, false, { fileName: _jsxFileName, lineNumber: 13, columnNumber: 5 }, this)] }, void 0, true, { fileName: _jsxFileName, lineNumber: 10, columnNumber: 13 }, this));
console.log(jsx_dev_runtime_1.jsxDEV("div", { children: [jsx_dev_runtime_1.jsxDEV("div", {}, void 0, false, { fileName: _jsxFileName, lineNumber: 12, columnNumber: 5 }, this), jsx_dev_runtime_1.jsxDEV("div", {}, void 0, false, { fileName: _jsxFileName, lineNumber: 13, columnNumber: 5 }, this)] }, void 0, true, { fileName: _jsxFileName, lineNumber: 10, columnNumber: 13 }, this));
console.log(jsx_dev_runtime_1.jsxDEV("div", { children: [1, 2].map(function (i) { return jsx_dev_runtime_1.jsxDEV("div", { children: i }, i, false, { fileName: _jsxFileName, lineNumber: 19, columnNumber: 21 }, _this); }) }, void 0, false, { fileName: _jsxFileName, lineNumber: 17, columnNumber: 13 }, this));
3 changes: 2 additions & 1 deletion tests/baselines/reference/objectLiteralShorthandPropertiesErrorFromNotUsingIdentifier.js
View file
Open in desktop
Expand Up @@ -35,7 +35,8 @@ var y = {
"typeof":
};
var x = (_a = {
a: a, : .b,
a: a,
: .b,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewer: It looks like these baseline changes are due to us no longer checking if previous and next nodes (when emitting the node list) are on the same line if either one does not have a parent set. We only check if nodes are on the same line if they have the same parent container.

It looks like the logic for checking if there is a parent set has been introduced as part of the following refactoring (not sure if this was intentional; or if it's reasonable to continue doing that). 68b0323.

a: a
},
_a["ss"] = ,
Expand Down
3 changes: 2 additions & 1 deletion tests/baselines/reference/objectLiteralShorthandPropertiesErrorWithModule.js
View file
Open in desktop
Expand Up @@ -25,7 +25,8 @@ var n;
(function (n) {
var z = 10000;
n.y = {
m: m, : .x // error
m: m,
: .x // error
};
})(n || (n = {}));
m.y.x;
3 changes: 2 additions & 1 deletion tests/baselines/reference/objectTypesWithOptionalProperties2.js
View file
Open in desktop
Expand Up @@ -42,5 +42,6 @@ var C2 = /** @class */ (function () {
return C2;
}());
var b = {
x: function () { }, 1: // error
x: function () { },
1: // error
};
3 changes: 1 addition & 2 deletions tests/baselines/reference/parserErrorRecovery_ObjectLiteral2.js
View file
Open in desktop
Expand Up @@ -3,5 +3,4 @@ var v = { a
return;

//// [parserErrorRecovery_ObjectLiteral2.js]
var v = { a: a,
"return": };
var v = { a: a, "return": };
3 changes: 3 additions & 0 deletions tests/baselines/reference/transformApi/transformsCorrectly.issue44068.js
View file
Open in desktop
@@ -0,0 +1,3 @@
const FirstVar = null;
const SecondVar = null;
[FirstVar, FirstVar];