Incorrect "Confusing use of '!'" warning #780

Closed
cowwoc opened this Issue Dec 19, 2012 · 12 comments

Comments

Projects
None yet
@cowwoc

cowwoc commented Dec 19, 2012

When processing this code:

Array.prototype.minus = function(other)
{
    "use strict";
    return this.filter(function(i)
    {
        return !(other.indexOf(i) > -1);
    });
};

JSHint complains about the use of !. Surrounding it with brackets does not help. I believe this is a bug.

@Skalman

This comment has been minimized.

Show comment Hide comment
@Skalman

Skalman Dec 20, 2012

This is not a bug: it would be better to write the same expression as return other.indexOf(i) <= -1, though in this particular case it might be clearer with simply return other.indexOf(i) === -1.

That said, would it be possible for JSHint to also give a tip on how to make it non-confusing?

Skalman commented Dec 20, 2012

This is not a bug: it would be better to write the same expression as return other.indexOf(i) <= -1, though in this particular case it might be clearer with simply return other.indexOf(i) === -1.

That said, would it be possible for JSHint to also give a tip on how to make it non-confusing?

@cowwoc

This comment has been minimized.

Show comment Hide comment
@cowwoc

cowwoc Dec 20, 2012

Agreed. If you give a tip on how to make it non-confusing it would be more obvious what is meant by this report.

cowwoc commented Dec 20, 2012

Agreed. If you give a tip on how to make it non-confusing it would be more obvious what is meant by this report.

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Dec 25, 2012

Has the warning been removed altogether?
console.log(!!5); does not give any warning to me, though I expected the error message

ghost commented Dec 25, 2012

Has the warning been removed altogether?
console.log(!!5); does not give any warning to me, though I expected the error message

@cowwoc

This comment has been minimized.

Show comment Hide comment
@cowwoc

cowwoc Dec 25, 2012

hg97,

Try the testcase I included with my original post. I believe special handling was added for !! but not !.

cowwoc commented Dec 25, 2012

hg97,

Try the testcase I included with my original post. I believe special handling was added for !! but not !.

@coolaj86

This comment has been minimized.

Show comment Hide comment
@coolaj86

coolaj86 Feb 8, 2013

Here's a use of ! that jshint thinks is confusing, but seems to be the simplest way to go about the problem:

var foo;
console.log(foo <= 7, !(foo > 7));
// false true

I'd really like to be able to turn confusing ! checking off, but I didn't see an option for it in the docs.

coolaj86 commented Feb 8, 2013

Here's a use of ! that jshint thinks is confusing, but seems to be the simplest way to go about the problem:

var foo;
console.log(foo <= 7, !(foo > 7));
// false true

I'd really like to be able to turn confusing ! checking off, but I didn't see an option for it in the docs.

@Skalman

This comment has been minimized.

Show comment Hide comment
@Skalman

Skalman Feb 8, 2013

While it's true that comparisons with NaN always are false, I'd still consider !(foo > 7) confusing. Just reading the code, it's not clear (to me) that you intend for every non-numeric value to give true. I'd probably just explicitly check for expected non-numeric values. foo == null || foo <= 7

Skalman commented Feb 8, 2013

While it's true that comparisons with NaN always are false, I'd still consider !(foo > 7) confusing. Just reading the code, it's not clear (to me) that you intend for every non-numeric value to give true. I'd probably just explicitly check for expected non-numeric values. foo == null || foo <= 7

@Nickwiz

This comment has been minimized.

Show comment Hide comment
@Nickwiz

Nickwiz Dec 17, 2013

Frequently use things like if (something && !(foo % 16)) over if (something && (foo % 16 === 0)). IMHO the former is both clear an concise.

Nickwiz commented Dec 17, 2013

Frequently use things like if (something && !(foo % 16)) over if (something && (foo % 16 === 0)). IMHO the former is both clear an concise.

@popham

This comment has been minimized.

Show comment Hide comment
@popham

popham May 19, 2014

See comments for how to disable: /*jshint -W018 */. (I immediately reenable after the instance with a /*jshint +W018 */.)

popham commented May 19, 2014

See comments for how to disable: /*jshint -W018 */. (I immediately reenable after the instance with a /*jshint +W018 */.)

@hbsdev

This comment has been minimized.

Show comment Hide comment
@hbsdev

hbsdev Jul 3, 2014

For anybody who needs a quick but valid workaround. Rewrite this:

if (!(somevariable > 0)) {
  something();
}

to the more explicit and no longer confusing:

// see https://github.com/jshint/jshint/issues/780
var my_condition = (somevariable > 0);
if (!my_condition) {
  something();
}

There may be situations when you are in a hurry and not all people can do inversions of logical operations and be absolutely sure that they do not miss some case where the variables have undefined or null values.

