`prefer_is_empty` does not evaluate constants

[pq]

Note the last test cases:

const int zero = 0;

bool le5 = [].length < zero; // <= Should lint
bool le6 = zero < [].length; // <= Should lint

Note that since this is currently a false negative, we'll want to be mindful of ecosystem impact when addressing.

[oprypin]

Consider also:

class Foo {
  static const MIN_INPUT_LENGTH = 1;
  static const MAX_INPUT_LENGTH = 10;

  void bar(String query) {
    if (query.length >= MIN_INPUT_LENGTH && // LINT
        query.length <= MAX_INPUT_LENGTH) {
      print('ok');
    }
  }
}

I think this change would be adding a new false positive, rather than fixing a false negative.

And rather than being a friendly equivalent suggestion, this would be policing your use of constants. Which, if that's wanted, should be a separate lint.

[srawlins]

WDYT, @pq?

[pq]

Sorry for the slow response here @oprypin!

I'm not sure I see

    if (query.length >= MIN_INPUT_LENGTH && // LINT
        query.length <= MAX_INPUT_LENGTH) {
      print('ok');
    }
  }

as a false positive anymore than

    if (query.length >= 1 && // LINT
        query.length <= 10) {
      print('ok');
    }
  }

(but maybe they both are?)

As for the general point about whether we should be evaluating constants, I'm on the fence and would be super happy to hear from folks who have strong opinions either way.

Thanks!

/cc @bwilkerson @asashour

[bwilkerson]

We should evaluate a constant when the value won't change, but we shouldn't evaluate a constant when the value will change, such as when it's a flag that's manually set for debugging. The problem, or course, is that we don't have any way of knowing whether the constant's value will change over time.

We have a similar question for code like

const bool debugging = false;

void f() {
  if (debugging) {
    // Is this dead code?
  }
}

we generally err on the side of reducing false positives, which usually means that we don't evaluate constants.

As for

if (query.length >= 1 &&
    query.length <= 10) {
  print('ok');
}

I think that code looks much better than

if (query.isNotEmpty &&
    query.length <= 10) {
  print('ok');
}

and hence shouldn't be flagged.

[albertms10]

As for

if (query.length >= 1 &&
    query.length <= 10) {
  print('ok');
}

Now that patterns are around, we could write an even more elegant solution that would not trigger the lint:

if (query.length case >= 1 && <= 10) {
  print('ok');
}