special case assert initializers and const contexts in prefer_is_empty

[pq]

Fixes dart-lang/sdk#57805

/cc @bwilkerson

[bwilkerson]

This isn't quite right. I think we need to ignore anything inside any ConstructorInitializer, but only when the constructor has been marked as const.

On the other hand, it's possible that we don't need to test for these cases at all, because inConstantContext might return true in the initializer list of const constructors (and given that one of the test cases is for a non-assert initializer and it passes, I suspect that's true).

[pq]

I was hoping to get rid of this check too but inConstantContext is not returning true in the initializer test case 🤔. Checking for assert initializers and testing inConstantContext felt like maybe a sweet spot. (Though kludgey.)

Maybe we can discuss offline?

[bwilkerson]

Does inConstantContext return true in the non-assert initializer cases:

class C {
  final bool empty;
  const C(dynamic l) : empty = l.length == 0; // here
  const C.a(this.empty);
  const C.b(dynamic l) : this.a(l.length == 0); // here
}
class E {
  const E(bool b);
}
class F extends E {
  const F(dynamic l) : super(l.length == 0); // here
}

If so, then it sounds like a bug that it doesn't within assert initializers.

[pq]

It does. I've added some tests too.

@scheglov: thoughts on why this is not working in the assert initializer case?

[scheglov]

I don't see anything about constructor initializers in the spec.

[pq]

Ah, super. Thank you!

[bwilkerson]

That begs the question of why it's returning true in other constructor initializers. According to the spec it seems like it shouldn't be. I think that the inconsistency is pointing out a bug, but I'm not sure what the bug is.

But if inConstantContext should not be returning true in these cases, then we just need to not generate the lint in a constructor initializer where the constructor is marked as const.

[scheglov]

There is a difference between a constant context, and "constantness requirement". Any expression in constant constructors must be a constant expression, but does not introduce a constant context itself. But once you said const C() : x = const MyClass(...), everything in ... is in the constant context.

It does not look to me that inConstantContext returns true for constructor initializers.

  solo_test_inConstantContext_XXX() {
    parse('''
class C {
  final bool empty;
  const C(dynamic l) : empty = l.length == 0; // 0
  const C.a(this.empty);
  const C.b(dynamic l) : this.a(l.length == 0); // 1
}
class E {
  const E(bool b);
}
class F extends E {
  const F(dynamic l) : super(l.length == 0); // 2
}''');
    assertInContext("length == 0; // 0", false);
    assertInContext("length == 0); // 1", false);
    assertInContext("length == 0); // 2", false);
  }

[bwilkerson]

It does not look to me that inConstantContext returns true for constructor initializers.

Thanks for checking!

That's good because it means there's no inconsistency and hence no bug, but it leaves me not understanding why the tests passed when the code only checked inConstantContext.