8000 [indent] incorrect treatment of multi-line parameter types · Issue #880 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]

Skip to content

[indent] incorr 8000 ect treatment of multi-line parameter types #880

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.

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

Closed
G-Rath opened this issue Aug 19, 2019 · 8 comments
Closed

[indent] incorrect treatment of multi-line parameter types #880

G-Rath opened this issue Aug 19, 2019 · 8 comments
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@G-Rath
Copy link
Contributor
G-Rath commented Aug 19, 2019

Just to throw my hat into the "rewrite indent" ring:

Repro

https://github.com/G-Rath/js-bugs/tree/typescript-eslint/issue-880

{
  "parser": "@typescript-eslint/parser",
  "plugins": [
    "@typescript-eslint"
  ],
  "rules": {
    "@typescript-eslint/indent": [ "error", 2 ]
  }
}
const j = (
  n: string,
  voo:
    | string
    | Record<string, string>
    | DependencyLock
//  ^ ESLint: Expected indentation of 2 spaces but found 4  @typescript-eslint/indent
) => {
  const version = typeof voo !== 'string'
    ? voo[n]
    : voo;

  return `${n}@${version}`;
};

Expected Result
No error.
Actual Result
ESLint: Expected indentation of 2 spaces but found 4 @typescript-eslint/indent
Additional Info
There were a couple of issues that looked very related, but none that quite hit the nail on the head; sorry if this is a duplicate 😬

Versions

package version
@typescript-eslint/eslint-plugin 2.0.0
@typescript-eslint/parser 2.0.0
TypeScript 3.5.3
ESLint 6.2.0
node 10.15.3
npm 6.10.3
@G-Rath G-Rath added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Aug 19, 2019
@bradzacher bradzacher added bug Something isn't working scope analyser Issues that are caused by bugs/incomplete cases in the scope analyser and removed triage Waiting for team members to take a look labels Aug 19, 2019
@bradzacher bradzacher added this to the indent rewrite milestone Aug 19, 2019
@viridia
Copy link
viridia commented Aug 19, 2019

I'm having this same problem - I'd like arrow function indents to be treated the same as function expressions.

What's particularly bad about this case is that you can't even disable the rule by turning off indent validation for function expressions; it means that you are basically forced into mis-indenting your code in order to get it to pass the linter.

@G-Rath
Copy link
Contributor Author
G-Rath commented Aug 19, 2019

@bradzacher let me know if there's anything I can help w/ for the rewrite - I've just finished rewriting eslint-plugin-jest to TS + @typescript-eslint, so I can dedicate myself to this for a bit if it'd help :)

@bradzacher bradzacher removed the scope analyser Issues that are caused by bugs/incomplete cases in the scope analyser label Aug 20, 2019
@bradzacher
Copy link
Member
bradzacher commented Aug 20, 2019

It's a pretty huge job, which is why I haven't worked on it.

The current rule implementation does some super duper dodgy stuff in the form of converting TSESTree AST nodes into "similar looking" standard ESTree AST nodes.

The problem with this is that not everything has a 1:1 representation, and some things look different.

For example - unions/intersections get converted to binary expressions. This kinda works, except for some cases where the syntax doesn't match up, causing the algorithm to go awry.
I.e. const x = | 1; is invalid javascript, but type X = | 1; is valid typescript. When you put this onto a new line, then getTokenBefore(node<1>) returns | instead of =.

This problem compounds a bit when you've got typescript specific nodes within typescript specific nodes. As you've then got hacked together indentation within hacked together indentation... It's messy.

On top of that, the base eslint rule is actually super relaxed, and doesn't check things that you'd expect.

For example - a roughly equivalent JS version of your example would be:

const j = (
  n = string,
  voo =
    		string
  | Record
    	| DependencyLock
) => {
  const version = typeof voo !== 'string'
    ? voo[n]
    : voo;

  return `${n}@${version}`;
};

Note the weird indentation I've put in? That's because eslint's indent rule doesn't actually validate the indentation of binary expressions, so that's valid according to the rule...
eslint demo repl


So what does this mean for the indent rule?
WELL I realised that the above approach worked for a few cases, but falls over very, very quickly as the number of cases balloons out.

So I decided that rather than calling eslint's implementation, it'd be better to just reimplement eslint's implementation, and then add true support for TS nodes (and probably fix up the weird, weird relaxed cases).

Step one of that was copy-pasting their code into this repo, and then converting it to typescript.
You can find that code in master here. OFC, the tests were copied over as well.

Step two is to start adding support for TS nodes.
This... this is what I never got back to.
The algorithm around how the indentation levels is stored is actually reasonably complicated, and the code isn't the nicest to read through and figure out.

I haven't had the motivation to sit down for the (what will probably be) 10+ hours to understand the algorithm, add in all of the typescript nodes, add tests, etc. Mainly because I don't use the rule any more - I just use prettier everywhere.


If you want to take this on, I'd be ecstatic!

It's on my todo list for the next time I'm stuck on a plane with nowhere else to go or do (or sometime like that).
Even just figuring out the basics and adding support for a single TS node would be a good start to someone else adding in the rest of the nodes.

8000

@G-Rath
Copy link
Contributor Author
G-Rath commented Aug 20, 2019

The algorithm around how the indentation levels is stored is actually reasonably complicated

Hmmm yes that is an algorithm alright 😂

I'll have a deeper look & maybe a play when I've got some spare time, but won't make promises at this point due to size & other commitments.

This might be obvious after a deeper look (or more general eslint/AST exp.), but currently the big question on my mind is:

add true support for TS nodes (and probably fix up the weird, weird relaxed cases).

What does that actually mean?

If I'm understanding you correctly, you've already converted the indent rule to TS, so ideally isn't it now a matter of implementing all the different TS options people want (as they come in)?

I suspect I've missed something, but if it is that, then I should able to semi-commit to incrementally tackle adding support for options as they come in 🙂

@bradzacher
Copy link
Member

isn't it now a matter of implementing all the different TS options people want?

Sounds simple right? 😃

Some of it will probably be simple. An interface/object type/enum can follow similar logic to objects, union/intersections follow similar logic to binary expressions, etc, etc. There's a bunch of prior art you can work from within the rule.

The problem is that it can be really ugly handling all of the cases properly for a single node.
For example - union/intersections. You have to setup different indentations for different ranges in the node depending on what line the range is on in relation to the parent.

   type T = foo | bar
//          ^^^^^^^^^ expected indent = 0

   type T = foo
//          ^^^ expected indent = 0
     | bar
//   ^^^^^ expected indent = 1

   type T =
     | foo
//   ^^^^^ expected indent = 1
     | bar
//   ^^^^^ expected indent = 1

The logic can be non-trivial 😅 for some nodes.
This is why I assume the rule has simple, relaxed logic in some cases.

@colthreepv
Copy link

Is there a way to ignore only the methods/functions part of indentation? so to suppress those kind of warnings without disabling the entirety of indent rule (that's a pretty big rule)

8000

@Tyriar
Copy link
Tyriar commented Mar 28, 2020

I'm hitting a similar problem:

export type TypedArray = Uint8Array | Uint16Array | Uint32Array | Uint8ClampedArray
  | Int8Array | Int16Array | Int32Array
  | Float32Array | Float64Array;

Errors on 2 last lines:

Expected indentation of 0 spaces but found 2.eslint(@typescript-eslint/indent)

@bradzacher
Copy link
Member

Merging into #1824

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

5 participants
0