wcjohnson / lightscript

A futuristic fork of the LightScript language.
http://wcjohnson.github.io/lightscript
7 stars 0 forks source link

Lint rule: `no-confusing-subscript-indentation` #92

Open wcjohnson opened 6 years ago

wcjohnson commented 6 years ago

We should point out the difference between

a
.b
.c()

and

a
.b
[c]
.d()

so that someone doesn't step on a land mine by inserting a bracket into a non-indented subscript chain.

We can suggest that the user either indent to subscript, add a newline for a separate expr, or put the bracket on the same line with its parent.

Since this rule is preventing a possible error it should be enabled by default.

rattrayalex commented 6 years ago

I think if we want to do something other than https://github.com/wcjohnson/lightscript/issues/93, I would recommend enforcing that the opening square bracket of a member expression is on the same line as the object, like this:

// illegal
a
  [b]

// legal (1)
a[b]

// legal (2)
a[
  b
]

I'm not sure whether to autofix to (1) or (2), but we should autofix to one of them.

Maybe something like, if it's at the end of a member expression chain, autofix to (1); if it's followed by a non-computed member expression (ie; a dot), autofix to (2). Probably don't want to get too clever though; ultimately we'll want prettier to make these decisions.

rattrayalex commented 6 years ago

Actually, poking around with prettier, it seems to have pretty good behavior here, choosing either something like

foo.d.d
  .baz[b]
  .d.a.q;

or

foo.d.baz[
  bdd
].d.a.q;

either way keeping the [ on the same line as the preceding part of the chain.

We could autofix to just (1) for now and recommend (2) in the docs for when it's needed.

Either way, I think a linter along these lines would really help this situation (though I still think we should include https://github.com/wcjohnson/lightscript/issues/93 anyway)

wcjohnson commented 6 years ago

The general plan would be to include all three rules in the linter, and let the user pick between them -- just as ESLint core bundles a whole bunch of style rules, most of which are disabled by default.

This one would be enabled by default, as it is the least strict of all the rules and it prevents a potentially serious error.

Unfortunately the fixing capabilities of eslint are not on par with those of prettier (it can't regenerate the output using the AST) and the transformation has to look like a series of text insertions before/after nodes. Something like "delete newline before bracket, insert newline and two spaces after bracket, insert newline after computed property node" which would make output like:

a
.b
[c]

goes to

a
.b[
  c
]

Although upon reflection, this may not be a case where autofixing is appropriate. For example, someone might write

object
.method()
[variable] = value

with the actual intent of the assignment being separate from the subscript, and then the autofix will have changed the semantics of their code.