Our guidelines are fairly clear that if (!foo.length) is preferred over if (foo.length == 0) (and ditto for the "positive" ie non-0 length case), we should have an eslint rule (w/ --fix functionality) to enforce this.

(In reply to Monika Maheshwari [:MonikaMaheshwari] from comment #1)

Hi, can I work on this?

Assuming Mark agrees we want this, I believe so, but it's not a super easy bug. You'd want to make an eslint rule that lives in https://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/rules , that recognizes when people use a .length property and compare it with 0 using a comparison operator, provides appropriate feedback, and has a "fix" implementation that replaces it with the equivalent boolean operator.

Hi Monika, I'd be happy for you to work on this if you want to.

As Gijs has said, there's a bit more work to it than some bugs, but if you want to take it on, I'd be happy to help you. It shouldn't be too complex, the main thing is working out the right checks to use within the rule.

I think what we could do is break this down into two main parts: 1) have this bug which creates a rule and tests 2) have a new bug(s) to spread the rule throughout the tree.

I think the existing rule use-includes-instead-of-indexOf would be a good starting point. That is very similar to what we want to do here.

There's also a test for it. There are also details on how to run the tests here.

If it was me, I'd probably start with adding the various cases to a new test file, and create an empty rule. Then, get the rule working. Once that's done, then it can be made fixable.

The ESLint side has some good docs on creating rules, though really you probably just need to look at the examples in the rules directory. Something that is useful is the AST explorer, you can put a snippet of code into it, e.g. if (foo.length == 0) {} and then find out what the tree looks like, which helps with filling in the rule.

That's a bit of a brain dump about where to get started. If you want to ask me on irc, then I'm normally in the #eslint channel and you'll find me as Standard8 (though other people can sometimes help as well).

Anyway, let me know, as I've already said, I'll be happy to help.

Hi, Will go through the existing rules and tests created and will try to implement.

Monika is actively working on this, so adding as assigned.

I hope we don't have any length getters that return -1 in a default/error case… this seems a bit risky to change on arbitrary types in existing code but maybe it's not a real problem.

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #8)

