feature: add no-boolean-literal-args lint rule

[CRBl69]

This PR contains the lint rule asked for in issue #975. It will not allow you to use boolean literals (true and false) when you make a function call.

It is an opt-in (as requested) lint rule.

Previously, this would be allowed :

foo(a: boolean) {
  console.log(a);
}

foo(true);

But with this lint rule, you have to do the following or else you will get a warning :

foo(a: boolean) {
  console.log(a);
}

let bar = true;

foo(bar);

[CLAassistant]

All committers have signed the CLA.

[l-cornelius-dol]

How can I track when this lands in deno lint?

[MierenManz]

There should be a speaker icon or button on this page that says subscribe. Then you'll get notifications about this thread in github. Ps: if you don't see a subscribe button but only a "unsubscribe" then you are already subscribed and you don't have to do anything

[l-cornelius-dol]

@MierenManz : That let's me get notifications about this issue. But I want to know when this lands in deno, so I can begin using it. I've not found any way on github to cleanly track a specific commit being merged into a specific branch.

[bartlomieju]

I agree with @dsherret, this rule should only consider declarations, not invocations of functions.

[nayeemrmn]

I agree this is a superfluous and overly specific rule (is this a real problem with code and why not other kinds of literals?), but applying it to declarations doesn't make sense either. Hitting it when passing args to third party functions aligns with the intent fwiw. It's supposed to make you name the argument on the calling side.

[dsherret]

Hitting it when passing args to third party functions aligns with the intent fwiw. It's supposed to make you name the argument on the calling side.

Oh, I didn't realize that.

[l-cornelius-dol]

This rule seems quite annoying for when calling external code outside the user's control.

The rule is intended to require that the calling code name boolean arguments and not simply pass true/false. E.g. someFunction(true,true,false,true);. Because boolean arguments are a particularly egregious maintenance problem.

I agree this is a superfluous and overly specific rule

Then don't enable it in your ruleset.

(is this a real problem with code...

Yes it's a real and well-known problem with code maintenance, and this simple rule can greatly improve the use of APIs that utilize boolean parameters (which is not always a bad API in and of itself).

... and why not other kinds of literals?)

Because boolean arguments are unique in that they always benefit from naming; other literals may or may not, depending on the context and API, and it's impossible to apply a blanket rule. For example, let cur = add(cur,10); does not benefit from let cur = add(cur,TEN);, but repaint(data,true) always benefits from repaint(data,IMMED) and repaint(data,DEFERRED).

but applying it to declarations doesn't make sense either. Hitting it when passing args to third party functions aligns with the intent fwiw. It's supposed to make you name the argument on the calling side.

That's 100% correct.

[nayeemrmn]

or example, let cur = add(cur,10); does not benefit from let cur = add(cur,TEN);, but repaint(data,true) always benefits from repaint(data,IMMED) and repaint(data,DEFERRED).

That's just a wrong analogy :P you wouldn't name it TEN, you would name it whatever the number was supposed to mean. It's the exact same situation.

I retract slightly, unnamed positional args in general can be a real problem. rust_analyzer shows their names inline like this: I think if this were a widely recognised problem in JS code maintenance, editors should do this for JS. I don't like the suggested fix associated with this lint rule. I disagree that passing literals as positional args should be considered a problem even under an optional rule.

[l-cornelius-dol]

That's just a wrong analogy :P you wouldn't name it TEN, you would name it whatever the number was supposed to mean. It's the exact same situation.

It's not an analogy, it's an example, specifically one in which a literal integer value would be used because there's no useful abstract name for the value.

I retract slightly, unnamed positional args in general can be a real problem.

Yes they can. But booleans are unique in that, again, they always benefit from being named; as the above example shows, that's not true of other literals. Sure it might be true of an integer being used as an enumeration, but one can't practically say all integers shall have names.

But I don't get the push-back. It's an optional rule that will have real, practical benefit for our team. If you don't like it, don't use it.

