Lint Rule: No boolean literal for arguments.

[l-cornelius-dol]

It would be great to have a lint rule to flag when true or false is passed to a function [edit] and that argument is not the only argument.

As a solution the problem of a vague true/false value passed to a function, the "boolean trap", I have a habit of creating a self-documenting constant and using that instead. Instead of redraw(true,true), I'll define const ALL_VIEWS = true, INLINE = true; and then use redraw(ALL_VIEWS,INLINE);. [edit: added another argument].

I'd like to be able to flag when I or a member of our team forgets to do this.

Thanks for considering this.

[bartlomieju]

I'm open to adding such rule, but it would be an optional rule (ie. required to enable it manually via a config file). PRs are welcome!

[CRBl69]

If nobody is already working on this PR, I'd like to give it a try !

[bartlomieju]

@CRBl69 please do!

[jmj0502]

@bartlomieju Is this issue still open? If so, I would like to give it a try (I may need some context of the code that should be affected in order to introduce the functionality tho')

[l-cornelius-dol]

@jmj0502 : The context is that you are consuming an API function that defines a boolean argument and you want to make the call-point self documenting by requiring that the call be made with a named constant instead of a literal true/false. This improves the code by making the argument in the call obvious (see the example I gave in my OP).

It should not flag a single boolean literal; APIs like enabled(true); and enabled(false) should be deemed acceptable, as they are common and usually self explanatory.

This applies to all code consuming an external API. Note that it does not address anything about whether boolean arguments should be allowed in a function to begin with, which is an separate, though related, matter. It should just flag when a true/false is passed to a function and it is not the sole argument.

[jmj0502]

@lawrence-dol Cool! I'll give the implementation a stab and reach out in case I happen to have any questions!

[jmj0502]

@lawrence-dol Question. Do you want the rule to take effect in the following cases as well?

invoke(true, remoteServerURL, true);
write(path, true, true);

Basically, in cases where there is a mix between named arguments and boolean literals or only in cases that match the example you previously provided?

[l-cornelius-dol]

@jmj0502 : Definitely yes. All cases where a boolean literal is provided with more than one argument of any type.

[jmj0502]

@lawrence-dol Perfect! Many thanks for the clarification. I'm almost done so the PR will probably be submitted soon!

[jmj0502]

@lawrence-dol @bartlomieju The PR is already pending for review! Look forward to your comments!

[l-cornelius-dol]

I am not a Rust programmer, but if I understand the code correctly, it's only flagging if there are at least two boolean args. But it should flag if there are any boolean args as long as there are two or more args total.

Given the initial check for 2+ arguments, I believe this:

for arg in args {
  if prev_argument_is_bool.clone() && is_boolean(arg.text()) {
    ctx.add_diagnostic_with_hint(
     call_expression.range(),
      CODE,
      MESSAGE,
      HINT,
    );
  }
  if is_boolean(arg.text()) {
    prev_argument_is_bool = true;
  }
}

just needs to be:

for arg in args {
  if is_boolean(arg.text()) {
    ctx.add_diagnostic_with_hint(
      call_expression.range(),
      CODE,
      MESSAGE,
      HINT,
    );
  }
}

[l-cornelius-dol]

Also, after thinking about it further, I do believe we should remove the 2+ args condition and flag any boolean literals in function calls. Even for the single arg case, say, button.enabled(true), it's arguably more readable to define YES=true,NO=false, and use button.enabled(YES), and then that also addresses the problem of "surprising booleans"*.

So, would you mind, terribly (assuming you agree with my thinking), removing the 2+ arg check as well?

(*) My initial example of redraw(true) is an example of a surprising boolean, where the argument is actually not true=redraw, false=don't redraw, but rather true=inline, false=deferred.

[jmj0502]

@lawrence-dol What you mention actually makes sense and I have no problem implementing it. When it comes to not allowing any boolean literals, I do agree because any floating literal seems like a magic value to me (IMO). I can proceed to implement the changes in a few minutes, but it I think is important to narrow down the expected behavior.

To recap, the rule should point out any boolean literals passed as arguments to a function, even if it is a single one.

So the following examples:

execute(true);
redraw(true, true);
executeCommand(true, EXECUTION_MODE.MODE_ONE);

Will all be marked as improvement points by the linter. Right?

Also, do you think the provided hint still works with these particular changes? Or do you recommend using other hint?

Look forward to your answer c:

[l-cornelius-dol]

Yes, all three examples would be flagged for improvement. And yes, the hint seems fine to me, though I would personally say "arguments" instead of "parameters". Thanks!

[jmj0502]

@lawrence-dol Cool! I'll submit my changes in a few minutes c:

[jmj0502]

@lawrence-dol Done! The PR is now updated with the changes. Look forward to any comments!

[jmj0502]

@bartlomieju @lawrence-dol there was some issue with the format of the code when checked by the actions. I already fixed and uploaded the changes. 👍

[jmj0502]

@bartlomieju @lawrence-dol A fix was applied in order to comply with a clippy linting recommendation. Now the PR is truly ready!

[jmj0502]

@lawrence-dol @bartlomieju The PR already landed so this issue can be closed! c:

[l-cornelius-dol]

Thank you so much!!