Cascaded null-aware field accesses participate in field promotion, but non-cascaded ones don't.

[stereotype441]

I've been investigating the behavior of field promotion inside null aware accesses, and I've found a little inconsistency:

All of these examples assume the following declaration:

class A {
  final int? _i;
  A(this._i);
}

An access to a?._i does not see a previous promotion of a._i:

main() {
  var a = A(0);
  a._i!;
  a._i.abs(); // OK: `_i` has been promoted.
  a?._i.abs(); // ERROR: `_i` could be `null`
}

But if that access is in a null-aware cascade, then it does see the previous promotion:

main() {
  var a = A(0);
  a._i!;
  a.._i.abs(); // OK: `_i` has been promoted.
  a?.._i.abs(); // Also OK
}

This seems inconsistent to me. Null-aware cascades ought to behave as much like null-aware accesses as possible.

Further inconsistencies arise if the new sound-flow-analysis language feature is enabled:

A non-null assertion applied to _i inside a null-aware access does not promote it (even in cases where it would be sound to do so, because the target of the null-aware access is non-nullable):

main() {
  var a = A(0);
  a?._i!;
  a._i.abs(); // ERROR: `_i` could be `null`
}

But a non-null assertion applied to _i inside a cascaded null-aware access does promote it (provided that the target of the null-aware access is non-nullable):

main() {
  var a = A(0);
  a?.._i!;
  a._i.abs(); // OK: `_i` has been promoted.
}

Again, this seems inconsistent to me. Null-aware cascades ought to behave as much like null-aware accesses as possible.

[stereotype441]

I'm considering changing flow analysis so that when sound-flow-analysis is enabled, promotions always carry into and out of null-aware accesses. So the following behaviors would change:

(1)

main() {
  var a = A(0);
  a._i!;
  a._i.abs(); // OK: `_i` has been promoted.
  a?._i.abs(); // Also OK (was previously an ERROR)
}

(2)

main() {
  var a = A(0);
  a?._i!;
  a._i.abs(); // OK: `_i` has been promoted (was previously an ERROR)
  a?._i.abs(); // Also OK (was previously an ERROR)
}

My rationale for proposing this is that it makes a?._i behave more uniformly like its equivalent desugared form (a == null ? null : a._i). For example:

main() {
  var a = A(0);
  a._i!;
  a._i.abs(); // OK: `_i` has been promoted.
  a == null ? null : a._i.abs(); // This has always been OK
}

Note, however, that this would still be an error:

main() {
  var maybeA = A(0) as A?;
  maybeA?._i!;
  maybeA?._i.abs(); // ERROR: `_i` could be `null`
}

