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

Bug: object-shorthand auto-fix removes comments #18441

Closed
1 task done
KubaJastrz opened this issue May 10, 2024 · 3 comments · Fixed by #18442
Closed
1 task done

Bug: object-shorthand auto-fix removes comments #18441

KubaJastrz opened this issue May 10, 2024 · 3 comments · Fixed by #18442
Assignees
@KubaJastrz
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly repro:yes

Comments

@KubaJastrz
Copy link
Contributor

KubaJastrz commented May 10, 2024 •
edited

Environment

Node version: any
npm version: any
Local ESLint version: 9.2.0
Global ESLint version: 9.2.0
Operating System: any

What parser are you using?

Default (Espree)

What did you do?

Run autofix on this:

/* eslint object-shorthand: ["error"] */

let b;

const a = {
  b: /* this comment will be auto-removed */ b,
  c: /* this comment will stay */ function () {}
};

What did you expect to happen?

All comments to stay:

/* eslint object-shorthand: ["error"] */

let b;

const a = {
  b: /* this comment will be auto-removed */ b,
  c: /* this comment will stay */ function () {}
};

What actually happened?

First comment got deleted:

/* eslint object-shorthand: ["error"] */

let b;

const a = {
  b,
  c: /* this comment will stay */ function () {}
};

Link to Minimal Reproducible Example

https://eslint.org/play/#eyJ0ZXh0IjoiLyogZXNsaW50IG9iamVjdC1zaG9ydGhhbmQ6IFtcImVycm9yXCJdICovXG5cbmxldCBiO1xuXG5jb25zdCBhID0ge1xuICBiOiAvKiBhc2QgKi8gYiwgICAvKiA8LS0gY2xpY2sgRklYIGhlcmUgKi9cblxuICBjOiAvKiBhc2QgKi8gZnVuY3Rpb24gKCkge30gICAgLyogPC0tIG5vIEZJWCBhdmFpbGFibGUgaGVyZSAqL1xufTtcblxuYTsiLCJvcHRpb25zIjp7InJ1bGVzIjp7fSwibGFuZ3VhZ2VPcHRpb25zIjp7InBhcnNlck9wdGlvbnMiOnsiZWNtYUZlYXR1cmVzIjp7fX19fX0=

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

I'm working on oxc-project/oxc#3211 and noticed this oversight in unit tests.

These cases are covered by eslint:

var x = { f: /* comment */ function() {} }
var x = { f /* comment */: function() {} }

But these aren't:

var x = { a: /* comment */ a }
var x = { a /* comment */: a }
@KubaJastrz KubaJastrz added bug ESLint is working incorrectly repro:needed labels May 10, 2024
@aladdin-add aladdin-add added accepted There is consensus among the team that this change meets the criteria for inclusion repro:yes and removed repro:needed labels May 11, 2024
@aladdin-add
Copy link
Member

Thanks, marking accepted. PRs are welcome!

@shulaoda
Copy link
Contributor

Maybe it's related to this place.

eslint/lib/rules/object-shorthand.js

Lines 259 to 262 in 069aa68

// key: /* */ () => {}
if (sourceCode.commentsExistBetween(lastKeyToken, node.value)) {
return null;
}

@KubaJastrz
Copy link
Contributor Author

@shulaoda Yes, but this particular part of code is handling function expressions only. The identifiers are handled here:

eslint/lib/rules/object-shorthand.js

Lines 493 to 516 in 069aa68

} else if (node.value.type === "Identifier" && node.key.name === node.value.name && APPLY_TO_PROPS) {
// {x: x} should be written as {x}
context.report({
node,
messageId: "expectedPropertyShorthand",
fix(fixer) {
return fixer.replaceText(node, node.value.name);
}
});
} else if (node.value.type === "Identifier" && node.key.type === "Literal" && node.key.value === node.value.name && APPLY_TO_PROPS) {
if (AVOID_QUOTES) {
return;
}
// {"x": x} should be written as {x}
context.report({
node,
messageId: "expectedPropertyShorthand",
fix(fixer) {
return fixer.replaceText(node, node.value.name);
}
});
}

I can make a PR later this weekend.

KubaJastrz added a commit to KubaJastrz/eslint that referenced this issue May 11, 2024
@KubaJastrz
fix: don't collapse comments in object-shorthand
5ee7f4d 
Fixes eslint#18441
@KubaJastrz KubaJastrz mentioned this issue May 11, 2024
Merged
1 task
@mdjermanovic mdjermanovic added the autofix This change is related to ESLint's autofixing capabilities label May 12, 2024
mdjermanovic pushed a commit that referenced this issue May 16, 2024
@KubaJastrz
fix: don't remove comments between key and value in object-shorthand (#…
5c28d9a 
…18442)

* fix: don't collapse comments in object-shorthand

Fixes #18441

* add missing test

* refactor

* fix: recognize comments inside value node
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@KubaJastrz KubaJastrz

Labels
accepted There is consensus among the team that this change meets the criteria for inclusion autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly repro:yes
Projects
Triage
Status: Complete
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: don't remove comments between key and value in object-shorthand KubaJastrz/eslint
4 participants
@KubaJastrz @aladdin-add @mdjermanovic @shulaoda