rome/tools

Implement ESLint rules

Closed this issue · 42 comments

sebmck commented

Followed is a list of ESLint rules that we should implement.

  • enforce return statements in getters (getter-return) #129
  • disallow using an async function as a Promise executor (no-async-promise-executor) #101
  • disallow comparing against -0 (no-compare-neg-zero) #106
  • disallow assignment operators in conditional expressions (no-cond-assign) #116
  • disallow the use of debugger (no-debugger) #103
  • disallow duplicate arguments in function definitions (no-dupe-args) #99
  • disallow duplicate keys in object literals (no-dupe-keys) #98
  • disallow a duplicate case label (no-duplicate-case) #100
  • disallow empty character classes in regular expressions (no-empty-character-class) #135
  • disallow reassigning exceptions in catch clauses (no-ex-assign) @weblancaster
  • disallow unnecessary boolean casts (no-extra-boolean-cast) @zinyando
  • disallow reassigning function declarations (no-func-assign) #130
  • disallow assigning to imported bindings (no-import-assign) #118
  • disallow variable or function declarations in nested blocks (no-inner-declarations) @tatchi
  • disallow invalid regular expression strings in RegExp constructors (no-invalid-regexp) @KevinKelbie
  • disallow multiple spaces in regular expression literals (no-regex-spaces) #121
  • disallow returning values from setters (no-setter-return) @torifat
  • disallow sparse arrays (no-sparse-arrays) #91
  • disallow unreachable code after return, throw, continue, and break statements (no-unreachable) @JayaKrishnaNamburu
  • disallow control flow statements in finally blocks (no-unsafe-finally) #117
  • disallow negating the left operand of relational operators (no-unsafe-negation) #89
  • disallow template literal placeholder syntax in regular strings (no-template-curly-in-string) #112
  • disallow deleting variables (no-delete-var) #95
  • disallow labels that are variable names (no-label-var) #102
  • disallow shadowing of restricted names (no-shadow-restricted-names) @MattsJones
  • suggest template literals instead of string concatenation (prefer-template) #138
  • (disallow-var) #107

Please comment if you want to work on one of these and I'll mark you to avoid duplicating work. Mentions next to rules indicate that someone else is working on it. If a PR isn't opened within a few days then it will be up for grabs.

A checkmark indicates an open PR.

yangshun commented

I'd like to try working on no-debugger

  • ❤️1
weblancaster commented

I started working on no-delete-var

JayaKrishnaNamburu commented

I would like to try working on no-unreachable

  • ❤️1
tanhauhau commented

I'd like to try on no-async-promise-executor

  • ❤️1
zinyando commented

I'd like to try taking on no-dupe-args

mattcompiles commented

Taking a look at no-dupe-keys

zinyando commented

I'm taking on no-duplicate-case

gobeli commented

Would take no-label-var

yangshun commented

I've added links to the ESLint rule documentation, src implementation and test implementation as it might be useful for people working on them.

  • enforce return statements in getters (getter-return)
  • disallow using an async function as a Promise executor (no-async-promise-executor)
  • disallow comparing against -0 (no-compare-neg-zero)
  • disallow assignment operators in conditional expressions (no-cond-assign)
  • disallow the use of debugger (no-debugger)
  • disallow duplicate arguments in function definitions (no-dupe-args)
  • disallow duplicate keys in object literals (no-dupe-keys)
  • disallow a duplicate case label (no-duplicate-case)
  • disallow empty character classes in regular expressions (no-empty-character-class)
  • disallow reassigning exceptions in catch clauses (no-ex-assign)
  • disallow unnecessary boolean casts (no-extra-boolean-cast)
  • disallow reassigning function declarations (no-func-assign)
  • disallow assigning to imported bindings (no-import-assign)
  • disallow variable or function declarations in nested blocks (no-inner-declarations)
  • disallow invalid regular expression strings in RegExp constructors (no-invalid-regexp)
  • disallow multiple spaces in regular expression literals (no-regex-spaces)
  • disallow returning values from setters (no-setter-return)
  • disallow sparse arrays (no-sparse-arrays)
  • disallow unreachable code after return, throw, continue, and break statements (no-unreachable)
  • disallow control flow statements in finally blocks (no-unsafe-finally)
  • disallow negating the left operand of relational operators (no-unsafe-negation)
  • disallow template literal placeholder syntax in regular strings (no-template-curly-in-string)
  • disallow deleting variables (no-delete-var)
  • disallow labels that are variable names (no-label-var)
  • disallow shadowing of restricted names (no-shadow-restricted-names)
  • suggest template literals instead of string concatenation (prefer-template)
  • 🎉5
  • ❤️6
yangshun commented

