linter icon indicating copy to clipboard operation
linter copied to clipboard

Add a lint rule for checking obvious assert mistakes

Open rrousselGit opened this issue 3 years ago • 1 comments

Describe the rule you'd like to see implemented

It is fairly common for classes/functions to include asserts such as:

void fn({int? a, int? b}) {
  assert((a == null) == (b == null));
}

Which allows:

fn();
fn(a: 42, b: 21);

but rejects:

fn(a: 42);
fn(b: 21);

The downside with doing this is, the user experience isn't perfect because it's a runtime error. I wonder if a lint rule could be implemented to showcase assertion failures statically.

Of course, we can't support every single assert failure. But simple use-cases probably could be detected

Examples

I think focusing on combinations of x == null / x != null expressions could be enough as an MVP

Using the previous function:

void fn({int? a, int? b}) {
  assert((a == null) == (b == null));
}

we could statically decompose the (a == null) == (b == null) expression to determine that the nullability of a should be the same as the nullability of b

From there, we should be able to implement:

// all of the following lines produce a lint
fn(a: 42);
fn(b: 42);

// the followings would pass instead
fn()
fn(a: 42, b: 42)

int? a, b;
fn(a: a, b: b); // we can't statically know if this is valid or not, so we don't lint anything

Other cases that probably could be supported would be <> operators

void fn(int a) {
  assert(a >= 0);
}

fn(0); // ok
fn(42); // ok
fn(-1); // KO

int a;
fn(a); // we don't know, so ignored

rrousselGit avatar Aug 04 '22 17:08 rrousselGit

I'm not sure that a lint rule is the appropriate way to support this, but it's definitely an interesting feature request.

I think what you're suggesting essentially amounts to

  • defining a subset of asserts that could be statically evaluated, and
  • evaluating them at every invocation site when the necessary information can be gleaned from the invocation site.

This would be prohibitively expensive as a lint because it would require re-analyzing the defining library for every function invocation in every analyzed file (in order to find the asserts that needed to be evaluated).

To make it efficient we'd probably need to store a representation of the asserts we could attempt to evaluate in the element model, just like we do for const constructors. At that point I'd suggest making is an analyzer/language setting.

The other option would be to make it part of an off-line whole-world analysis. @srawlins

I suspect that neither of these are things we'd be able to staff any time soon, but it's definitely an interesting idea.

bwilkerson avatar Aug 05 '22 19:08 bwilkerson