Tools: Implement ESLint rules

[sebmck]

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

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

I'd like to try working on no-debugger

[weblancaster]

I started working on no-delete-var

[JayaKrishnaNamburu]

I would like to try working on no-unreachable

[tanhauhau]

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

[zinyando]

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

[mattcompiles]

Taking a look at no-dupe-keys

[zinyando]

I'm taking on no-duplicate-case

[gobeli]

Would take no-label-var

[yangshun]

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

[yangshun]

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]

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]

Taking no-compare-neg-zero

[macovedj]

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

[JayaKrishnaNamburu]

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

[sebmck]

@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.

[sebmck]

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

[mattcompiles]

My user is @mattsjones not @MattJones btw 😅

[sebmck]

Oops, sorry @MattJones :stuck_out_tongue:

[bsotodo]

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

[wgardiner]

I'll take no-func-assign

[macovedj]

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]

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]

@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]

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]

Sounds good to me!

[weblancaster]

Picking up no-ex-assign

[mitchellhamilton]

I'll pick up no-import-assign

[zinyando]

I'm taking on no-unsafe-finally

[ikeryo1182]

I will pick up no-regex-spaces

[torifat]

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

[macovedj]

I can do getter-return

[sebmck]

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

[Kelbie]

I can do no-empty-character-class

[tatchi]

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

[zinyando]

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

[macovedj]

picking up prefer-template

[Kelbie]

I'll take on no-invalid-regexp

[sebmck]

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]

@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]

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

[thecodrr]

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

[sebmck]

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!