Skip to content

feat(no-json-stringify-error): scope-aware suggestions + detail-preserving second suggestion #43748

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

Open
Copilot wants to merge 4 commits into
base: main
Choose a base branch
from
+375 −30
Open

feat(no-json-stringify-error): scope-aware suggestions + detail-preserving second suggestion #43748

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72df5b3
Initial plan
Copilot Jul 6, 2026
a882f0e
feat(no-json-stringify-error): add scope-aware suggestions and detail…
Copilot Jul 6, 2026
c014ca1
fix(no-json-stringify-error): use string concatenation for detail-pre…
Copilot Jul 6, 2026
9636d69
fix no-json-stringify-error review feedback
Copilot Jul 6, 2026
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
186 changes: 164 additions & 22 deletions eslint-factory/src/rules/no-json-stringify-error.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `try { f(); } catch (err) { core.error(String(err)); }`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "err" },
output: `try { f(); } catch (err) { core.error(getErrorMessage(err)); }`,
output: 'try { f(); } catch (err) { core.error(String(err) + "\\n" + (err.stack ?? "")); }',
},
],
},
Expand All @@ -99,9 +104,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "error" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "error" },
output: `try { f(); } catch (error) { core.error(\`details: \${String(error)}\`); }`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "error" },
output: `try { f(); } catch (error) { core.error(\`details: \${getErrorMessage(error)}\`); }`,
output: 'try { f(); } catch (error) { core.error(`details: ${String(error) + "\\n" + (error.stack ?? "")}`); }',
},
],
},
Expand All @@ -123,9 +133,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `p.catch(err => core.error(String(err)));`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "err" },
output: `p.catch(err => core.error(getErrorMessage(err)));`,
output: 'p.catch(err => core.error(String(err) + "\\n" + (err.stack ?? "")));',
},
],
},
Expand All @@ -147,9 +162,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `p.catch(function(err) { core.error(getErrorMessage(err)); });`,
output: `p.catch(function(err) { core.error(String(err)); });`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "err" },
output: 'p.catch(function(err) { core.error(String(err) + "\\n" + (err.stack ?? "")); });',
},
],
},
Expand All @@ -171,9 +191,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "outer" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "outer" },
output: `try { f(); } catch (outer) { try { g(); } catch (inner) { } core.error(getErrorMessage(outer)); }`,
output: `try { f(); } catch (outer) { try { g(); } catch (inner) { } core.error(String(outer)); }`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "outer" },
output: 'try { f(); } catch (outer) { try { g(); } catch (inner) { } core.error(String(outer) + "\\n" + (outer.stack ?? "")); }',
},
],
},
Expand Down Expand Up @@ -209,9 +234,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `p.then(result => result, err => core.error(String(err)));`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "err" },
output: `p.then(result => result, err => core.error(getErrorMessage(err)));`,
output: 'p.then(result => result, err => core.error(String(err) + "\\n" + (err.stack ?? "")));',
},
],
},
Expand All @@ -225,9 +255,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `p.then(null, err => core.error(getErrorMessage(err)));`,
output: `p.then(null, err => core.error(String(err)));`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "err" },
output: 'p.then(null, err => core.error(String(err) + "\\n" + (err.stack ?? "")));',
},
],
},
Expand All @@ -249,9 +284,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `p.then(null, function(err) { core.error(getErrorMessage(err)); });`,
output: `p.then(null, function(err) { core.error(String(err)); });`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "err" },
output: 'p.then(null, function(err) { core.error(String(err) + "\\n" + (err.stack ?? "")); });',
},
],
},
Expand Down Expand Up @@ -280,9 +320,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "e" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "e" },
output: `try { fetch(url); } catch (e) { console.error(getErrorMessage(e)); }`,
output: `try { fetch(url); } catch (e) { console.error(String(e)); }`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "e" },
output: 'try { fetch(url); } catch (e) { console.error(String(e) + "\\n" + (e.stack ?? "")); }',
},
],
},
Expand All @@ -296,9 +341,14 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `p.then(ok, err => console.error(getErrorMessage(err)));`,
output: `p.then(ok, err => console.error(String(err)));`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "err" },
output: 'p.then(ok, err => console.error(String(err) + "\\n" + (err.stack ?? "")));',
},
],
},
Expand All @@ -320,9 +370,101 @@ describe("no-json-stringify-error", () => {
data: { errorVar: "inner" },
suggestions: [
{
messageId: "useGetErrorMessage",
messageId: "useStringFallback",
data: { errorVar: "inner" },
output: `try { f(); } catch (outer) { try { g(); } catch (inner) { core.error(String(inner)); } }`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "inner" },
output: `try { f(); } catch (outer) { try { g(); } catch (inner) { core.error(getErrorMessage(inner)); } }`,
output: 'try { f(); } catch (outer) { try { g(); } catch (inner) { core.error(String(inner) + "\\n" + (inner.stack ?? "")); } }',
},
],
},
],
},
],
});
});

