[no-unnecessary-condition] What's the point of ignoreRhs: false option?

[ark120202]

I am probably missing something, but reading through all of comments in original PR (#699) I couldn't find a reason to include ignoreRhs option. It probably made sense for strict-boolean-expressions, but I can't think of any cases where I'd want return items.length && items[0].toUpperCase() to be reported, since there is no condition part.

At the same time I think RHS should always be reported when used in a condition, like:

function test(foo: { bar: true } | undefined) {
  if (foo && foo.bar) {}
  //         ^^^^^^^
}

(note: items.length && items[0].toUpperCase() from docs isn't correct example for this rule, since toUpperCase returns possibly falsy value)

EDIT: Actually, I think it might make sense for this to be behavior for strict-boolean-expressions as well. Looks like that option was added just to avoid breaking change, see palantir/tslint#4447.

[sindresorhus]

Here's a use-case: xojs/eslint-config-xo-typescript#17

// @EdJoPaTo @yangmingshan

[bradzacher]

With rules it's much better to implement them with zero options and relax them over time as people find cases that are reasonable to want to ignore.

In this case, the rule was implemented strictly, making the assumption that you didn't use short circuiting - && is a boolean operator, after all.

Then someone decided to add the option, because they felt that it should be safe to allow short circuiting.

Whilst adding a new option is a minor feature bump, turning on the option as default is a major breaking bump.

I'm not sure if it's a good idea to turn it on by default - in 3.7 they are implementing optional chaining and nullish coalescing, which will handle many of the cases people use short circuiting for.

That's a point for discussion however

[ark120202]

In this case, the rule was implemented strictly, making the assumption that you didn't use short circuiting

I guess that's about strict-boolean-expressions rule? In no-unnecessary-condition it was there since the first commit: 5ec23e1#diff-1dcdebcfacc649bba21e6ffe12f28ba1R43.

&& is a boolean operator, after all

That's a good point, I can see how someone would like to enforce that and it fits strict-boolean-expressions well, but I don't see why it should be in no-unnecessary-condition

[bradzacher]

Sorry, I forget which one had what at PR time.
In that case it was that the PR author created the rule via copy pasting strict-boolean-expressions, and then didn't think about changing the option to be default on.
I think I had a busy week, so I didn't review it in depth enough to make a decision on if the options should be on or if it should even exist.

I'm not sure if there is a case for having the option turned off in no-unnecessary-condition.

[Retsam]

Original author here, unless I'm misreading @ark120202's comment, it's not a question of changing the default; it's that the option is unnecessary and can be removed altogether.

And I think they're right. Instead of using an option to determine when the right-hand-side should be checked, it should be done by the context of the expression.

In the context of a condition, (e.g. if(foo && bar)) it should always check bar to ensure the condition is necessary. In other contexts (e.g. return foo && bar; it doesn't ever make sense to check bar.

I'll see if I can make the changes to make this happen. I'm not sure if it'll count as a breaking change to remove an option and unify both cases.

[bradzacher]

Related: #1118

Removing an option is a breaking change. It is not backward compatible.

[bradzacher]

Commenting on an issue like this is not the place to report or discuss unrelated bugs.
Next time start with opening a new issue and fill out the template, so we can look into the problem with all the information required.