Rule suggestion: disallow !a instanceof b

[tornewuff]

As the existing rule no-negated-in-lhs, it seems like it would be good to also disallow

if (!a instanceof b) // do something

since the developer certainly meant to write

if (!(a instanceof b)) // do something

instead. This prevents errors, and there's no legitimate case for the former that I can think of, so I think this could be enabled by default as with no-negated-in-lhs. I've thrown together a quick copy and paste job of no-negated-in-lhs which works; will attach PR.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[michaelficarra]

We'd probably just want this behaviour in the same rule. I don't see why anyone would turn on one and not the other.

[tornewuff]

Fair enough; just extend the same rule despite the name?

[michaelficarra]

That'd be my preference.

[nzakas]

I'm not sure I agree. The rule is specifically about in, it even says so in the name. I don't see how we can rationally extend the current rule without confusion.

We either need to rename the rule or create a new one.

@tornewuff in the future, please wait until the issue has been labeled "approved" before starting work.

[michaelficarra]

@nzakas Names don't matter. I think we can all agree that this proposed functionality is desired, and it does not need to be separately configurable from this rule. Is your only concern that people will assume one kind of behaviour from the name, and observe something else? Because we have documentation for that reason.

[tornewuff]

Happy to do it as a new rule instead, but I don't know what the process is for renaming a rule if we want to go that way.

(I don't generally wait for an issue tracker discussion before preparing a patch if it's only going to take five minutes to write; if it's not accepted that's fine).

[nzakas]

@michaelficarra names definitely do matter, otherwise we would just do W012 as JSHint does. We've seen over and over that people don't necessarily read the documentation for every rule they use, and we have gotten reports that rules don't say what that mean.

@tornewuff I'd just do a new rule, it's easier all around. Regarding the label, that's fine, I just try to be mindful of people's time.

[Schoonology]

@tornewuff Were you going to submit a new PR for the fix? I was just looking for this rule, stumbled upon this issue, and would be happy to submit a PR myself if you've not started in on the work already.

[platinumazure]

@tornewuff @Schoonology This is something I would also be interested in implementing. Do either of you still want this?

[ilyavolodin]

@platinumazure Issue has been accepted, so it's just waiting for PR at this point.

[platinumazure]

@ilyavolodin I understand. I was just asking if the two people who had expressed an interest were still interested, in case they had been making a work in progress.

Since we haven't heard from either of them in nearly two months, I'll get started on this.

[platinumazure]

Okay, so another possible approach was brought up in the pull request. Deprecate no-negated-in-lhs, deprecate no-negated-instanceof-lhs if that lands before this approach is considered, and add a new rule no-unsafe-negation which would cover both of the binary expressions and also anything else we might want to add. I don't know the process for deprecating rules, though.

Any thoughts on this approach?

[Schoonology]

I think there's a Long and Glorious Tradition of similar practices, and it makes a lot of sense to me. It's the same lint, just found around two different keywords (in and instanceof).

[platinumazure]

There hasn't been much discussion since I suggested deprecating no-negated-in-lhs in favor of something more generic that operates on specific binary expressions. Anyone else have thoughts on that?

[nzakas]

I'd say go forward with no-unsafe-negation.

[platinumazure]

I know we already agreed on no-unsafe-negation, but I'm wondering if no-unsafe-logical-negation might be better here, just for clarity. Or is that too long?

[nzakas]

I don't think logical adds any further clarity, so if prefer to stick with the decision.

[platinumazure]

As opposed to arithmetic negation, I mean (- unary operator).

[michaelficarra]

Or bitwise negation (~).

[nzakas]

The reason we're creating a new rule is because we were overly specific with the name of the previous one. I think I've learned my lesson there.

[michaelficarra]

What's in a name? That which we call a rose by any other name would smell as sweet.

[ilyavolodin]

@michaelficarra but names are used for communication. And we have too many rules for users to smell:-)

[mysticatea]

How about no-unexpected-unary-ops?

[nzakas]

Are there other unary ops we care about other than "!"?

[mysticatea]

I'm not sure.
Sorry I didn't understand this thread.

[platinumazure]

I'm busy for most of the day but I will try to weigh in with more detail
later. I do remember going through an operator precedence table to see if
it was worth flagging other unary ops with lower precedence than binary ops
and this seemed to be the only really important one. Less important ones
might have been relational binary operators (e.g., !a > b) though.
On Nov 28, 2015 9:15 AM, "Toru Nagashima" notifications@github.com wrote:

I'm not sure.
Sorry I didn't understand this thread.

—
Reply to this email directly or view it on GitHub
#2716 (comment).

[michaelficarra]

It's the combination of unary ops and binary ops whose types don't align. For example, ! works with ===, but not in, +, <, etc; ~ works with +, *, ===, etc. but not in, <, instanceof, etc.

[nzakas]

For this, we are strictly talking about using ! with in and instanceof, though. I'm not sure it makes sense to extend the rule further than that.

I'm still thinking that no-unsafe-negation is clearest for this use case.

[platinumazure]

I'm okay with that for now, but I think it would be good to revisit this in 2.0 (as more of a general-purpose unary ops with higher precedence than binary ops sort of thing).

[nzakas]

This is for 2.0.0, and I'd prefer not to revisit again after this. If you have a proposal for something more than what is here, then now would be the best time to share. We can't keep deprecating and removing rules that do the same thing without seriously annoying users.

[alberto]

Closed by lack of activity

[mysticatea]

@alberto I think this should be reopened.

[nzakas]

@mysticatea can you please share why you think this should be reopened? There was no activity in 8 months.

[mysticatea]

@nzakas I think this is a rule to prevent enough common problem and this is accepted.

Accepted issues are good start points for new contributors, but until now, rule's deprecation process is not clear; I guess it was a barrier to contributing to this issue. Just now, we are making some rules deprecated. I think we can go to advance this as well. (I can work on this later)

[nzakas]

@mysticatea we have "help wanted" issues for new contributors and our policy has been to close accepted issues if they have been open for 30 days without being completed. This is pretty far beyond 30 days. :)

If you want to implement it, that's fine, just be sure to say that when you reopen an issue.

[tornewuff]

Oops, sorry for disappearing here, and thanks @mysticatea for implementing it - I was away from github stuff for a while and forgot about this entirely when coming back.