Example: https://searchfox.org/mozilla-central/rev/0671407b7b9e3ec1ba96676758b33316f26887a4/devtools/shared/Parser.jsm#181
(though I don't know if length: -1 is ever returned or it's just a dummy default value that's never used)

I've filed bug 1563474 on that, especially as it appears to be unused code.

We had a discussion on irc about the potential for length values to be -1, and as a result this would cause the changes to fail.

The general consensus is that there is definitely a little risk here, but it is unlikely to be happening and shouldn't be encouraged. As Mossop said:

I think the only case we're concerned about is converting |if (foo.length > 0)| to |if (foo)| because that might inadvertently add negatives to the set returned. I think we should just go ahead and say that that is a real edge case and just go ahead .
I will go on record and say that having a length property that can return negative values is not something we should encourage.

Let me know when the reformatting has been done.

Turning if (length > 0) into if (length) seems like a really odd change to me. The two checks do not produce equivalent results for negative lengths. Granted, lengths should never be negative, but that's a semantic restriction based on what the variable represents. It seems odd to have a style rule whose application is dependent on the semantics of a variable.

Moreover, what if a bug somewhere results in something that does create a negative length, and as a result we now enter codepaths that we didn't before, and perform calculations with / index into arrays using the negative length? That seems dangerous to me, and doing that on the basis on style doesn't seem justified.

(In reply to Botond Ballo [:botond] [standards meeting July 15-20] from comment #13)

Turning if (length > 0) into if (length) seems like a really odd change to me.

So, to be clear, this is only about property accesses (normally on JS arrays, though there's no way for the linter to check that), not about random variables named "length"

The two checks do not produce equivalent results for negative lengths. Granted, lengths should never be negative, but that's a semantic restriction based on what the variable represents. It seems odd to have a style rule whose application is dependent on the semantics of a variable.

In JS this happens all the time... we use kFoo or SOME_FOO for constants, even if not declared as const (e.g. because we actually modify them in tests) - but there are no style rules for that because the use of either of those styles is not consistent in the codebase today, and because we've so far shied away from enforcing any styling wrt variable naming. We do have rules that e.g. enforce the use of Services.foo instead of doing the Cc[...].getService(Ci....) magic yourself, which is technically about the value/semantics of a particular variable, so this doesn't seem strange to me.

Moreover, what if a bug somewhere results in something that does create a negative length,

This can never happen with actual JS arrays. If it does, that's a JS engine bug and I sincerely hope the JS team itself has unit tests that catch it, and in any case, such an eventuality seems a poor basis for any other decisions (much) further up the food chain.

and as a result we now enter codepaths that we didn't before, and perform calculations with / index into arrays using the negative length? That seems dangerous to me, and doing that on the basis on style doesn't seem justified.

Again, the only way I see that happening is if there's a JS engine bug or the relevant object isn't actually a JS array. Either of those would be surprising.

I'll note that this pattern is currently pretty widespread in the codebase anyway, cf. the majorify of the hits in this (naive) searchfox query: https://searchfox.org/mozilla-central/search?q=.length)&case=false&regexp=false&path=browser%2F* .

(In reply to :Gijs (he/him) from comment #14)

Moreover, what if a bug somewhere results in something that does create a negative length,

This can never happen with actual JS arrays. If it does, that's a JS engine bug [...]

Sure, actual JS arrays are not what concern me.

and as a result we now enter codepaths that we didn't before, and perform calculations with / index into arrays using the negative length? That seems dangerous to me, and doing that on the basis on style doesn't seem justified.

Again, the only way I see that happening is if there's a JS engine bug or the relevant object isn't actually a JS array. Either of those would be surprising.

Why is it surprising to have a non-array object with a property named length? (e.g. say I have an object representing a line segment.)

(In reply to Botond Ballo [:botond] [standards meeting July 15-20] from comment #15)

and as a result we now enter codepaths that we didn't before, and perform calculations with / index into arrays using the negative length? That seems dangerous to me, and doing that on the basis on style doesn't seem justified.

Again, the only way I see that happening is if there's a JS engine bug or the relevant object isn't actually a JS array. Either of those would be surprising.

Why is it surprising to have a non-array object with a property named length? (e.g. say I have an object representing a line segment.)

That could be possible, but I don't know of any in the code base. What I'm starting to realise is important is highlighting differences from expectations. So if there was a non-array object with property length that doesn't behave the same, maybe that property should be called something else, or highlighted differently. That was it is abundantly clear what it is. Otherwise, someone might come along later, and without checking assume its an array or related to an array and modify it incorrectly.

(In reply to Botond Ballo [:botond] [standards meeting July 15-20] from comment #15)

Why is it surprising to have a non-array object with a property named length? (e.g. say I have an object representing a line segment.)

I've never needed or seen any code representing a line segment in JS (assuming we're talking "line" as in "line of text" ie what layout normally does for us, in C++...), so I would be surprised. Even if/where they exist, they are surely a tiny minority. It doesn't make sense to not improve readability and consistency elsewhere just for the sake of that minority. Of course, if there were a way to know or use type information in eslint then we could restrict the style rule to only builtin strings and arrays, but there is no such way, so we cannot.

If this is a serious concern for a given file or class, turn the rule off for those comparisons or that file (eslint provides a number of ways to do so). Some of us discussed the custom-object-with-length case on IRC and it was felt that it was niche enough not to block having a rule - as always, exceptions can be used when reasonable.

(In reply to :Gijs (he/him) from comment #0)

Our guidelines are fairly clear that if (!foo.length) is preferred over if (foo.length == 0) (and ditto for the "positive" ie non-0 length case), we should have an eslint rule (w/ --fix functionality) to enforce this.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices says

Do not compare x == true or x == false. Use (x) or (!x) instead. x == true, is certainly different from if (x)! Compare objects to null, numbers to 0 or strings to "", if there is chance for confusion.

I wouldn't take that to mean a preference if (a.length) over if (a.length > 0), etc. length == 0 is explicitly allowed. The "Use (x) or (!x) instead" instruction applies only when considering x as a boolean.

Are there other guidelines with a special case when the number is from a property and that property is called "length"?

(In reply to Karl Tomlinson (:karlt) from comment #18)

(In reply to :Gijs (he/him) from comment #0)

Our guidelines are fairly clear that if (!foo.length) is preferred over if (foo.length == 0) (and ditto for the "positive" ie non-0 length case), we should have an eslint rule (w/ --fix functionality) to enforce this.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices says

Do not compare x == true or x == false. Use (x) or (!x) instead. x == true, is certainly different from if (x)! Compare objects to null, numbers to 0 or strings to "", if there is chance for confusion.

I wouldn't take that to mean a preference if (a.length) over if (a.length > 0), etc. length == 0 is explicitly allowed. The "Use (x) or (!x) instead" instruction applies only when considering x as a boolean.

Are there other guidelines with a special case when the number is from a property and that property is called "length"?

No, I think I read the style guide differently from you. Specifically, I think that "if there is chance for confusion" is key. We routinely falsy-check objects (for being undefined/null) and strings (for being empty or undefined) rather than using error-prone == "" or == null comparisons. I think this is prevailing style in browser/. It's certainly what I have been told to do, and have been telling people to do, for all the years I've worked on Firefox frontend code.

The only case where there is a possibility for confusion is if a single variable is used with more than one type of value (e.g. mixing strings and numbers), or where context doesn't make it abundantly clear what type the variable would have. That's generally best avoided anyway, but where that is not possible, explicit comparisons that imply a type can be helpful.

In the array/string.length case, that is never so, because it is obvious to any reader of the code that .length will always be a number, so there is no need to make it explicit by comparing with a number.

Thank you for filling me in. Perhaps there is some folklore here, but it is not at all indicated by the style guide. There is a qualifier, but the presence of a qualifier does not imply that the reverse applies when the qualifier does not hold.

Is there a reason to prefer conversion to boolean for types that are known to be number, or is this about adding rules to provide consistency?

(In reply to Karl Tomlinson (:karlt) from comment #20)

Is there a reason to prefer conversion to boolean for types that are known to be number, or is this about adding rules to provide consistency?

I'm not sure I understand what you mean by "conversion to boolean" ? Any if condition implicitly converts to boolean (where necessary). You'd then use ! if you want to inverse that conversion. Does that help?

(In reply to :Gijs (he/him) from comment #21)

I'm not sure I understand what you mean by "conversion to boolean" ? Any if condition implicitly converts to boolean (where necessary). You'd then use ! if you want to inverse that conversion.

Yes, that is what I mean by "conversion to boolean". i.e. I'm including the implicit ToBoolean() of both/either of an if statement and/or a logical NOT operator.

The patch here also modifies expressions outside of if conditions, preferring for example !!rows.length over rows.length > 0.
(I don't know whether that was your original intention or not.)

Does that help?

I'm asking what is the motivation for a requirement that equality tests for Number values against 0 must use implicit ToBoolean() rather than explicit comparison operations? i.e. ToBoolean(x) over x == 0, etc.

I can follow that x == 0 could produce unexpected results if x might be undefined, but here we are talking about the situation where there is no "chance for confusion" about the type.

Is there a safety or readability reason to prefer the implicit ToBoolean(), or is there another reason?

(In reply to Karl Tomlinson (:karlt) from comment #22)

Yes, that is what I mean by "conversion to boolean". i.e. I'm including the implicit ToBoolean() of both/either of an if statement and/or a logical NOT operator.

The patch here also modifies expressions outside of if conditions, preferring for example !!rows.length over rows.length > 0.

I'm confused - the result of both of these expressions is a boolean, so I don't think this has anything to do with boolean conversions.

(I don't know whether that was your original intention or not.)

I think it probably helps to be consistent, though depending on the context in the example you cite, you might want to not convert to boolean at all - but the linter can't know that, so to ensure it doesn't break things, it should probably use !!foo rather than just foo for cases where no other boolean conversion takes place.

Does that help?

I'm asking what is the motivation for a requirement that equality tests for Number values against 0 must use implicit ToBoolean() rather than explicit comparison operations? i.e. ToBoolean(x) over x == 0, etc.

I can follow that x == 0 could produce unexpected results if x might be undefined, but here we are talking about the situation where there is no "chance for confusion" about the type.

If there were a way to enforce more of these truthy/falsy checks through a linter, I would suggest we should, but unfortunately that isn't as easy.
In the .length case, the number of cases where a linter is 'right' that you can substitute truthy/falsy checks so vastly outnumbers possible exceptions to such a rule (where, as suggested in other comments, .length is some kind of custom property that could be negative) that a linter won't get in people's way or need loads of exceptions, whereas if you tried to lint for any loose comparison against null or undefined or "" or 0 then I'd expect more issues to come up. We might still investigate that in the future...

Is there a safety or readability reason to prefer the implicit ToBoolean(), or is there another reason?

I think half of the issue is with prevailing style and trying to be consistent. However, although readability is often subjective, I do also firmly believe that the numeric comparisons are worse for readability when we're only trying to check if the array/string is or is not empty. if (foo.length) is more readable than if (foo.length > 0). It more intuitively captures "does foo have any elements". I don't have to parse whether we're checking if foo.length is bigger than 42, or exactly 5, or some other more specific condition. It's a truthy/falsy check in the same way that you would check for if (foo) or if (!foo) in C++ for cases where a pointer could be nullptr, or using if not list in python, or if (list.isEmpty()) in java, or ...
It's also shorter[1] in a way that is a strict subset of the previous condition, ie the " > 0" can be removed from the comparison and the result is the same (yes, this is not quite true for the !foo.length case, but summing the operators and operands, there's still fewer tokens in the expression to think about).

[1] I'm aware that shorter is not always better where code is concerned, but in this case, I think it is.

Drive-by comment: is there a reason for landing changes like this as one big patch instead of breaking it up per directory (devtools, js/src, toolkit etc) and getting separate reviews for each part? That seems strictly better when it comes to bisecting in case of regressions, ease of review, etc.

(In reply to Jan de Mooij [:jandem] from comment #24)

Drive-by comment: is there a reason for landing changes like this as one big patch instead of breaking it up per directory (devtools, js/src, toolkit etc) and getting separate reviews for each part? That seems strictly better when it comes to bisecting in case of regressions, ease of review, etc.

With the latest changes to the rule, I'm fairly confident that this is less likely to cause regressions. Personally, I don't think this needs separate reviews for each part as this is an automated replacement which also has its own in-tree tests. A lot of the changes are also to test files, and from the issues I've debugged so far in the earlier versions it was generally fairly obvious what had caused them.

I'm happy to discuss further.

Comment on attachment 9068390 [details]
Bug 1552055 Add linting if(foo.length) instead of foo.length>0

Revision D33011 was moved to bug 1578190. Setting attachment 9068390 [details] to obsolete.

Turning this into a meta here since that's what it really is.

Monika, now that the other patches have been landed for a while, would you like to start rolling this out to more?

We could probably split this over a few bugs mainly based on the major directories, now that we also have a list in the top-level .eslintrc.js.

Hey, I am not able to do this right now. If you wan't you can assign this to someone else.

Hi Monika, no problem. Thank you for getting this most of the way.

I'll likely try and schedule the remaining parts in with some other bits I have planned.

This is now turned on for everywhere apart from layout. Layout we'll handle later in separate bugs.