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

fix: add preceding semicolon in suggestions of no-object-constructor #17649

Merged
merged 4 commits into from Oct 17, 2023
Merged

fix: add preceding semicolon in suggestions of no-object-constructor #17649

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b704d9a
Add semicolon in suggestions of `no-object-constructor`
fasttime Oct 14, 2023
fecad74
Apply suggestions from code review
fasttime Oct 15, 2023
7db860a
`output` in test suggestions
fasttime Oct 17, 2023
b8e2e32
Apply suggestions
fasttime Oct 17, 2023
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
93 changes: 88 additions & 5 deletions lib/rules/no-object-constructor.js
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@
// Requirements
//------------------------------------------------------------------------------

const { getVariableByName, isArrowToken } = require("./utils/ast-utils");
const { getVariableByName, isArrowToken, isClosingBraceToken, isClosingParenToken } = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Helpers
Expand Down Expand Up @@ -53,7 +53,8 @@ module.exports = {

messages: {
preferLiteral: "The object literal notation {} is preferable.",
useLiteral: "Replace with '{{replacement}}'."
useLiteral: "Replace with '{{replacement}}'.",
useLiteralAfterSemicolon: "Replace with '{{replacement}}', add preceding semicolon."
}
},

Expand All @@ -80,6 +81,72 @@ module.exports = {
return false;
}

/**
* Determines whether a parenthesized object literal that replaces a specified node needs to be preceded by a semicolon.
* @param {ASTNode} node The node to be replaced. This node should be at the start of an `ExpressionStatement` or at the start of the body of an `ArrowFunctionExpression`.
* @returns {boolean} Whether a semicolon is required before the parenthesized object literal.
*/
function needsSemicolon(node) {
const prevToken = sourceCode.getTokenBefore(node);

if (
!prevToken ||
prevToken.type === "Punctuator" && [":", ";", ">", "{", "=>", "++", "--"].includes(prevToken.value)
fasttime marked this conversation as resolved.
Show resolved Hide resolved
) {
return false;
}

const prevNode = sourceCode.getNodeByRangeIndex(prevToken.range[0]);

if (isClosingParenToken(prevToken)) {
return ![
"DoWhileStatement",
"ForInStatement",
"ForOfStatement",
"ForStatement",
"IfStatement",
"WhileStatement",
"WithStatement"
].includes(prevNode.type);
fasttime marked this conversation as resolved.
Show resolved Hide resolved
}

if (isClosingBraceToken(prevToken)) {
return (
prevNode.type === "BlockStatement" && prevNode.parent.type === "FunctionExpression" ||
prevNode.type === "ClassBody" && prevNode.parent.type === "ClassExpression" ||
prevNode.type === "ObjectExpression"
);
}

if (["Identifier", "Keyword"].includes(prevToken.type)) {
if (["BreakStatement", "ContinueStatement"].includes(prevNode.parent.type)) {
return false;
}
fasttime marked this conversation as resolved.
Show resolved Hide resolved

// Keywords that can immediately precede an ExpressionStatement node, mapped to the their node types.
const nodeTypesByKeyword = {
__proto__: null,
break: "BreakStatement",
continue: "ContinueStatement",
debugger: "DebuggerStatement",
do: "DoWhileStatement",
else: "IfStatement",
return: "ReturnStatement",
yield: "YieldExpression"
};
const keyword = prevToken.value;
const nodeType = nodeTypesByKeyword[keyword];

return prevNode.type !== nodeType;
}

if (prevToken.type === "String") {
return !["ExportAllDeclaration", "ExportNamedDeclaration", "ImportDeclaration"].includes(prevNode.parent.type);
fasttime marked this conversation as resolved.
Show resolved Hide resolved
}

return true;
}

