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
Update: support logical assignments in core rules (refs #13569) #13618
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these look good. Can I get a second set of eyes on the prefer-promise-reject-errors
case? I'm not confident there, which is why this is "Comment" rather than "Approve" or "Request changes".
{ | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice work @mdjermanovic!
Thanks for contributing! |
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-assignment
rule to NOT report&&=
,||=
, and??=
for now.astUtils.couldBeError
to returntrue
for code such asfoo &&= 5
. Also, to returnfalse
for code such asfoo += bar
(this change can produce more warnings).constructor-super
to 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-assignment
I'm not sure what would be the best course of action for the
operator-assignment
rule.Without any changes, that rule would (and currently does when used with
@babel/eslint-parser
) reportx &&= y
,x ||= y
, andx ??= y
if the option is"never"
, but notx = x && y
,x = x || y
, andx = x ?? y
if 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 ??= y
for 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 ?? y
by default, but only ifecmaVersion
is set to2021
or above (which might be missing in a configuration for a third-party parser).complexity
rule: 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 &&= x
and other logical assignments to itself.func-names
:x &&= function () {}
, and other two logical assignment operators, do assign function'sname
automatically. Our documentation foras-needed
says "requires function expressions to have a name, if the name cannot be assigned automatically in an ES6 environment". Should we reword this?AssigmentExpression
is 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.couldBeError
andconstructor-super
now, 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).