lint prefer_is_empty: List.isEmpty is not a const expression

[hugovdm]

Dart VM version: 2.1.0-dev.7.1.flutter-1f4dfce179 (Fri Oct 12 10:39:28 2018 +0000) on "linux_x64"

Lint suggests "prefer_is_empty". isEmpty doesn't work as a const expression, whereas List.length == 0 does.

I think isEmpty and isNotEmpty should be made to work for const expressions too, if possible?

[bwilkerson]

@leafpetersen @lrhn For the language question.

[lrhn]

Actually list.length == 0 does not work as a constant expression. The only type where .length works as a const expression is String.

We could extend to also allowing .isEmpty on string constants, but I don't think the return on investment is worth it. In general, you can assume that constant.something is not allowed (with string.length as the only exception).

[bwilkerson]

@hugovdm Can you provide an example of where the rule is producing a lint where it shouldn't?

[hugovdm]

Here is a List length assert that seems to pass without problems:
flutter/engine@master...hugovdm:lint_demo line 193, search for "variants.length" (anchor #diff-d96e98b12513726b7f36b22e1c6b6442R193)

My unit tests include constructing a const Locale with an empty list:
flutter/engine@master...hugovdm:lint_demo line 35 of locale_test.dart, "variants: []" (anchor #diff-9715ac6504248ba406cd5dd44091bd36R35)

Running ci/build.sh, I get this lint error:
...

• flutter/ci/analyze.sh
  Analyzing dart:ui library...
  lint • Use isEmpty instead of length at out/host_debug_unopt/gen/sky/bindings/dart_ui/window.dart:193:35 • prefer_is_empty
  Failed.

$ out/host_debug_unopt/dart --version
Dart VM version: 2.1.0-dev.7.1.flutter-d2c5a24fd9 (Tue Oct 2 19:20:35 2018 +0000) on "linux_x64"

[hugovdm]

And by comparison, if I change it to isEmpty as per the lint suggestion, this is the result:

output: org-dartlang-sdk:///flutter/lib/ui/window.dart:193:35: Error: Not a constant expression.
       assert(variants == null || variants.isEmpty || variants[0].length >= 4), // ignore: const_eval_type_bool_num_string, https://dart-review.googlesource.com/c/sdk/+/71340. Also demonstrates https://github.com/dart-lang/sdk/issues/34798
                                  ^^^^^^^^

[bwilkerson]

Here is a List length assert that seems to pass without problems:
flutter/engine@master...hugovdm:lint_demodiff-d96e98b12513726b7f36b22e1c6b6442R193

Thanks! Unfortunately, those links are broken so I can't see the examples.

[hugovdm]

The # got swallowed. I've edited the original, and I'm trying proper Markdown this time:
variants.length,
unit test creating an empty list.

[bwilkerson]

Thanks!

That code definitely looks invalid to me, and analyzer seems to confirm that by producing an error.

The lint rule should probably be updated to not generate a lint in this case because an error has already been produced, but the error is still there and still valid.

[hugovdm]

I'm not sure what you mean by invalid. The code in my branch compiles and runs and unit tests pass? (Only the "variants.isEmpty" version I mentioned yesterday in in this thread fails.)

[bwilkerson]

My understanding is that the condition in the assert is required to be a potentially constant expression because the assert is in a constructor with the const modifier. (@eernstg I can't actually find the place in the spec where it says that.) As far as I can see, variants[0] is not a potentially constant expression.

But, after re-reading the spec's discussion of constants, it looks like you're right about length:

An expression of the form e.length is potentially constant if e is a potentially constant expression ...

and that the rule needs to not create a lint if length is used in a potentially constant context.

[lrhn]

It's true that x.length is potentially constant, but it's also guaranteed to be an error if x is not a string when the constructor is invoked with const.

So:

class C {
  final v;
  const C(dynamic x) : v = x.length;
}

is a valid class. The invocation const C("1") is valid. The invocation const C([]) is a compile-time error.

If the type of x precludes it being a String, maybe we should make that an error as well. We don't do that now, to make it easier for compilers to recognize potentially constant expressions.
I guess we could add the checks on top of the current syntactic checks. Maybe something like:

If e.length occurs in a const constructor initializer list, then it is a compile-time error if String is not assignable to the static type of e.

(That would effectively preclude anything but dynamic, String and T extends String as types, because those are the only ones assignable to String and allowing a .length access).

[yjbanov]

This issue affects flutter/sentry (getsentry/sentry-dart#52 (comment))