Update: support logical assignments in core rules (refs #13569) #13618
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mdjermanovic
added
rule
1 task
btmills
reviewed
Aug 29, 2020
Comment on lines
+130
to
+149
| { | ||
| code: "class A extends (B += C) { constructor() { super(); } }", | ||
| errors: [{ messageId: "badSuper", type: "CallExpression" }] | ||
| }, | ||
| { | ||
| code: "class A extends (B -= C) { constructor() { super(); } }", | ||
| errors: [{ messageId: "badSuper", type: "CallExpression" }] | ||
| }, | ||
| { | ||
| code: "class A extends (B **= C) { constructor() { super(); } }", | ||
| errors: [{ messageId: "badSuper", type: "CallExpression" }] | ||
| }, | ||
| { | ||
| code: "class A extends (B |= C) { constructor() { super(); } }", | ||
| errors: [{ messageId: "badSuper", type: "CallExpression" }] | ||
| }, | ||
| { | ||
| code: "class A extends (B &= C) { constructor() { super(); } }", | ||
| errors: [{ messageId: "badSuper", type: "CallExpression" }] | ||
| }, |
There was a problem hiding this comment.
I agree we can say these 5 new errors are a semver-minor bug fix. Does anyone else object to that?
Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>
btmills
approved these changes
Aug 30, 2020
kaicataldo
approved these changes
Aug 31, 2020
|
Thanks for contributing! |
12 tasks
1 task
1 task
eslint-github-bot
bot
added
the
archived due to age
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[X] Other, please explain:
refs #13569
Changes the following rules and helper functions in order to support logical assignments:
operator-assignmentrule to NOT report&&=,||=, and??=for now.astUtils.couldBeErrorto returntruefor code such asfoo &&= 5. Also, to returnfalsefor code such asfoo += bar(this change can produce more warnings).constructor-superto treat classes such asclass A extends (B &&= 5)as those that extend a constructor. Also, to treat classes such asclass A extends (B += C)as those that don't extend a constructor (this change can produce more warnings).What changes did you make? (Give an overview)
Fixed the above, and also added tests for:
operator-linebreak,space-infix-ops,no-invalid-this,no-param-reassign,no-bitwise,func-names-matching,prefer-destrucuring, andno-extend-native.Is there anything you'd like reviewers to focus on?
operator-assignmentI'm not sure what would be the best course of action for the
operator-assignmentrule.Without any changes, that rule would (and currently does when used with
@babel/eslint-parser) reportx &&= y,x ||= y, andx ??= yif the option is"never", but notx = x && y,x = x || y, andx = x ?? yif the option is"always".This is certainly inconsistent and should be fixed, either to report both directions or neither of them.
I fixed the rule to ignore
x &&= y,x ||= y, andx ??= yfor now, with an idea to add an option for logical operators in a subsequent release. Logical assignments are a bit different from other assignments due to their short-circuiting semantics, so a separate configuration might make sense anyway.An alternative is to fix the rule to report
x = x && y,x = x || y, andx = x ?? yby default, but only ifecmaVersionis set to2021or above (which might be missing in a configuration for a third-party parser).complexityrule: logical assignments should probably increase complexity. This looks to me like a breaking change for users of@babel/eslint-parser, so I think we should make this change in the next major version, rather than now.no-constant-condition: This rule should probably report conditions such asif (foo ||= true) {}. However, the existing code which would be used for this change has some issues that should be fixed first (see Update: treat all literals like boolean literal in no-constant-condition #13245 (comment) and other comments below). I guess we could make this change later, either in a minor or major version.no-self-assign: It might make sense to consider reportingx &&= xand other logical assignments to itself.func-names:x &&= function () {}, and other two logical assignment operators, do assign function'snameautomatically. Our documentation foras-neededsays "requires function expressions to have a name, if the name cannot be assigned automatically in an ES6 environment". Should we reword this?AssigmentExpressionis always the=direct assignment. In other words, they don't check the assignment's operator, and thus have some false positives or false negatives with+=,-=etc. I fixed onlyastUtils.couldBeErrorandconstructor-supernow, because only these two wouldn't work well with logical assignments. For other rules, this possible oversight turned out to be correct behavior for&&=,||=, and??=, so I didn't fix them now for+=,-=and other operators (I'll do that in another PR, since it's an existing issue unrelated to logical assignments).