Some general observations/thoughts while working on the noDebugger rule:

  • Not clear how to test for both valid and invalid cases. Would be good to have a format similar to ESLint's where contributors just specify the valid and invalid patterns along with desired errors
  • Might want to sort the exports in packages/@romejs/js-compiler/transforms/lint/index.ts alphabetically to reduce the chance of merge conflicts. From looking at the PRs, there will be quite a few merge conflicts as most people add it to the bottom #132
  • Might want to sort the cases within packages/@romejs/js-compiler/__rtests__/lint.ts or start a convention of one test file per rule. Lowers the chance of merge conflicts and smaller files are easier to read #140
  • The unusedVariables rule is firing on the other lint rules tests. Would be nice to have a way to silence it for other tests
JayaKrishnaNamburu commented

Hey is anyone interested in discussing some pointers on implementation of no-unreachable implementation. I tried some way, but one way or the other few edge cases are failing when tested over eslint test rules 😓 😅

jloleysens commented

Taking no-compare-neg-zero

macovedj commented

@JayaKrishnaNamburu Sounds like one of the more fun ones. I'd be interested in thinking about it. Got a branch up yet?

  • 😄1
JayaKrishnaNamburu commented

Sure @macovedj i will update it tomorrow and let's discuss there 😄

sebmck commented

@JayaKrishnaNamburu

Hey is anyone interested in discussing some pointers on implementation of no-unreachable implementation. I tried some way, but one way or the other few edge cases are failing when tested over eslint test rules sweat sweat_smile

I don't think it would be as straightforward as ESLint since we don't have the same concept of code paths. It's fine if it's not as smart. Rome has a more sophisticated analysis system in js-analysis that we can utilize in the far future for something even better.

  • 👍1
sebmck commented

I've updated the main issue body with the latest PRs.

mattcompiles commented

My user is @MattsJones not @MattJones btw 😅

sebmck commented

Oops, sorry @MattJones 😛

  • ❤️1
eliassotodo commented

I will work on no-template-curly-in-string

wgardiner commented

I'll take no-func-assign

macovedj commented

no-cond-assign please.
There are two options, always and parens referenced in eslint. IMHO only always should be supported, but I'd be curious if there's a good argument for supporting parens.

sebmck commented

Yeah we would want the always/only option. Parens seems like an odd escape hatch. Just suppress it if it's what you actually want to mean.

JayaKrishnaNamburu commented

@JayaKrishnaNamburu

Hey is anyone interested in discussing some pointers on implementation of no-unreachable implementation. I tried some way, but one way or the other few edge cases are failing when tested over eslint test rules sweat sweat_smile

I don't think it would be as straightforward as ESLint since we don't have the same concept of code paths. It's fine if it's not as smart. Rome has a more sophisticated analysis system in js-analysis that we can utilize in the far future for something even better.

Sure, Thank you. I will continue my work then 👍 😄

mattcompiles commented

Happy to pickup no-shadow-restricted-names. ESLint currently only handles "undefined", "NaN", "Infinity", "arguments", "eval".

I was thinking we should block everything matching scope.getRootScope().isGlobal(name)?

sebmck commented

Sounds good to me!

weblancaster commented

Picking up no-ex-assign

emmatown commented

I'll pick up no-import-assign

zinyando commented

I'm taking on no-unsafe-finally

ikeryo1182 commented

I will pick up no-regex-spaces

torifat commented

I'm taking no-setter-return. Want to try a simple one first 🙂
I'm taking prefer-template

macovedj commented

I can do getter-return

sebmck commented

Updated the issue body. Thanks everyone! Super happy and appreciative of all the PRs so far!

  • 🎉6
Kelbie commented

I can do no-empty-character-class

tatchi commented

I would like to try no-inner-declarations (hopefully it's not too complex 😄)

zinyando commented

I'm going to try to work on no-extra-boolean-cast

macovedj commented

picking up prefer-template

Kelbie commented

I'll take on no-invalid-regexp

sebmck commented

There's been some changes to structure since this issue was created and many of the PRs submitted. I just created a PR that adds a simple rule #196 and included some instructions that will hopefully be helpful to others.

thecodrr commented

@sebmck can you please update the list? I see some rules in the list that are unchecked but are already implemented. Oh and I implemented no-inner-declarations rule (#305) since @tatchi never made a PR. So please update accordingly.

Edit: Actually, all of them are done after my PR. Unless you add some more, this can be closed?

tatchi commented

@thecodrr actually I did open a PR #142 but we decided to close it. Sorry, I should have added a comment here.

thecodrr commented

@tatchi and I should have confirmed (and researched) before jumping to implementation. 😆 Duplicate efforts. haha

sebmck commented

It actually looks like all these rules have been implemented or deferred. If anyone has any other ideas for lint rules, whether ones that already exist, or new ones that are made easier/justified with an easy autofix, please open an issue to discuss!

  • 🎉2