it("invalid: when getErrorMessage is in scope, suggests getErrorMessage and detail-preserving form", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] No ESM in-scope test: all three new test cases use cjsRuleTester. The ESM import path (import { getErrorMessage } from ...) is untested — scope analysis for import declarations differs from CJS require() and is worth an explicit esmRuleTester case.

💡 Suggested test addition 
it("invalid: when getErrorMessage is imported via ESM, first suggestion uses getErrorMessage", () => {
  esmRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
    valid: [],
    invalid: [
      {
        code: `import { getErrorMessage } from "./error_helpers"; try { f(); } catch (err) { core.error(JSON.stringify(err)); }`,
        errors: [{
          messageId: "jsonStringifyError",
          data: { errorVar: "err" },
          suggestions: [{
            messageId: "useGetErrorMessage",
            data: { errorVar: "err" },
            output: `import { getErrorMessage } from "./error_helpers"; try { f(); } catch (err) { core.error(getErrorMessage(err)); }`,
          }],
        }],
      },
    ],
  });
});

Without this, a regression in ESM scope detection would go unnoticed.

@copilot please address this.

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The three new "in scope" test cases use cjsRuleTester with CJS require() patterns. The scope-walk (context.sourceCode.getScope(node).variables) should also detect ES module import { getErrorMessage } from "..." bindings — but there is no test covering this path.

Adding a test with esmRuleTester:

it("invalid: when getErrorMessage is imported (ESM), suggests getErrorMessage", () => {
  esmRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
    valid: [],
    invalid: [
      {
        code: `import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { core.error(JSON.stringify(err)); }`,
        errors: [
          {
            messageId: "jsonStringifyError",
            data: { errorVar: "err" },
            suggestions: [
              { messageId: "useGetErrorMessage", data: { errorVar: "err" },
                output: `import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { core.error(getErrorMessage(err)); }` },
              { messageId: "useDetailPreservingForm", data: { errorVar: "err" },
                output: \`import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { core.error(getErrorMessage(err) + "\\n" + (err.stack ?? "")); }\` },
            ],
          },
        ],
      },
    ],
  });
});

Without this, a regression in ESM scope detection would go unnoticed.

@copilot please address this.

valid: [],
invalid: [
{
code: `const getErrorMessage = require('./error_helpers').getErrorMessage; try { f(); } catch (err) { core.error(JSON.stringify(err)); }`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "err" },
output: `const getErrorMessage = require('./error_helpers').getErrorMessage; try { f(); } catch (err) { core.error(getErrorMessage(err)); }`,
},
{
messageId: "useDetailPreservingForm",
data: { errorVar: "err" },
output: 'const getErrorMessage = require(\'./error_helpers\').getErrorMessage; try { f(); } catch (err) { core.error(getErrorMessage(err) + "\\n" + (err.stack ?? "")); }',
},
],
},
],
},
],
});
});

it("invalid: when getErrorMessage is in scope via destructured require, detail-preserving form uses getErrorMessage", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `const { getErrorMessage } = require('./error_helpers'); try { f(); } catch (error) { core.error(\`details: \${JSON.stringify(error, null, 2)}\`); }`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "error" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "error" },
output: `const { getErrorMessage } = require('./error_helpers'); try { f(); } catch (error) { core.error(\`details: \${getErrorMessage(error)}\`); }`,
},
{
messageId: "useDetailPreservingForm",
data: { errorVar: "error" },
output: 'const { getErrorMessage } = require(\'./error_helpers\'); try { f(); } catch (error) { core.error(`details: ${getErrorMessage(error) + "\\n" + (error.stack ?? "")}`); }',
},
],
},
],
},
],
});
});

