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

Make `no-cond-assign` work for ternaries too #10091

Closed
fvictorio opened this Issue Mar 15, 2018 · 13 comments

Comments

Projects
None yet
5 participants
@fvictorio

fvictorio commented Mar 15, 2018

What rule do you want to change?

no-cond-assign

Does this change cause the rule to produce more or fewer warnings?

More warnings

How will the change be implemented? (New option, new default behavior, etc.)?

I guess new default?

Please provide some example code that this change will affect:

  const str = (x = 0) ? 'zero' : 'non-zero'

What does the rule currently do for this code?

No warnings are produced

What will the rule do after it's changed?

Produce an "Unexpected assignment" warning for the condition of the ternary

@eslint eslint bot added the triage label Mar 15, 2018

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 18, 2018

Thanks for the proposal. I think your example is actually different from other cases that no-cond-assign reports, because it's parsed like this:

const str = (x = (0 ? 'zero' : 'non-zero'))

In other words, the "condition" of the ternary isn't the assignment x = 0, it's just the constant 0. So I would expect this to be reported by no-constant-condition, not no-cond-assign. It appears that no-constant-condition is already reporting it with the current implementation.

@fvictorio

This comment has been minimized.

fvictorio commented Mar 20, 2018

You are right, sorry. I meant (x = 0) ? 'zero' : 'non-zero'. I will edit the issue's description.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 21, 2018

I'm not sure if this will really be a worthwhile change, due to the fact that replacing two == with one = in many cases will change the precedence rules for the statement (as outlined by not-an-aardvark here). So I'm not sure that a change here would even work in most cases.

It might be better to use no-restricted-syntax to catch both of these cases, using selectors similar to the following:

  • AssignmentExpression[right.type='AssignmentExpression'][right.right.type='ConditionalExpression']
  • ConditionalExpression[test.type='AssignmentExpression']
@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 21, 2018

I think this is worth changing because it highlights a potential error -- in most cases, users probably didn't intend to use an assignment in a conditional.

@fvictorio

This comment has been minimized.

fvictorio commented Mar 21, 2018

replacing two == with one = in many cases will change the precedence rules for the statement

Yes, but I think many developers surround the condition in the ternary with parentheses, so in many other cases the rule will be useful.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 21, 2018

I won't stand in the way of this. If we can get a champion for this rule (or if not-an-aardvark, VictorHom, or Aladdin-ADD champion and we get one more 👍 on the original issue comment), then we can accept the proposal.

I do think we might need to bikeshed on which of the cases I highlighted above should be supported by this rule. It would be good to agree on that before we accept the proposal.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 21, 2018

Could you clarify which cases you're referring to? My working hypothesis is that the rule should always report ConditionalExpression nodes where the test property is an AssignmentExpression.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 21, 2018

@not-an-aardvark Agreed that we should definitely fix that case.

I'm more worried about AssignmentExpression[right.type='AssignmentExpression'][right.right.type='ConditionalExpression'], something like x = y = 0 ? true : false. Does that really fit in with this rule? Hard to say.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 21, 2018

I don't think so, I think that should be handled by no-constant-condition (or no-multi-assign if the user has that enabled).

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 21, 2018

Okay, I can get behind no-multi-assign flagging the latter case. We'll fix ConditionalExpression[test.type='AssignmentExpression'] here. I'll 👍 the opening comment. Now we just need a member of the team to champion.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 21, 2018

I'll champion it.

@not-an-aardvark not-an-aardvark self-assigned this Mar 21, 2018

@not-an-aardvark not-an-aardvark added accepted and removed evaluating labels Mar 21, 2018

goodoldneon added a commit to goodoldneon/eslint that referenced this issue Mar 21, 2018

goodoldneon added a commit to goodoldneon/eslint that referenced this issue Mar 21, 2018

@fvictorio

This comment has been minimized.

fvictorio commented Mar 21, 2018

Do you folks think this could be a good first contribution? I'd like to give it a try.

goodoldneon added a commit to goodoldneon/eslint that referenced this issue Mar 21, 2018

@ljharb

This comment has been minimized.

Contributor

ljharb commented Mar 21, 2018

@fvictorio looks like #10109 already attempts to handle it

@eslint eslint bot locked and limited conversation to collaborators Sep 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.