The reason for this is because maybeA?._i! has two reachable code paths, one of which null-checks _i, and one of which doesn't. It's beyond the capabilities of flow analysis to see that the code path containing .abs() can only follow the code path that null-checks _i (because it requires seeing that the two null checks are equivalent, and flow analysis doesn't reason about the equivalence of null checks).

@dart-lang/language-team what do you think? Any objections?

[lrhn]

Sounds like we are only promoting a._i, but a?.._i got included because it's desugared, while a?._i is not.

Desugaring should not affect semantics. We should specify the behavior on the original syntax.

I'm OK with making both a?._i and a?.._i be promoted, and also with making both not promoted. It shouldn't really matter, the ? can be removed.

And a.._i should always work (be promoted).

Does a.._i! promote?

Maybe we should just make ?._i not promote. If only a test on a._i can introduce the promotion, it's fine that only a similar access can use the promotion.

Avoids having to explain why this doesn't promote:

  A? a = A();
  a._i!;
  if (test) a= null;
  a?._i.foo();

Logically it's safe. The value of a`` is either the same, or it's null, and we only access it _iof the value pantnull`, so it's the same as what was promoted.

But it's probably easier to say that what's promoted is the access to _i one the same object, not just a particular syntax.

But that also means

var A(_i: i) = a;
i.foo();

and

if (a case A(:var _i) when _i.foo()) ...

We also don't allow this, right?

  A? a = maybeA;
  if (a != null) a._i!;
  if (a != null) a._i.foo();

Not because we couldn't, but because we don't track conditional promotion.

Or do we?

  A? a = maybeA;
  var canUse = a != null && a._i != null; 
  if (canUse) a._i.foo();

If this does work, then why does doing a!=null twice on the same value not remember the consequences?
Because there is no variable to attach the information to? But that doesn't stop us from remembering the promotion of a._i, we do that on the expression. (But then, this issue exists because that's not trivial, we should be remembering it on the access, not the syntax.)
Or because we remember the consequences of tests, not what they lead to? But isn't that a little naive? (But more manageable.)
Maybe this example is just particularly sikker, and it would not be viable for even slightly more complex branches.

[eernstg]

As @lrhn noted, this is concerned with an occurrence of ?. that isn't needed, which makes the code with modified behavior less important. So it's basically about (1) maintaining soundness, of course, and (2) achieving a semantics for the language as a whole which is as simple and consistent as possible.

I think this makes the proposed updates look very good:

An access to a?._i does not see a previous promotion of a._i

This can reasonably be described as an inconsistency because the promotion is equally justified (hence sound), and the update will eliminate it. Similarly, I agree that a member access (including ?.) should not gratuitously behave in a different way than the corresponding cascade (?..). Finally, the similarities among a?._i.abs() and a == null ? null : a._i.abs() again support the wish to ensure that they have similar semantics.

(No need to discuss specification by syntactic sugar here: those two expressions shouldn't gratuitously have different behaviors in any case.)

I haven't noticed any reason why we would not want to introduce these changes.

[lrhn]

If we are going to go for consistency, then I'd like every access to the variable on the same value to be promoted, independently of the syntax used to achieve that (as long as it's recognizable as reading that variable of that value).

That should include object pattern getter access, like a case A(_i: var i) and a!._i, as well as the discussed just a._i, a?._i, a.._i, and a?.._i.

I also see no problem with supporting that. If we know that reading that value has a type, it doesn't matter how we read it.
(May want to consider how much aliasing we can handle in patterns, fx a case (A(_i: var x) && A(_i: _ as int) && A(_i: var y)) ... , where we promoted _i in the middle pattern and read it in the last one. It probably "just works".)

[stereotype441]

@lrhn

Does a.._i! promote?

Yes, a.._i! has always promoted (well, as long as we've supported field promotion that is).

But that also means

var A(_i: i) = a;
i.foo();

Hmm, I would have expected this to work whenever a._i.foo() works. But testing it out, it appears that it doesn't. That's probably a bug. I'll investigate.

and

if (a case A(:var _i) when _i.foo()) ...

This works today (whenever a._i.foo() works).

We also don't allow this, right?

  A? a = maybeA;
  if (a != null) a._i!;
  if (a != null) a._i.foo();

Not because we couldn't, but because we don't track conditional promotion.

Correct, this doesn't work because flow analysis doesn't track that the two a != nulls are the same condition, and pick up where it left off.

Or do we?

  A? a = maybeA;
  var canUse = a != null && a._i != null; 
  if (canUse) a._i.foo();

Yes, this does work today.

If this does work, then why does doing a!=null twice on the same value not remember the consequences? Because there is no variable to attach the information to? But that doesn't stop us from remembering the promotion of a._i, we do that on the expression. (But then, this issue exists because that's not trivial, we should be remembering it on the access, not the syntax.) Or because we remember the consequences of tests, not what they lead to? But isn't that a little naive? (But more manageable.) Maybe this example is just particularly sikker, and it would not be viable for even slightly more complex branches.

Yeah, I think you've got that about right. The way I think of it is: putting a flow model "to sleep" and then "resuming" it later is hard, because we have to be able to (a) figure out that we might need it later, and store it away, (b) retrieve it when we need it, and (c) update it to reflect other state changes that might have happened while it was asleep. I figured out how to do (c) after a lot of hair pulling, but (a) and (b) are still tricky, and I've only managed to get the process to work in the specific case where a condition gets written to a variable and then later read (because the write and the read act as signals for (a) and (b)).

If we are going to go for consistency, then I'd like every access to the variable on the same value to be promoted, independently of the syntax used to achieve that (as long as it's recognizable as reading that variable of that value).

Agreed, this is a good principle.

That should include object pattern getter access, like a case A(_i: var i) and a!._i, as well as the discussed just a._i, a?._i, a.._i, and a?.._i.

Agreed!

I also see no problem with supporting that. If we know that reading that value has a type, it doesn't matter how we read it. (May want to consider how much aliasing we can handle in patterns, fx a case (A(_i: var x) && A(_i: _ as int) && A(_i: var y)) ... , where we promoted _i in the middle pattern and read it in the last one. It probably "just works".)

This is a good test case for me to experiment with when I'm making the fix. Thanks!

[stereotype441]

But that also means
var A(_i: i) = a;
i.foo();

Hmm, I would have expected this to work whenever a._i.foo() works. But testing it out, it appears that it doesn't. That's probably a bug. I'll investigate.

Ok, I've investigated this and I do believe it's a bug. I've filed a separate issue to track it: #4347.

[stereotype441]

Since @lrhn and @eernstg seem on board with making these improvements, I'm going to go ahead and work on them. I'll aim to release them as part of the sound-flow-analysis feature.