it("invalid: when getErrorMessage is not in scope, first suggestion uses String() fallback", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `try { f(); } catch (err) { console.error(JSON.stringify(err)); }`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useStringFallback",
data: { errorVar: "err" },
output: `try { f(); } catch (err) { console.error(String(err)); }`,
},
{
messageId: "useDetailPreservingFormFallback",
data: { errorVar: "err" },
output: 'try { f(); } catch (err) { console.error(String(err) + "\\n" + (err.stack ?? "")); }',
},
],
},
Expand Down
53 changes: 46 additions & 7 deletions eslint-factory/src/rules/no-json-stringify-error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ export const noJsonStringifyErrorRule = createRule({
jsonStringifyError:
"JSON.stringify({{errorVar}}) produces {} for Error objects — Error properties (message, stack, etc.) are non-enumerable. Prefer getErrorMessage({{errorVar}}) from error_helpers.cjs or explicitly serialize a guarded value after narrowing it.",

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The jsonStringifyError diagnostic message always says "Prefer getErrorMessage({{errorVar}}) from error_helpers.cjs" regardless of whether getErrorMessage is in scope. When the scope check yields false, the first suggestion becomes String(err) — but the diagnostic still points the developer at getErrorMessage. The two signals disagree.

Consider a scope-neutral phrasing, e.g.:

"JSON.stringify({{errorVar}}) produces {} for Error objects — ... Use getErrorMessage({{errorVar}}) if it is imported, or String({{errorVar}}) as a safe import-free alternative."

Alternatively, register a second messageId (e.g. jsonStringifyErrorNoHelper) and emit it when getErrorMessageInScope is false.

@copilot please address this.

useGetErrorMessage: "Replace with getErrorMessage({{errorVar}}) — ensure getErrorMessage is imported from error_helpers.cjs.",
useStringFallback: "Replace with String({{errorVar}}) — getErrorMessage is not in scope; String() is a safe import-free alternative.",
useDetailPreservingForm: 'Replace with getErrorMessage({{errorVar}}) + "\\n" + ({{errorVar}}.stack ?? "") to preserve the stack trace — ensure getErrorMessage is imported from error_helpers.cjs.',

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/grill-with-docs] The jsonStringifyError primary message still says "Prefer getErrorMessage({{errorVar}})" unconditionally, even though the rule now sometimes suggests String() as the first option. The message misleads users in the no-scope case.

💡 Suggested fix

Consider splitting into two message variants, or make the primary message scope-neutral:

jsonStringifyError:
  "JSON.stringify({{errorVar}}) produces {} for Error objects — Error properties are non-enumerable. " +
  "Use the suggestions below to replace it with a safe alternative.",

This avoids confusing users who don't have getErrorMessage imported.

@copilot please address this.

useDetailPreservingFormFallback: 'Replace with String({{errorVar}}) + "\\n" + ({{errorVar}}.stack ?? "") to preserve the stack trace.',
},
},
defaultOptions: [],
Expand Down Expand Up @@ -125,18 +128,54 @@ export const noJsonStringifyErrorRule = createRule({
if (!caughtNames.has(firstArg.name)) return;

const errorVar = firstArg.name;

// Check whether getErrorMessage is resolvable in the current scope chain.
const getErrorMessageInScope = (() => {

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] No test for getErrorMessage declared inside the catch block itself. The scope walk starts at context.sourceCode.getScope(node) (the CallExpression) and walks upward, which means a local const getErrorMessage = ... inside the catch body would be found. That's probably the right behavior, but there's no test confirming it — or ruling it out.

💡 Why this matters

Consider:

try { f(); } catch (err) {
  const getErrorMessage = customHelper;
  core.error(JSON.stringify(err)); // would this pick useGetErrorMessage or useStringFallback?
}

Add a test to document the intended behavior (accept useGetErrorMessage here, or document why it shouldn't).

@copilot please address this.

let scope: ReturnType<typeof context.sourceCode.getScope> | null = context.sourceCode.getScope(node);
while (scope) {
if (scope.variables.some(v => v.name === "getErrorMessage")) return true;

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/codebase-design] scope.variables.some(v => v.name === "getErrorMessage") matches any variable named getErrorMessage — including uninitialized, re-assigned, or completely unrelated variables like let getErrorMessage = null. This could cause the rule to emit useGetErrorMessage when calling getErrorMessage(err) would throw at runtime.

💡 Consider checking for at least one write reference (i.e., an actual binding) 
// More precise: require at least one write reference so that uninitialized declarations don't count
if (scope.variables.some(v =>
  v.name === "getErrorMessage" && v.defs.length > 0
)) return true;

v.defs.length > 0 ensures the variable has an actual definition (import, const, function) rather than only being an identifier reference.

@copilot please address this.

scope = scope.upper;
}
return false;
})();

context.report({
node,
messageId: "jsonStringifyError",
data: { errorVar },
suggest: [
{
messageId: "useGetErrorMessage" as const,
data: { errorVar },
fix(fixer) {
return fixer.replaceText(node, `getErrorMessage(${errorVar})`);
},
},
// Suggestion 1: concise message-only replacement
getErrorMessageInScope
? ({
messageId: "useGetErrorMessage" as const,
data: { errorVar },
fix(fixer) {
return fixer.replaceText(node, `getErrorMessage(${errorVar})`);
},
} as const)
: ({
messageId: "useStringFallback" as const,
data: { errorVar },
fix(fixer) {
return fixer.replaceText(node, `String(${errorVar})`);
},
} as const),
// Suggestion 2: detail-preserving replacement (retains stack trace)
getErrorMessageInScope
? ({
messageId: "useDetailPreservingForm" as const,
data: { errorVar },
fix(fixer) {
return fixer.replaceText(node, `getErrorMessage(${errorVar}) + "\\n" + (${errorVar}.stack ?? "")`);
},
} as const)
: ({
messageId: "useDetailPreservingFormFallback" as const,
data: { errorVar },
fix(fixer) {
return fixer.replaceText(node, `String(${errorVar}) + "\\n" + (${errorVar}.stack ?? "")`);
},
} as const),
],
});
},
Expand Down
Loading