New linter for complex conditional expressions

[IndrajeetPatil]

closes #1830

Draft PR for early feedback.

---

Examples

library(lintr)

lint(
  text = "if (a && b) NULL",
  linters = complex_conditional_linter()
)
#> ℹ No lints found.

lint(
  text = "if (a && b || c) NULL",
  linters = complex_conditional_linter()
)
#> ℹ No lints found.

lint(
  text = "if (a && b || c) NULL",
  linters = complex_conditional_linter(1L)
)
#> <text>:1:5: warning: [complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
#> if (a && b || c) NULL
#>     ^~~~~~~~~~~

^{Created on 2024-11-01 with reprex v2.1.1}

[MichaelChirico]

I don't think a && b || c is complex at all TBH, besides the mixing of precedences (but presumably (a && b) || c would also throw a lint here?), which I think is a separate issue.

I'm more comfortable calling nontrivial_expr(a) && nontrivial_expr(b) || nontrivial_expr(c) complex, but just plain names don't seem to add complexity IMO.

Defining the metric for "complexity" here is going to be quite hard -- are there other languages that have implemented something similar, or is there some CS literature we can turn to for some more generic definitions?

[IndrajeetPatil]

Defining the metric for "complexity" here is going to be quite hard -- are there other languages that have implemented something similar, or is there some CS literature we can turn to for some more generic definitions?

You are correct that this is a difficult question, but I don't think we need to have a consensus definition of what counts as "complex" conditional because that's an inherently subjective notion. The only thing we need to figure out is a good default (the same way we did for cyclocomp linter). And, since this is a configurable linter, even if we adopt a threshold that users feel is too restrictive, they can easily change this in config.

I think the current default of threshold = 1L is too aggressive, but we can easily change it to threshold = 2L (or higher):

lint(
  text = "if (a && b || c) NULL",
  linters = complex_conditional_linter(2)
)
#> ℹ No lints found.

So the question for us to resolve is what should be the default threshold?
How did we decide on 15L as the default threshold for McCabe complexity linter?

---

I would vote for threshold = 2L; any conditional expression with more than 2 operators (and 3 logical operands) can benefit from simplification, IMO.

Here is an example from our codebase:

if (inherits(e, "lint") && (is.na(e$line) || !nzchar(e$line) || e$message == "unexpected end of input")) {
  ...
}

which lints with this new linter and can be modified something along these lines:

is_expression_valid <- (is.na(e$line) || !nzchar(e$line) || e$message == "unexpected end of input")
if (inherits(e, "lint") && is_expression_valid) {
  ...
}

---

P.S. As for linters in other programming languages, I can't think of any (neither ruff nor eslint have anything similar; the closest ones are about complex conditional in type stubs or mixing operators).

[IndrajeetPatil]

I don't understand why the repo-meta-tests workflow is failing. There are already metadata tests for the new linter.

[AshesITR]

What happens when you run .dev/lint_metadata_tests.R locally?

From my understanding the script replaces all line numbers by 2^31 - 1 during Lint() construction and assumes the test file for each linter the_linter is named test-the_linter.R.

--- edit: Ah, the file name is the problem. Rename the test file and it should work.

[IndrajeetPatil]

@AshesITR Thanks!! That was it. Changing the filename fixed the issue.