Also this code is nice and explicit and no longer confusing, so jshint is truly satisfied.

You want to refactor but not change the behaviour of the code, which probably someone else wrote and probably it is not covered by a test.

Btw the /*jshint +W018 */ throws an error:
Bad option: '+W018'.

The /*jshint -W018 */ does work.

hbsdev commented Jul 3, 2014

For anybody who needs a quick but valid workaround. Rewrite this:

if (!(somevariable > 0)) {
  something();
}

to the more explicit and no longer confusing:

// see https://github.com/jshint/jshint/issues/780
var my_condition = (somevariable > 0);
if (!my_condition) {
  something();
}

There may be situations when you are in a hurry and not all people can do inversions of logical operations and be absolutely sure that they do not miss some case where the variables have undefined or null values.

Also this code is nice and explicit and no longer confusing, so jshint is truly satisfied.

You want to refactor but not change the behaviour of the code, which probably someone else wrote and probably it is not covered by a test.

Btw the /*jshint +W018 */ throws an error:
Bad option: '+W018'.

The /*jshint -W018 */ does work.

@poppinlp

This comment has been minimized.

Show comment Hide comment
@poppinlp

poppinlp Jan 21, 2016

I try if (!+n) and if (!(+n)), but jshint show Confusing use of '!'.
Wanna some help.
jshint version 2.9.1

I try if (!+n) and if (!(+n)), but jshint show Confusing use of '!'.
Wanna some help.
jshint version 2.9.1

@jugglinmike

This comment has been minimized.

Show comment Hide comment
@jugglinmike

jugglinmike Jul 30, 2016

Owner

Given that there are numerous workarounds (including an explicit API to disable
the warning--/* jshint -W018 */, I don't think there is any further action
required here.

Owner

jugglinmike commented Jul 30, 2016

Given that there are numerous workarounds (including an explicit API to disable
the warning--/* jshint -W018 */, I don't think there is any further action
required here.

@englishextra englishextra referenced this issue in eligrey/classList.js May 4, 2017

Open

Fixing "Confusing use of '!'" warning #65

@SkyLeach

This comment has been minimized.

Show comment Hide comment
@SkyLeach

SkyLeach Oct 15, 2017

I was recently reminded of this issue when dealing with some d3js code and I saw this:

    node.append("text")
        .attr("dy", "0.31em")
        /* warning next 2 lines */
        .attr("x", function(d) { return d.x < Math.PI === !d.children ? 6 : -6; })
        .attr("text-anchor", function(d) { return d.x < Math.PI === !d.children ? "start" : "end"; })
        .attr("transform", function(d) { return "rotate(" + (d.x < Math.PI ? d.x - Math.PI / 2 : d.x + Math.PI / 2) * 180 / Math.PI + ")"; })
        .text(function(d) {
            return document.comment_data.comment_dict[d.data.id].user;
        })
        .attr('id',function(d){return 'nodetext_'+d.data.id;}); //add an id to each nodetext for reference selection

For the most part I agree with @jugglinmike, however in cases like this one the only available alternatives would be considerably less concise:

        .attr("x", function(d) {return ((d.x < Math.PI && !d.children)||(d.x>=Math.PI && d.children)) ? 6 : -6;})
        .attr("text-anchor", function(d) { return ((d.x < Math.PI && !d.children)||(d.x>=Math.PI && d.children)) ? "start" : "end"; })

SkyLeach commented Oct 15, 2017

I was recently reminded of this issue when dealing with some d3js code and I saw this:

    node.append("text")
        .attr("dy", "0.31em")
        /* warning next 2 lines */
        .attr("x", function(d) { return d.x < Math.PI === !d.children ? 6 : -6; })
        .attr("text-anchor", function(d) { return d.x < Math.PI === !d.children ? "start" : "end"; })
        .attr("transform", function(d) { return "rotate(" + (d.x < Math.PI ? d.x - Math.PI / 2 : d.x + Math.PI / 2) * 180 / Math.PI + ")"; })
        .text(function(d) {
            return document.comment_data.comment_dict[d.data.id].user;
        })
        .attr('id',function(d){return 'nodetext_'+d.data.id;}); //add an id to each nodetext for reference selection

For the most part I agree with @jugglinmike, however in cases like this one the only available alternatives would be considerably less concise:

        .attr("x", function(d) {return ((d.x < Math.PI && !d.children)||(d.x>=Math.PI && d.children)) ? 6 : -6;})
        .attr("text-anchor", function(d) { return ((d.x < Math.PI && !d.children)||(d.x>=Math.PI && d.children)) ? "start" : "end"; })

danielctull pushed a commit to danielctull-forks/jshint that referenced this issue Jan 14, 2018

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