/**
* Reports on nodes where the `Object` constructor is called without arguments.
* @param {ASTNode} node The node to evaluate.
Expand All @@ -93,16 +160,32 @@ module.exports = {
const variable = getVariableByName(sourceCode.getScope(node), "Object");

if (variable && variable.identifiers.length === 0) {
const replacement = needsParentheses(node) ? "({})" : "{}";
let replacement;
let fixText;
let messageId;
fasttime marked this conversation as resolved.
Show resolved Hide resolved

if (needsParentheses(node)) {
replacement = "({})";
if (needsSemicolon(node)) {
fixText = ";({})";
messageId = "useLiteralAfterSemicolon";
} else {
fixText = "({})";
messageId = "useLiteral";
}
} else {
replacement = fixText = "{}";
messageId = "useLiteral";
}

context.report({
node,
messageId: "preferLiteral",
suggest: [
{
messageId: "useLiteral",
messageId,
data: { replacement },
fix: fixer => fixer.replaceText(node, replacement)
fix: fixer => fixer.replaceText(node, fixText)
}
]
});
Expand Down
256 changes: 255 additions & 1 deletion tests/lib/rules/no-object-constructor.js
View file
Open in desktop
Expand Up @@ -104,6 +104,260 @@ ruleTester.run("no-object-constructor", rule, {
output: "({} instanceof Object);"
}]
}]
}
},

...[

// Semicolon required before `({})` to compensate for ASI
{
code: `
foo
Object()
`
},
{
code: `
foo()
Object()
`
},
{
code: `
new foo
Object()
`
},
{
code: `
(a++)
Object()
`
},
{
code: `
++a
Object()
`
},
{
code: `
const foo = function() {}
Object()
`
},
{
code: `
const foo = class {}
Object()
`
},
{
code: `
foo = this.return
Object()
`
},
{
code: `
var yield = bar.yield
Object()
`
},
{
code: `
var foo = { bar: baz }
Object()
`
}
].map(props => ({
...props,
errors: [{
messageId: "preferLiteral",
suggestions: [{
desc: "Replace with '({})', add preceding semicolon.",
messageId: "useLiteralAfterSemicolon"
}]
fasttime marked this conversation as resolved.
Show resolved Hide resolved
}]
})),

...[

// No semicolon required before `({})` because ASI does not occur
{ code: "Object()" },
{
code: `
{}
Object()
`
},
{
code: `
function foo() {}
Object()
`
},
{
code: `
class Foo {}
Object()
`
},
{ code: "foo: Object();" },
{ code: "foo();Object();" },
{ code: "{ Object(); }" },
{ code: "if (a) Object();" },
{ code: "if (a); else Object();" },
{ code: "while (a) Object();" },
{
code: `
do Object();
while (a);
`
},
{ code: "for (let i = 0; i < 10; i++) Object();" },
{ code: "for (const prop in obj) Object();" },
{ code: "for (const element of iterable) Object();" },
{ code: "with (obj) Object();" },

// No semicolon required before `({})` because ASI still occurs
{
code: `
const foo = () => {}
Object()
`
},
{
code: `
a++
Object()
`
},
{
code: `
a--
Object()
`
},
{
code: `
function foo() {
return
Object();
}
`
},
{
code: `
function * foo() {
yield
Object();
}
`
},
{
code: `
do {}
while (a)
Object()
`
},
{
code: `
debugger
Object()
`
},
{
code: `
for (;;) {
break
Object()
}
`
},
{
code: `
for (;;) {
continue
Object()
}
`
},
{
code: `
foo: break foo
Object()
`
},
{
code: `
foo: while (true) continue foo
Object()
`
},
{
code: `
<foo />
Object()
`,
parserOptions: { ecmaFeatures: { jsx: true } }
},
{
code: `
<foo></foo>
Object()
`,
parserOptions: { ecmaFeatures: { jsx: true } }
},
{
code: `
const foo = bar
export { foo }
Object()
`,
parserOptions: { sourceType: "module" }
},
{
code: `
export { foo } from 'bar'
Object()
`,
parserOptions: { sourceType: "module" }
},
{
code: `
export * as foo from 'bar'
Object()
`,
parserOptions: { sourceType: "module" }
},
{
code: `
import foo from 'bar'
Object()
`,
parserOptions: { sourceType: "module" }
},
{
code: `
var yield = 5;

yield: while (foo) {
if (bar)
break yield
new Object();
}
`
}
].map(props => ({
...props,
errors: [{
messageId: "preferLiteral",
suggestions: [{
desc: "Replace with '({})'.",
messageId: "useLiteral"
}]
fasttime marked this conversation as resolved.
Show resolved Hide resolved
}]
}))
]
});