[CRBl69]

Perhaps this rule would be more useful if it were no-boolean-literal-params where it checks function, constructor, and method declarations for use of a boolean parameter instead of the call expressions?

I agree with @dsherret, this rule should only consider declarations, not invocations of functions.

I'm not sure I understand this correctly, you want the rule to forbid the following ?

constructor(isVisible: boolean /* forbid this boolean */) {
  // ...
}

[CRBl69]

I understood @lawrence-dol opinion (which I personally agree with), I want to understand others' too

[Cre3per]

I'm neither a deno user nor a js/ts expert. I find this discussion interesting nonetheless, because it applies to all languages without keyword arguments.

IDE inlays for positional arguments, as shown by @nayeemrmn, solve the issue this lint rule is addressing, if the reader has an IDE and it the IDE understands the code.
If a developer is working with broken code, and the IDE fails to find a function declaration matching a call, the developer will be at a loss. It's that time when the developer would most benefit from readable code.
Personally I think inlays for arguments have great potential for worsening code readability.

There are valid uses of unnamed boolean arguments, and it's too hard for a linter to tell them apart from invalid uses

// valid
window.set_bordered(true);
// invalid
window.repaint(true);

More often than not, an unnamed boolean argument makes code harder to understand. Uses should be flagged by a linter, but without causing false positives.

foxglove, an eslint plugin, has this same rule, but with an important option:

allowLoneParameter: true will not report an error if a boolean parameter is the only parameter to a function

I've never used this rule, I suppose it acts like this

window.set_bordered(true); // valid
window.repaint(true); // valid
window.set_border(color::red, true); // invalid
window.repaint(true, true); // invalid

I can't think of an example where a function with multiple parameters, one of them being a bool, makes sense. This is easy to detect for a linter.

Here's a good read about boolean traps

[CRBl69]

Hi, I've read the blog from @Cre3per's comment and I must say that IMO, it justifies this rule as an opt-in rule. Although as @dsherret said, it should also warn on function declaration (and constructors), and suggest using an object instead of a boolean.

This :

class Widget {
  function repaint(immediate: boolean) {
    // ...
  }
}

Should be replaced by this :

class Widget {
  function repaint({immediate: boolean}) {
    // ...
  }
}

So in the end, the rule will warn users if they use boolean literals in function and constructor calls, and warn the use of booleans in function and constructor declaration.

What do you think ? Should I start making these modifications ?

[l-cornelius-dol]

@CRB169

What do you think ? Should I start making these modifications ?

No, I don't think you should.

Whether to use an object or a boolean argument is a design choice and there a good arguments for both. The requested linting rule is to encourage labeling boolean arguments at the call point so that the calling code is self-documenting. It has nothing to do with trying to force the designer of the API to use or not use boolean arguments. It is, in effect, a warning upon finding the literals true or false in an argument list.

What you are now describing is an entirely separate rule which affects API design, whereas this one is intended for API use.

I do think @Cre3per's earlier suggestion to exempt lone boolean calls may have merit, but even there I think the code is better with a constant than simply true/false and the burden is not undue. If the caller desires they can always define const TRUE=true,FALSE=false if they really believe that true/false is the best description of the value (remembering that they have opted into this rule to start with), but I think the article referred to by @Cre3per thoroughly refutes this notion.

[l-cornelius-dol]

@CRBl69 : Incidentally, your idea could be implemented as no-boolean-args and address the same problem at the declaration point. Personally, I would likely leave this off, while turning no-boolean-literal-args on, but who knows? If JavaScript had a good syntax for defining and using enumeration types I'd likely opt to use them instead of boolean values.

[l-cornelius-dol]

To be clear, what I am saying is that the two rules call-point and declaration-point should be separate, as they address separate developers. The one I requested in this feature has been implemented as I've suggested. An additional rule could be also added to address the need at the declaration-point.