[object-shorthand] autofix breaks arrow functions with function types

[nstepien]

What code were you trying to parse?

/* eslint object-shorthand: [1, "always", { avoidExplicitReturnArrows: true }] */
const test = {
  key: (arg: () => any) => {}
}

What did you expect to happen?
Autofix result should be

/* eslint object-shorthand: [1, "always", { avoidExplicitReturnArrows: true }] */
const test = {
  key(arg: () => any) {}
}

What actually happened?
Autofix gives

/* eslint object-shorthand: [1, "always", { avoidExplicitReturnArrows: true }] */
const test = {
  key(arg: () any) => {}
}

Versions

package	version
@typescript-eslint/parser	1.0.0
TypeScript	3.2.4
ESLint	5.12.1
node	11.7.0
npm	6.5.0

[platinumazure]

Seems this could be fixed upstream (i.e., in the core ESLint rule) if we are careful to look for and remove arrow tokens after the full range of the arguments section of the node, instead of just getting the first arrow token.

What happens if you have multiple arguments that are function types? Does it only remove the arrow for the first argument? If so, that supports my hypothesis.

All of this assumes the parser is returning the correct range info-- we should check that first. 😄

[nstepien]

/* eslint object-shorthand: [1, "always", { avoidExplicitReturnArrows: true }] */
const test = {
  key: (arg: () => any, arg2: () => any) => {}
}

becomes

/* eslint object-shorthand: [1, "always", { avoidExplicitReturnArrows: true }] */
const test = {
  key(arg: () any, arg2: () => any) => {}
}

---

Following your logic, I tried with a default parameter in a js file and it bugs out as well:

/* eslint object-shorthand: [1, "always", { avoidExplicitReturnArrows: true }] */
const test = {
  key: (arg = () => {}) => {}
}

becomes

/* eslint object-shorthand: [1, "always", { avoidExplicitReturnArrows: true }] */
const test = {
  key(arg = () {}) => {}
};

---

I'll open up an issue in the ESLint repo. I don't know if we should close this one yet, maybe once it's fixed in ESLint, it might still be broken with TS types.

[platinumazure]

Thanks @MayhemYDG for your tests, this all seems to point to my hypothesis as you noted.

@JamesHenry We don't have any labels related to the issue being blocked, or that this might be solvable in ESLint core. How do you want to handle the labeling of this issue? I agree with @MayhemYDG that it could be worthwhile to leave this open.

[j-f1]

Let’s close this for now since the issue is with ESLint. If the upstream fix doesn’t fix our case, please comment and we’ll reopen this.

[platinumazure]

@MayhemYDG Once you've opened the ESLint issue, please CC me-- I have some thoughts on how to fix. Thanks!

[nstepien]

Upstream issue: eslint/eslint#11305

[platinumazure]

For what it's worth: I used AST Explorer to check the parser output (although this was with typescript-eslint-parser rather than @typescript-eslint/parser) and the range info looks correct, so there's nothing we need to change there.

[nstepien]

@platinumazure @j-f1
I've updated to ESLint 5.13.0, which fixes most bugs with the rule, but unfortunately the rule's autofix is still broken if the function has a return type defined:

/* eslint object-shorthand: [1, "always", { avoidExplicitReturnArrows: true }] */
const test = {
  key: (arg: any): any => {}
}

becomes

const test = {
  key((arg: any): any) {}
}

instead of

const test = {
  key(arg: any): any {}
}

[bradzacher]

I put up a fix for this in the eslint repo:
eslint/eslint#12260

I'll close this, feel free to track the fix via the above PR.

If that PR gets rejected, I'll reopen this issue and we can bring the rule into the plugin.