Shadowing a non-local variable

[eernstg]

Promotion of fields is probably the most prominent request we have in response to the release of null safety. We could support the coding style where a field is copied into a local variable in the following way:

class C {
  num n;
  C(this.n);

  void foo() {
    shadow n; // Desugars as `num n = this.n`.
    if (n is int) {
      // `n` is promoted here.
      n = 42; // Desugars to `this.n = n = 42;`.
    }
  }
}

This would help keeping the local variable and the field in sync (so it's less error prone than the "manual" version where we simply declare var n = this.n; and remember to write it back as needed). It is also rather explicit: the declaration of the local variable would be an error if there is no field with the same name, and the declaration immediately sends the signal to a reader of the code that this local variable is intended to be synchronized with the instance field with the same name.

The idea easily generalizes to class variables (static num n2 = 0; and shadow n2;) and top-level variables.

Assignments to the local variable will be a compile-time error in the case where the associated non-local variable does not have an implicitly induced setter.

Surely it will be complex to cover all cases, such that any updates to the local variable is guaranteed to be accompanied by an update to the field, but I'm sure we can sort out the details.

If it turns out to be too much of a breaking change to make shadow a built-in identifier (this means that no type can have the name shadow, but that would be really rare anyway because user-written type names in Dart are almost universally Capitalized), then we could consider a syntax like var this.n;.

[srawlins]

Would this also allow assigning to the field (without this.) before shadowing? For example, replacing shadow n with num n = this.n, and inserting before this line something like n = 7; produces a static error:

line 7 • Local variable 'n' can't be referenced before it is declared.

[eernstg]

I think it would be at least worth trying to keep the rules similar to the ones we have today. This means that the assignment to the field before the declaration of shadow n; would have to assign to this.n. Slightly inconvenient, but probably good for readability.

[srawlins]

Slightly inconvenient, but probably good for readability.

I think you're right. I like it.

[rrousselGit]

Would we be able to do the following?

class C {
  num n;
  C(this.n);

  void foo() {
    if (shadow n is int) {
      // `n` is promoted here.
    }
  }
}

[lrhn]

So, a local variable which is initialized to the value of a non-local variable, and where writes to the local variable are also written through to the non-local variable.

What if the variable we shadow is final (has no getter).We just make the local variable final too. Then it's very, very close to just being the same as if-variables (https://github.com/dart-lang/language/issues/1201) or binding expressions (#1210) that only work as a declaration (so strictly worse). I think this feature would be better suited to be combined with an in-line variable declaration (which can be used as a plain declaration as well).

What if the shadowed variable is late? (Can we know at all, or will it be entirely interface based)? Can we make the local variable late too, and not read the shadowed variable until we read the local? Or will we just eagerly read, and eagerly write back even if the shadowed variable is late-and-final? (I guess it's your own responsibility to only write it once, as usual).

Can you shadow local variables:

var x; { shadow x; x = 4; } print(x);

If not, why not?

We are aliasing an assignable variable, and allowing assignment to it. That's effectively a reference variable, an alias for a LHS. Can we allow that as a parameter, foo(int x, shadow int y) => y = x;? (If not, why not?)

I think this proposal is aiming too low. Fields (or even static/top-level variables) are not the only problem! It would be nice if it could also shadow variables that are not in the lexical scope at all, so you can maybe do shadow something.x; if (x != null) x += 2;. (Or shadow x = something.x; for when the RHS is not a single identifier.) If you can handle this.x, you should be able to handle any o.x where you know o won't change its value (usually because o is a local variable with no assignments). It would be nice to also handle o[v] as well.

So, what if you could shadow any assignable expression. Let's say shadow s = assignableExpression;.

void foo(List<int?> foos) {
   shadow s = foo[1];
   if (s != 0) s += 42;
}

where shadow s; is simply shorthand for shadow s = <what s would refer to without this declaration>;

I do worry that people won't understand the underlying model of a separate local variable, not just an alias for the instance variable, just thinking of it as a magic incantation to allow promoting instance variables, and then get bitten by concurrent modification.

Would it be worth adding checks for concurrent modification, so each read of the variable would check that the shadowed non-local variable has the expected value. Treat the shadow variable's scope as an implicit exclusion scope where no-one else are allowed to write to the variable (that would be a dynamic error, and hard to debug since the error doesn't happen at the modification, only at the next read).

I wonder if this could be optimized better if we allow the compiler to not write through every time, or can delay the write back to a later time. If it knows that the shadowed variable will be read again by the function, it can delay the write. If the variable will definitely not be read again, it must have written the value back. Maybe make it scope based, so the compiler only needs to write the value back when the variable goes out of scope, but may do so at any time. Maybe even allow the compiler to forget the variable's value and re-read it from the source. Many subtle errors can happen, but isn't that basically how C++ does non-volatile variables? (And then the syntax should probably be cache x;).

Maybe it should be cache x; in any case. The way I read shadow x;is that it hides thex`, not that it gives access to it.

[eernstg]

@rrousselGit wrote:

Would we be able to do the following?

class C {
  num n;
  C(this.n);

  void foo() {
    if (shadow n is int) {
      // `n` is promoted here.
    }
  }
}

I think that would be allowed as a consequence of adopting one of the proposals about declarations in expressions (#1201, #1210, #1420). If we can do if (var: this.n is int) ... then we can also enable something like if (shadow: n is int) ....

[eernstg]

@tatumizer wrote:

the word "shadow" is not normally used in the proposed meaning

True, usually a declared name 'shadows' another declared name when the latter is unavailable because the former is declared in a scope which is "nearer":

void main() {
  int i = 0;
  {
    int i = 1; // Shadows the `i` declared 2 lines up.
    i++; // Works on the `i` declared 1 line up.
  }
}

But this property also holds for the proposed mechanism: The local variable does actually shadow the field (which is the reason why we have to use terms like this.n in order to access the field). We do have more in this case: The automatic initialization of the local variable (shadow n; means var n = this.n;) and the automatic write-back upon assignments to the local variable (n = 14 means this.n = n = 14) are extras. But the extras don't contradict the basic notion of shadowing.

[eernstg]

@tatumizer wrote:

DART: (reluctantly, after a year of deliberations): OK, now you can.

I love it. :grin:

[eernstg]

But we could certainly allow developers to specify that they prefer this mechanism by default for a given instance member:

class C {
  shadow num n;
  C(this.n);

  void foo() {
    if (n is int) { // The reference to `this.n` implicitly induces `shadow n;`
      // `n` is promoted here.
      n = 42; // Desugars to `this.n = n = 42;`.
    }
  }
}

The implicitly induced shadow n; declaration would be added at the beginning of the outermost enclosing function body.

I'm not quite convinced that we want this, however. It is a subtle semantic change that we have shadowed a given non-local variable as a local variable, because the whole point is that updates to the non-local variable won't change the value that we're looking at, and hence we can promote and so on. So if it is a bug to ignore updates to the non-local variable then we've just made the program buggy by an implicit mechanism. I suspect that it is worthwhile to ask for a small syntactic payment for this semantics: Please do write shadow n;.

[eernstg]

@lrhn wrote:

What if the variable we shadow is final (has no getter).We just make the local variable final too

We could do that. We can also allow for "refreshing" the local variable, and still get an error on all other assignments to a shadowing variable:

class C {
  // Getter that almost always returns the same object, but is very costly.
  int get g => ...;

  void foo() {
    shadow g; // Snapshot the value of `g` here.
    ...
    // This particular assignment to `g` could be allowed.
    if (somePropertyIndicatesThatGHasChanged) g = this.g;
    ...
  }
}

this feature would be better suited to be combined with an in-line variable declaration

Yes, we should support this variant of a declaration inside expressions, just like we'd support normal local variable declarations inside expressions. It is not obvious to me that it would be better to shadow a variable inside an expression, it's just a relevant case, similar to the normal local variable declaration which is also likely to be used in both ways.

What if the shadowed variable is late?

As you mention, we'd probably just make the local variable late as well.

Can you shadow local variables:

We could allow that, with the same semantics (that is, the same desugaring), but I doubt that it would be very helpful. It would then be exclusively motivated by the ability of the shadowing variable to keep the value while the shadowed variable is mutated. Is that useful in the case where you also have the scope based shadowing, i.e., you can't even read or write the shadowed variable, unless it's captured by some local function? ;-)

That's effectively a reference variable

I'm not convinced about this: We do want the mechanism to be a caching mechanism rather than an indirection mechanism, otherwise it won't allow the promotion which is the main motivation.

shadow x = something.x;

That's an interesting idea! It does require the shadowed expression to denote a property (so something.x must be a getter, and they may or may not be a corresponding setter).

However, I'm not really happy about the idea that we will keep evaluating the rest of that expression implicitly upon each update:

var x = 1;
int f() {
  print(x++);
  return x;
}

void main() {
  shadow b = f().isEven;
  b = false; // Prints '2'!
}

just thinking of it as a magic incantation to allow promoting instance variables, and then get bitten by concurrent modification

This is indeed something to be careful about. However, the mechanism is itself a correctness aid in that it ensures that the variable is initialized from the shadowed variable, and it ensures that the value is written back each time the local variable is mutated (or we get an error at the local variable update because we can't update the shadowed variable), and it's a very real possibility that manual shadowing will give rise to bugs because that write-back was omitted by accident.

The whole point around concurrent modification is that we can only use shadow when we know that there will not be any concurrent modifications, or that it is correct to use the cached value anyway. But the fact that we're using a specific kind of declaration to get this shadowing semantics it is self-documenting: When you see shadow you know that we're relying on the absence or benignness of concurrent modifications.

Would it be worth adding checks for concurrent modification

That would be rather easy, but I suspect that it's a better trade-off to omit these checks (they can always be written manually (if (n != this.n) throw 'Concurrent modification, help!!!';, and I believe that performance is an important consideration for this mechanism).

[lrhn]

However, I'm not really happy about the idea that we will keep evaluating the rest of that expression implicitly upon each update.

We definitely wouldn't. If we do cache x = foo().bar().x; we would partially evaluate foo().bar() to a value o and remember o and x. The any write would write to o.x. We'd do that for any shape of assignable expression, whether it's e.x, C.x, Ext(o).x, super.x, e[e2], super[e2], Ext(o)[e2], prefix.x or just a local variable x, ... or anything I've forgotten. (I have a plan! This fits right into that plan!)

So:

var x = 1;
int f() {
  print(x++);
  return x;
}

void main() {
  shadow b = f().isEven;  // Prints `2` here!
  b = false; // Invalid since `b` shadows a property with no setter.
}

[eernstg]

Cool! Sounds like that could work.

[eernstg]

I actually think shadowing should be somewhat rare: Lots of variables are not nullable, and lots of code won't need promotion (it's one of the oldest tenets of OO that we shouldn't "type case", we should use virtual methods! ;-), and it is only in the situation where we have to step out of a clean OO coding style that we'd need shadowing. On top of that, it's only appropriate to use shadowing if we can actually rely on having no "concurrent modifications" of the shadowed variable. Isn't it unreasonable to assume that it would be used for anything near every instance variable in every method body?

[rrousselGit]

I'd like to say that, from my experience, this is not much of an issue

I personally use InheritedWidgets+freezed+flutter_hooks, which work around the issue by simply not having to shadow variables at all

This principle could be broadened to more areas. For example, static meta-programming could alleviate the issue as it tries to support use-cases like functional widgets

This means if (is) would work just fine:

@StatelessWidget
Widget MyWidget({Key? key, String? title}) {
  if (title != null) {
    // title is no-longer nullable here
  }
}

In the same fashion, destructuring would help, because it is a natural way for developers to declare a local variable. Especially so combined with pattern matching

class Data {
  final String? title;
}

class Example extends StatelessWidget {
  Loading | Failure | Data data;

  @override
  Widget build(context) {
    switch (data) {
      <handle error and loading states>
      case Data(title) {
        if (title != null) {
          // title is upcasted to non-nullable here
        }
      }
    }
  }
}

So maybe there are alternate ways to solve this issue.

[eernstg]

https://github.com/dart-lang/language/issues/1518 outlines an idea, stable getters, which would allow us to promote a getter invocation or even a whole path of getter invocations.

[Levi-Lesches]

Here's an observation:

class Temp {
  int? maybeNull = 3;

  void needsNonNull(int value) { }

  void test() {
    final int? maybeNull = this.maybeNull;  // the shadow
    if (maybeNull != null) {
      needsNonNull(maybeNull);
      maybeNull = 5;  // ERROR: maybeNull is final and cannot be set
    }
    print(this.maybeNull);
  }
}

Currently, Dart recognizes there's an error because the in-scope variable is final. How about we add one simple modification: IF that is the case, try to find a non-final in-scope variable of the same name. This should fix this problem without adding a new keyword, and in other cases, the error will still show as normal because there won't be another variable of the same name. Besides, I think it is farily intuitive to see that assigning a variable would mean looking for one that's non-final, even if it's not the local variable.

[esDotDev]

Couple thoughts to toss in:

1. tatumizer is totally right that UI code, especially the way flutter was designed, has a disproportionate number of nullable params. Since the entire framework is built around config objects, passed to instance methods, we will hit this a lot. Especially as we can't give non-constant defaults to our fields, meaning in many cases, we have no choice but to accept null, and then assign our default in a later method.

2. It's very hard to think of a use case where we would not want the expected behavior of 'auto-shadow' or auto-! . If I ever had a reason to cache a class field, I would already be doing it.

3. shadow * would be nice, but it still feels backwards. Why make the 99.99% use case declare something, shouldn't it go the other way? If you want the super-strict behavior, you declare it?

4. I'm still a bit confused why the compiler is seeking to be safer than all of our existing code. Lines like if(this.foo != null) use(this.foo) are not a real source of null errors in any production code I've ever seen. I know it comes down to being 100% sound... but this is a net loss in the end imo. Ideally null safety shouldn't come with a readability hit, maximizing it's upside.

5. This issue clearly will be less of a concern if you make your entire widget a single method with no class level state at all (aka flutter_hooks), but vanilla StatefulWidget is a different story, and you still have the widget configs to deal with.

[eernstg]

@Levi-Lesches mentioned the maybeNull example here. The following is a variant of that:

class Temp {
  int? maybeNull = 3;

  void needsNonNull(int value) {}

  void test() {
    shadow maybeNull;
    if (maybeNull != null) {
      needsNonNull(maybeNull);
      maybeNull = 5;  // OK, will set both the local variable and the instance variable.
    }
    print(this.maybeNull); // '5'.
  }
}

I understand that the motivation for the original example was to show that we can get a warning if we try to assign to the shadow by using a final variable (and no new language constructs). By the way, we could indeed have a final shadow maybeNull if we want the behavior where a getter evaluation is cached and a setter evaluation is an error.

But I still suspect that the above behavior would be more useful: We don't get a compile-time error, but we also don't get the bug. ;-)

[Levi-Lesches]

Yes, and I was also asking why we can't redirect those types of errors into automatic shadows (so no more error). That way, no one has to learn a new language feature (and you don't have to make one!), and the feature will always be used exactly where intended.

[eernstg]

why we can't redirect those types of errors into automatic shadows

Ah, sorry, I misread that.

But I don't think we can make shadowing implicit: It's a non-trivial assumption about the enclosing software that we can cache the value of any instance variable for any amount of time. The software might be designed for that, and it might even be OK to use the cached value in some situations where the underlying instance variable was mutated, but it might as well be a subtle, nasty bug to cache the instance variable at all.

Note that #1518 is aimed exactly at the situation where caching is a developer commitment: "I hereby declare that this particular getter can be cached". In that issue I already mentioned that auto-shadowing is completely appropriate.

[lrhn]

If we are willing to accept unsoundness, which @esDotDev's 4th dot suggest, then the question becomes one of syntax.

I do not want unsoundness without a syntactic marker. I don't want it as the default. (It's not a completely bonkers idea to have unchecked unsafe operations. We do it all the time with things like

if (list.isNotEmpty) something(list.last);

where list.last throws if the list became empty between the check and the use. Nobody complains about that unsafety, it's just part of the contract of List that you should check before using .last).

A marker to enable unsound access means something like https://github.com/dart-lang/language/issues/1187 (not https://github.com/dart-lang/language/issues/1188, which is implicit). The only question then is which syntax to use.

@leafpetersen suggested x is type!. It doesn't work with != null. Maybe x ?= null and x is? type could be used (it's somewhat symmetric, goes where the negating ! would otherwise go). It would allow any promotion that == and is would, but only if:

• The variable hasn't changed until it's used (auto check identity at use),
• or the variable still satisfies the same test when it's used (auto reapply the test at use),
• or we allow the compiler to cache the value until any visible write to the variable (or we auto-cache the original value, and don't read the expression again, if it's something that we allow optimistic promotion for).

In any case, it's basically similar to a Swift-style implicitly unwrapped variable, where you do an extra check when you read the variable, but you declare it at the variable declaration (or, here, at the variable promotion) so it becomes part of the variable that it needs to be checked. It's still visible in the program in at least one place.

(And I still think it's aiming too low because it only works for single identifiers resolved against the static or instance scope, which still doesn't allow you to use it on other expressions which used to work, like if (o.x is? int) o.x.toRadixString(16), and so we are still left without a general solution).

[eernstg]

@esDotDev wrote:

I'm still a bit confused why the compiler is seeking to be safer than all of our existing code

That's a very common development for Dart: We have been enhancing the support for various kinds of compile-time correctness checks for a long time.

It may be true that if (this.foo != null) use(this.foo); is almost always safe, but from a static analysis point of view that isn't enough: We need to detect situations that are guaranteed to be safe, and treat all other situations as unsafe.

We may then handle the potential failure automatically (by letting the compiler insert a null check or a type cast or whatever is needed), and perhaps also implicitly (by not emitting any diagnostic messages about this action at all).

But we have been moving in the direction of more compile-time safety for a long time, and I completely agree with @lrhn here:

I do not want unsoundness without a syntactic marker. I don't want it as the default.

A marker to enable unsound access means something like #1187

Let me plug this one again: https://github.com/dart-lang/language/issues/1187#issuecomment-684760034 :smile:

With a dynamic check at each call site, I don't see a strong need for enforcing that the promoted expression evaluates to the same value each time, it just needs to pass the type check.

The "e != null doesn't fit" problem is irritating, but e is! late Null will work. We could support e != late null, which has a bunch of built-in weird exceptions, but so does the treatment of x != null today. We could drop the special casing and just recommend x is! Null rather than x != null in the first place. Wouldn't be hard to auto-migrate.

[eernstg]

We all know that x is 42, so we don't need to check it at run time. ;-)

Other than that, I think it makes a big difference that x != null can be expressed as a property that the static analysis will actually keep track of (and we can express it in terms of types: x is! Null), but the Dart type system probably won't ever make any attempt to support proofs of x > 42.

If we wish to maintain any correctness properties associated with specific values then I think statically checked immutability (e.g., #1518) is one of the best ways to go: Anything which is verified at some point for a stable expression is known to hold for that same stable expression in the same execution of the relevant scope.

[lrhn]

@tatumizer It's not just about overriding final fields. It's also about replacing them with getters. Or implementing them, since any class has an implementable interface, and it's silly to avoid this.foo to be promoted, but not o.foo, especially since this can be an extension target or mixin application where you only know the interface of the target. Even if we provide promotion only for a final field, on this, in a non-extension/mixin method, but not a getter, it's no longer a non-breaking change to change a field into a getter. Being a field would be a strong promise to the future that you would always stay a final field. No making it mutable. No making it a getter.

We'll then likely see people doing:

  final int _field;
  int get field => _field;

in order to not make that promise. This kind of unnecessary wrapping was exactly what Dart getters and setters were introduced to avoid.

So, would you accept to completely remove getters and setters from Dart? Just have fields and methods, like Java? Or like C# where you have getters and setters separate from fields, with syntax to easily make getter/setter backed by an implicit field?

(I don't want that, not in the name of performance or to help with promoting some otherwise unpromotable expressions. I'd rather have a way to promote any expression, even if it means introducing a new local variable for it. Whether there is write-through would be a cherry on top of a generally useful functionality).

[Levi-Lesches]

I disagree that "if vars" and the like are too cumbersome -- they pretty much avoid the whole issue of getters in the first place and are easy for people to understand. Although I personally probably wouldn't use them, I can see how they would be very helpful, and seem to have lots of support (like #1201)

But I don't think we can make shadowing implicit

My example has the user declare a final variable, just like people would today to get promotion. The only implicit part is that when someone does myVar = value;, if myVar is final, then look for another variable with the same name that does have a setter. Personally, even before NNBD, I felt like this should be the default behavior to begin with.

[eernstg]

That could be a good trade-off!

It's rather similar to the likely form for a binding expression variant for shadows:

if (shadow: x != null) use(x);
if (shadow: foo.bar > 42) use (bar);

We have use(bar) rather than use(foo.bar) at the end because the binding expression shadow: foo.bar introduces a local variable, not a new interpretation of an existing composite syntactic term. The new variable is called bar because we haven't explicitly asked for anything else.

About latching: That term does indeed capture the caching effect, but not the write-back. The word 'shadow' doesn't imply write-back either, but it does hint that there is a certain connectedness—we just need to imagine that pushing the shadow a bit will also push the shadow-giver in the same direction. ;-)

[esDotDev]

What worries me in general is that the current guidance is to use ! as the a workaround for non-local null-checks being ignored by the Analyzer. This is fundamentally a poor idea, every time I add one of these I'm plopping a small grenade in the code base.

If this sugar is going to take longer than a few mths to materialize, I do think the Flutter team needs to make some videos about this issue, and just really recommend that local-caching is the way to go, and ! should virtually never be used, it's so much safer to just cache the var, and allows the compiler to keep the whole thing sound.

If you look at the guidance, it doesn't even discuss local caching as a strategy, it just says you should slap in a bunch of ! operators:

In other words, we human maintainers of the code know that error won’t be null at the point that we use it and we need a way to assert that ... Of course, like any cast, using ! comes with a loss of static safety. The cast must be checked at runtime to preserve soundness and it may fail and throw an exception. But you have control over where these casts are inserted, and you can always see them by looking through your code.

This is a pretty superficial description of the problem, and we're going to end up very soon with code bases full of ! operators. I think it should discuss how this is not the ideal approach, and for now you should maintain a local cache when possible.

https://dart.dev/null-safety/understanding-null-safety#null-assertion-operator

[Edit] Apologies, I'm sure this is off topic. Thought it's a worthwhile topic to put on the radar though.

[eernstg]

local-caching is the way to go, and ! should virtually never be used

That's the approach I've used in a substantial amount of null-safety migration, and it is not a bad solution. I'd just prefer to have better support for doing it, safely.

Alternatively, if we add lots and lots of ! until the compiler is happy then it's just a different syntax for exactly the same lack of safety that we've had until now. We might as well take the opportunity to migrate a lot of code to be better than it was, not just more verbose. ;-)

[eernstg]

@tatumizer wrote:

Yes it does capture the write-back!

You're right!

[eernstg]

Haha! Chasing moving comments is fun. :smile:

problem with latches is that it's a purely static, compile-time device

var self=this;
if (*x != null) {
  use(x); //OK, uses the latched value
  use(this.x); // what does it mean? Is it a latched value or not?
  use(self.x); // what does it mean? Is it a latched value or not?
  (){x=1;}(); // does x refer to a latched value??? 
}

If the starting point is that we wish to enable promotion of fields (or any kind of promotion) then we have to use a compile-time property, because promotion is concerned with the static type of an expression (until now: a variable), and that's inherently a compile-time concept.

We could have a set of rules about guaranteed equivalence (that is, an equational theory) for expressions. So we'd be able to know (at compile-time) that y can be the same thing as this.y (because the compiler actually transforms the former into the latter at some point), and we might also be able to recognize that self is the same object as this.

But I don't think it's particularly useful to go down that path, because there will be a few cases where we can prove the equivalence, and a lot of cases (surely many of them "obvious") where we can't prove it, so it won't actually work. Also, I think it's better for the readability of the code if we simply require that developers write the same expression each time they want to refer to the same object, especially if it is crucial that it is actually the same object.

But in the example above we can't actually know that x is the same thing as this.x, because (if I understood the meaning of * correctly) x is a local variable that caches the value of this.x, and the cache could be out of sync.

Alternatively, if we have a design where each evaluation of x is accompanied by an evaluation of this.x and a comparison, incurring a dynamic error if they differ, then we can actually trust x to be the same object as this.x, but I still think the code would be cleaner and more readable if we would simply stick to x and avoid going down the path of having many different ways to write an expression denoting that same object.

[Hixie]

FWIW, I feel the language should help the programmer, not the other way around. This feature feels like a workaround where the programmer is telling the compiler "don't worry your pretty little head about this, I swear it's ok".

[Levi-Lesches]

I think the common factor between ! and shadow is that, to @Hixie's point, they're both workarounds for null-safe promotion, and there is no one-size-fits-all -- you shouldn't add ! everywhere, but not all fields can be shadowed. Is there another big-picture approach we can take here? If not, this just feels like ! with extra steps.

[esDotDev]

I'm not totally clear on what the argument was for not just auto-shadowing, that would clearly be the best option from dev standpoint: we don't see it or think about it, it works like we expect.

I think it was that, some really hard to find (but super rare bugs) could be introduced? Is there a pragmatic approach to that issue? Can we opt in (or better, opt out) with some lint rule?

I think the problem case is something like:

void foo(){
  if(sometimesReturnsNull == null){
    if(sometimesReturnsNull != null) {
      // all of a sudden, this is dead code, it didn't used to be... 
    }
  }
}

This remains a bad edge case to optimize for I think, especially when you consider all of the costs.

[Hixie]

@esDotDev How do you know if something is safe to shadow or not?

class Foo {
  List<int?> _foo = <int?>[null];
  int? get last => _foo.last;

  void test() {
    if (last != null) {
      _foo.add(null);
      print(last.round()); // last is non-null, right?
    }
  }
}

[esDotDev]

I believe, correct me if I'm wrong, the shadowing proposal would actually cache last when you do the comparison. At that point you are just working with a non-null integer for the remainder of this scope.

So that code would work, and developer might be a little surprised? (I haven't yet seen a example case for this that doesn't require totally bizarre code, fwiw)

In this case, because there is no set you could not assign back to last.

[esDotDev]

The latch idea sounds totally fine though:

 if (*last != null) {
  _foo.add(null);
  print(last.round()); // last is non-null, right? ... YES because you latched it
}

...

if(*widget.label != null) Text(widget.label);

...

if(*_controller) return;
useController(_controller);

This gives developers the "Just use this" fix they want in production, but also provides a clear indication something new is going on, and a opportunity to explain to developers what is happening and why.

What I really like about this, vs vanilla caching, is it maintains readability, so we're always working with widget.label or _controller, wheras caching confuses things by introducing duplicate variable names that can often be awkward.

What about use widget.label == null?

[Levi-Lesches]

What about use widget.label == null?

...and we've come full circle to if-vars: #1201

[Hixie]

I think it would be extremely bad if this code:

if (securityProtocol != null) {
  doSomethingHarmless();
  enable(securityProtocol);
}

...were to silently turn into this code:

if (securityProtocol != null) {
  doSomethingHarmless();
  if (securityProtocol != null) {
    enable(securityProtocol);
  }
}

[Hixie]

Maybe I misunderstood. Did you instead mean that it would turn into this?:

if (securityProtocol != null) {
  doSomethingHarmless();
  enable(securityProtocol!);
}

[Hixie]

(If that is what you mean, then that seems bad too. The whole point of null safety is to prevent that.)

[Hixie]

I think there's value in Dart's null safety being sound (which it wouldn't be if we inserted a null check at runtime in the example above), but if we could go back in time I think I would define getters such that they're not allowed to do something that makes them able to return a different value if called twice in a row with no other code in between, unless you explicitly mark them as unstable or something. I expect it's too late to do that though.

[esDotDev]

It's actually worse than that, most of the time with Flutter, the compiler is saying "this field might be overriden in a sub-class even though you are literally typing inside the base class right now, and no class exists in the project scope that extends this class"

Its one of those things where if something is going to be smart, it needs to actually be smart. Partially-smart just ends up in the uncanny valley of intelligence, where it is trying to do too much, but comes off as not very bright at all.

In this case, the programmer knows intuitively that what the compiler is saying is not really true, 1) I have not overridden this field with a getter, 2) I know the the compiler knows that cause it can see every class in our scope.

[Hixie]

The problem is imagine a world where we support post-hoc dynamically loaded libraries. On Monday you write an app that has a base class that does this thing we're talking about and the compiler is happy that nothing overrides the getter and so it allows you to omit the !. On Tuesday, you deploy the app. On Wednesday, you create a plugin for your app that overrides the getter, and deploy that. When should the compiler warn you that the code you wrote on Monday is unsafe?

[esDotDev]

But does that actually matter? If you run with this logic, then no code we have ever written in our lives is actually safe. There is a certain level of risk we just have to accept, because it's not pragmatic otherwise.

But I would assume that I'd get an error on Wednesday when I write the plugin and test it inside of the scope of the larger app. How is the plugin extending the base class without having it in scope? If it does have it in scope, it can flag the overridden field, and the missing ! operators, and call it out then. The plugin author needs to modify/override the base class methods to do explicit var caching, or change their implementation in the sub-class.

In terms of post-hoc dynamically loaded libs, that override fields into getters, and sometimes return null... I mean, feels like we're getting miles off the beaten path here.

[Hixie]

But does that actually matter?

So what difference does it make who inserted the ! - you or the compiler, in terms of safety?

These are philosophical questions. For better or worse, Dart has opted to implement sound null safety, which is to say the compiler guarantees that it will never get into a situation where a type that claims to be non-nullable is null (or throws even though the code does not have a cast or !). Everything else here flows from that decision.

Now, reasonable people can differ on whether a language should have sound null safety or unsound null safety (e.g. Kotlin picked "unsound" as its choice). But that is the choice that has been made, and we must be consistent in our implementation now that we have made that choice.

[esDotDev]

But once ! is introduced it is no longer actually sound. So it doesn't matter if we're putting them in, or the compiler, either way they suffer from the same achilles heel, and are in no way truly "safe".

[Levi-Lesches]

I think this conversation keeps circling around the same three approaches:

1. Implicit !. (IMO, the user should manually insert these) @tatumizer:

   if (securityProtocol != null) {
    enable(securityProtocol); // compiler doesn't complain, but inserts the check
   }

   @esDotDev But once ! is introduced it is no longer actually sound. So it doesn't matter if we're putting them in, or the compiler.

2. If-vars (#1201) @tatumizer

   Now, consider a more heavyweight solution, with local vars.

   var cachedProtocol = securityProtocol; // local var
   if (cachedProtocol != null) {
     enable(cachedProtocol); // hurrah, the compiler promotes
   }

   @esDotDev What about use widget.label == null?

3. Stable getters (#1518) -- @Hixie

   if we could go back in time I think I would define getters such that they're not allowed to do something that makes them able to return a different value if called twice in a row with no other code in between, unless you explicitly mark them as unstable or something.

Personally, I'm against implicit !, because that defeats the point of sound null-safety. I think if-vars can work well in most cases, and manually inserting a ! should be the fallback. Stable getters are a good idea too but I believe that might take slightly longer to implement, and isn't fit for every circumstance anyhow.

[esDotDev]

What's the argument again auto-inserting ! following null checks? The great thing about auto-insert, is it will auto-remove, something the compiler is much better at than me.

For me the entire problem is the manual addition, which is added at some snapshot in time, and can easily be broken later. It cuts the compiler off from any future help.

I agree that with if-vars, this kinda becomes a non issue, as use of ! will hopefully be dropped to near zero with strong guidance from the dart team. I imagine it would be considered quite bad practice to do a null-check + !, when an if-var will do. Since one is truly sound, and the other a potential NPE.

[Levi-Lesches]

My thoughts exactly. Instead of auto-!, wait for if-vars. I guess what I was getting at is "is there a meaningful difference between this issue and if-vars, or are they just two different proposals for syntax?"

[Levi-Lesches]

It seems you're saying that "a getter may return different values at different times, so you can never trust getters". No syntax sugar can save you from that, it's just what happens when you define your own getter. If-vars is still useful because it can help in the case where you know your getter returns the same value if used twice in a row.

And I'd say if-vars do explain the logic -- it's essentially caching the value in a variable of the same name. That's exactly what devs do today when using complex/changing values.