dart-lang / language

Design of the Dart language
Other
2.67k stars 205 forks source link

If-variables #1201

Open munificent opened 4 years ago

munificent commented 4 years ago

This is a proposal for how to handle the lack of field promotion with null safety (though it covers more than just that).

The big hammer in the language for making nullable types usable is flow analysis and type promotion. This lets the imperative code that users naturally write also seamlessly and soundly move nullable variables over to the non-nullable type where the value can be used.

Unfortunately, this analysis isn't sound for fields and getters, so those do not promote:

class C {
  Object obj;
  test() {
    if (obj is int) obj + 1; // Error. :(
  }
}

One option is to enable promotion in the cases where using the field is sound, but the boundary there is subtle, it's easy to move a variable across it, and may be too narrow to cover most cases. Another option is to automatically promote fields when failure to do so would cause a static error. That trades static failures, which let users know their code is unsound, with a runtime error that could cause their program to crash.

Given that the trend in Dart is away from code that may silently fail at runtime, I'm not enthusiastic about the latter approach. This proposal describes a feature called "if-variables" that is local, sound, efficient, explicitly opted in (while being concise), cannot fail at runtime, and covers a larger set of painful cases than any of the other proposals.

It looks like this:

class C {
  Object obj;
  test() {
    if (var obj is int) obj + 1; // OK!
  }
}

Basically, take the code you would write today that doesn't promote and stick a var (or final) in front of the if condition. That keyword means "if the type test succeeds, bind a new local variable with the same name but with the tested type". In other words, the above example is roughly syntactic sugar for:

class C {
  Object obj;
  test() {
    if (obj is int) {
      var obj = this.obj as int;
      obj + 1; // OK!
    }
  }
}

This binds a new local variable. That means reading it later does not read the original backing field and assigning to it does not assign to the field, only to the local variable. This is what makes the proposal efficient and sound. The var keyword should hopefully make it clear enough that there is a new local variable in play.

Promoting on null checks

You can also use if-var with nullability checks:

class C {
  int? n;
  test() {
    if (var n != null) n + 1; // OK!
  }
}

Promoting getters

The tested value can be any expression as long as it ends in a named getter:

class C {
  List<Point<num>> points = [Point(1, 2)];
  test() {
    if (var points[0].x is int) x.isEven; // OK!
  }
}

In this case, the last identifier in the selector chain is the one whose name is used for the newly bound variable. The expression is only evaluated once, eagerly, and the result is stored in the new variable.

So not only does this let you promote a getter, it gives you a very nice shorthand to access the value repeatedly.

Negative if-vars

The above examples all test that some value has a promotable type. You can also test that the variable does not have the type and then exit:

class C {
  Object obj;
  test() {
    if (var obj is! int) return;
    obj + 1; // OK!
  }
}

When using is! and == null, the then branch of the if statement must exit by return, throw, etc. The newly-bound variable goes into the block scope surrounding the if statement and continues to the end of the block. In other words, the desugaring is something like:

class C {
  Object obj;
  test() {
    int obj;
    if (obj is! int) return;
    obj = this.obj as int;
    obj + 1; // OK!
  }
}

Proposal

There are basically two separate statements here:

Here is a somewhat more precise description. We change the grammar like so:

ifStatement         ::= "if" "(" expression ")" statement ( "else" statement )?
                      | positiveIfVariable
                      | negativeIfVariable

positiveIfVariable  ::= "if" "(" ifVariable positiveTest ")" statement ( "else" statement )?
negativeIfVariable  ::= "if" "(" ifVariable negativeTest ")" statement

ifVariable          ::= ( "var" | "final" ) ifValue
ifValue             ::= ( ( primary selector* | "super" ) ( "." | "?." ) ) ? identifier
positiveTest        ::= receiver? identifier ( "is" typeNotVoid | "!=" "null" )
negativeTest        ::= receiver? identifier ( "is" "!" typeNotVoid | "==" "null" )

As far as I know, this is unambiguous and compatible with the existing grammar.

Positive if variables

It is a compile time error if the then statement is a block that declares a local variable whose name is the same as the identifier in ifValue. In other words, the new variable goes in the same block scope as the then block and you can't have a collision.

To execute a positiveIfVariable:

  1. Evaluate the expression ifValue to a value v.
  2. Use that value to perform the appropriate type or null test in the positiveTest. If the result is true:
    1. Create a new scope and bind the identifer from ifValue to v.
    2. Execute the then statement in that scope.
    3. Discard the scope.
  3. Else, if there is an else branch, execute it.

Negative if variables

It is a compile time error if the end of the then statement is reachable according to flow analysis.

It is a compile time error if the block containing the if-var statement declares a local variable whose name is the same as the identifier in ifValue. The scope of the declared variable begins before the if-var statement and ends at the end of the surrounding block. The variable is considered definitely unassigned inside the then branch of the if-var statement and definitely assigned afterwards.

To execute a negativeIfVariable:

  1. In the current scope, declare a new variable named with the identifer from ifValue.
  2. Evaluate the expression ifValue to a value v.
  3. Use that value to perform the appropriate type or null test in the negativeTest. If the result is true:
    1. Execute the then statement.
  4. Else:
    1. Assign v to the variable.

Questions

Compatibility?

Since this claim new currently-unused syntax, it is backwards compatible and non-breaking. We can add it before or after shipping null safety.

Is the local variable's type declared to be the promoted type or promoted to it?

In other words, is the desugaring like:

class C {
  Object obj;
  test() {
    if (obj is int) {
      int obj = this.obj as int;
    }
  }
}

Or:

class C {
  Object obj;
  test() {
    if (obj is int) {
      Object obj = this.obj as int;
    }
  }
}

I suggest the former. Mainly because this prevents assigned an unexpectedly wide type to the local variable. Attempting to do so likely means the user thinks they are assigning to the original field and not the shadowing local variable. Making that a static error can help them catch that mistake.

What about pattern matching?

You can think of this feature as a special pattern matching construct optimized for the common case where the value being matched and the name being bound are the same. I think it's unlikely that this syntax will clash with a future syntax for pattern matching, even if we allow patterns in if statements. The var foo is Type syntax is pretty distinct because it mixes both a little bit of an expression and a bit of a pattern.

What about other control flow combinations?

The positive and negative forms allowed here don't cover every possible valid combination of control flow, scoping, and unreachable code. In particular, we could also allow:

class A {
  Object obj;
  test() {
    if (var obj is! int) {
      ...
    } else {
      obj; // If-variable in scope here.
    }
    // And not here.
  }
}

This isn't particularly useful. You can always swap the then and else cases and turn it into a positive conditional variable.

Also:

class B {
  test() {
    if (var obj is! int) {
      return;
    } else {
      obj; // If-variable in scope here.
    }
    obj; // And also here.
  }
}

There's no real value in allowing an else clause when the then always exits. You can just move the code out of the else to after the if.

Finally:

class C {
  test() {
    if (var obj is int) {
      obj; // If-variable in scope here.
    } else {
      obj; // Definitely unassigned here?
      return;
    }
    obj; // If-variable in scope here too.
  }
}

This one is particularly confusing, since there's a region in the middle where you really shouldn't use the variable.

I don't propose we support these forms. I want it to be clear to users when the conditional variable is scoped to the if statement's then branch and when it goes to the end of the surrounding block. The fewer forms we support, the easier it is for users to understand that.

munificent commented 4 years ago

@leafpetersen @eernstg @lrhn @jakemac53 @stereotype441 @natebosch, thoughts?

mnordine commented 4 years ago

Yeah, I love this. Solves a pain point concisely. The only objection that comes to mind is the "Dart always does what you'd expect" or "no surprises" may come into play with the negative if-vars and the scoping involved there. But as you already cite, there's prior art there, and worth the tradeoff imho.

jamesderlin commented 4 years ago
if (var obj is! int) return;
obj + 1; // OK!

That seems slightly surprising to me since for (var i in iterable) and for (var i = 0; ...) restrict the scope of i to the loop body. Similarly, in C++, if (T x = f()) restricts x to the if body (or to any of its else if or else bodies). And while that's a different language, if there's any possibility that Dart ever might want to allow something similar, it might be confusing if if (var obj is ...) and if (var obj = ...) had different scopes for obj.

lrhn commented 4 years ago

I'm not too fond of making the name of the new variable be implicitly the name of the existing variable. For one thing, it means that it only works for variables (instance or static), but not for arbitrary expressions (like obj as int intobj, #1191, would). It also hides that you are introducing a new variable., which makes it easier to confuse users.

I'd be more inclined towards a full declaration-as-expression: if ((var local = obj) is int) local + 1; That will simply allow you to introduce a local variable as an expression, and use it in any code dominated by the declaration. The declaration expression would be treated as an expression evaluating to the declared variable, so you can use it to promote that variable.

jamesderlin commented 4 years ago

FWIW, one thing that I do like about this (and about @lrhn's suggestion elsewhere for an x as T; statement) is that it wouldn't require introducing a new variable name. There are cases where I'd much prefer to intentionally shadow variables than to have multiple variables that are subtly different but with similar (and sometimes awkward) names, opening the door to accidentally using the wrong variable later.

munificent commented 4 years ago

That seems slightly surprising to me since for (var i in iterable) and for (var i = 0; ...) restrict the scope of i to the loop body.

Yeah, it is somewhat surprising. We could not do this. It's not essential to the proposal. But my hunch is that supporting this would cover a lot of cases where users will expect it to work. The corresponding case where you type promote a local variable does promote the variable in the rest of the block.

I'm not too fond of making the name of the new variable be implicitly the name of the existing variable.

It's magical, but that basically is the value proposition of this feature. If you don't mind writing both the expression and a name for it, you can simply make a local variable:

class C {
  Object obj;
  test() {
    var otherName = obj;
    if (otherName is int) otherName + 1; // OK!
  }
}

The main pain point we have today is that having to write that second name is pointless.

For one thing, it means that it only works for variables (instance or static), but not for arbitrary expressions (like obj as int intobj, #1191, would).

Yeah, I think pattern matching should cover cases like these where you do want to come up with a different name.

It also hides that you are introducing a new variable., which makes it easier to confuse users.

My hope is that var or final are visually distinct enough to mitigate this, but, yes, it's a concern.

leafpetersen commented 4 years ago

It's magical, but that basically is the value proposition of this feature. If you don't mind writing both the expression and a name for it, you can simply make a local variable:

This really is an important point. It sounds small, but my experience with doing this refactor during migrations is that there's a lot of friction introduced by having to come up with a meaningful name for something where the meaningful name is already in use.

rrousselGit commented 4 years ago

Maybe we could have both:

Obj value;

if (var number = value; number is int) {
  int another = number; // valid
}

to support complex expressions

And then offer syntax sugar for the common case, as described in the OP?

jamesderlin commented 4 years ago

Yeah, it is somewhat surprising. We could not do this. It's not essential to the proposal. But my hunch is that supporting this would cover a lot of cases where users will expect it to work.

I do think the ability to declare the variable in that outer scope would be very helpful to avoid needing an extra level of indentation.

Taking a step back, is something like:

if (var obj is int) {
  ...
}

that much better than:

some_new_redeclaration_syntax_for_obj;
if (obj is int) {
  ...
}

?

If we had some way to declare a local version of a variable, I think that we'd get the same benefit without much loss of convenience, and the variable scope would be clear. Throwing some ideas out:

with var obj;
var obj = obj; // (I've always wanted C to allow this so that macros could be hygienic)

Or introducing new, context-dependent keywords:

local var obj;
jakemac53 commented 4 years ago

The difference in scoping between the scenarios is what gives me the most hesitation, although I do agree there are some common scenarios that wouldn't work if you don't add the variable to the outer scope for is!.

One possible resolution would be to always define the variable in the outer scope, in either scenario. That is universally consistent, and it works for all the other control flow combinations that you listed. I think it is a bit unexpected, but we have that case anyways with the specified is! behavior, so it feels like just making that consistent is better.

Edit: Actually the more I think about it the more I think that having it available in the outer scope in either case seems like a real problem. It has a high probability of introducing bugs if people expect it to only be defined for that inner scope - since it is now shadowing the field.

lrhn commented 4 years ago

Our "scopes" are currently mostly following block statement structure, with exceptions:

That's probably just an accident of specification, we can otherwise only declare variables as declaration-"statements", and scopes only matter where they can contain declarations. We do introduce new scopes for the bodies of loops and branches of if statements, even when they are not explicitly block statements.

For example, I don't think label: var x = e; introduces a new scope, even though it is a composite statement.

It wouldn't be too surprising if we defined that all composite statements introduce a new scope covering the entire constructor (perhaps except labeled statements), so while (test) { ... } would have a new scope covering test and body, which only matters if you can declare a variable in test.

If we have expressions introducing variables, that would make a kind of sense. On the other hand, it would preclude potentially useful code like:

if (_instanceVar is! int x) throw "Wot?";
x.toRadixString(16); // Code is dominated by positive edge of `is int x` test.
hpoul commented 4 years ago

I personally like the way this works in swift:

    if let movie = item as? Movie {
        print("Movie: \(movie.name), dir. \(movie.director)")
    } else if let song = item as? Song {
        print("Song: \(song.name), by \(song.artist)")
    }

with dart syntax maybe something like

  if (final movie = item as? Movie) {
    ...
  }

i think as? is a bit more clear than is, it would also be nice if the syntax if (final ...) would generally be true if evaluated to non-null, so you could also use

String? property;
if (final property = property) {
  // property is non-null
}
natebosch commented 4 years ago

I'm not too fond of making the name of the new variable be implicitly the name of the existing variable.

It's magical, but that basically is the value proposition of this feature. If you don't mind writing both the expression and a name for it, you can simply make a local variable:

For static class variables we can write var someName = ThisClass.someName; and for instance variables we can write var someName = this.someName;. The other situations are local variables which couldn't promote for some other reason or top level variables. Do those latter situations come up often enough that the pain of finding a second name is worth this feature? My preference would be for a more general solution that works for other expressions.

eernstg commented 4 years ago

I made an attempt to combine two nice properties: The idea from this proposal that we may introduce a variable with an existing name without repeating that name, and the idea from #1191 'binding type cast and type check' that the introduction of a new name should be usable in a general expression. The resulting proposal is #1210 'binding expressions'.

munificent commented 4 years ago

One possible resolution would be to always define the variable in the outer scope, in either scenario. That is universally consistent, and it works for all the other control flow combinations that you listed. I think it is a bit unexpected, but we have that case anyways with the specified is! behavior, so it feels like just making that consistent is better.

No, that doesn't work because you'll end up with access to a variable whose type does not match its value:

class C {
  Object obj;
  test() {
    if (var obj is int) obj + 1; // OK!
    obj + 1; // If the local obj is still in scope here, what is its type?
  }
}

We could maybe say it's still in scope but un-promote it back to the shadowed member's declared type. So after the if, it demotes back to Object. But that doesn't seem useful. You don't get any useful affordances and now assigning to it just assigns to the shadowed local and not the outer member.

munificent commented 4 years ago

with dart syntax maybe something like

  if (final movie = item as? Movie) {
    ...
  }

I agree that this would be a useful feature and is something I hope we can cover with pattern matching. But this forces you to write the name twice. My goal with this proposal was to give you a nice way to reuse the same name in the very common case where that's what you want.

hpoul commented 4 years ago

My goal with this proposal was to give you a nice way to reuse the same name in the very common case where that's what you want.

hmm.. it's just repeated variable name basically, but if you want to save on typing something like kotlins (item as? Movie)?.apply { print(director) } you'd get rid of the item altogether in scope 😅️ sorry just fantasising, probably not feasible in dart.. OP sounds good, everything in that direction 👍️

jakemac53 commented 4 years ago

We could maybe say it's still in scope but un-promote it back to the shadowed member's declared type. So after the if, it demotes back to Object. But that doesn't seem useful. You don't get any useful affordances and now assigning to it just assigns to the shadowed local and not the outer member.

Right, this is basically what I would expect. Basically, it would allow you to think about if (var obj is int) desugaring to just:

var obj = this.obj;
if (obj is int) ...

So within the if it would be promoted, and outside of that it wouldn't be.

I would ultimately prefer that the variable wasn't available at all outside the if block though, it doesn't feel nearly as intuitive as the rest of darts scoping rules.

munificent commented 4 years ago

I would ultimately prefer that the variable wasn't available at all outside the if block though, it doesn't feel nearly as intuitive as the rest of darts scoping rules.

Yeah, that's definitely an option. We could simply not support the if (var foo is! Bar) ... form at all.

But my experience from type promotion is that that would be annoying. Type promotion used to only promote inside the body of ifs and not in the continuation and it was a constant annoyance to have to take code like:

bool operator ==(other) {
  if (other is! Point) return false;
  return x == other.x && y == other.y;
}

And turn it into:

bool operator ==(other) {
  if (other is Point) return {
    return x == other.x && y == other.y;
  }
  return false;
}

Because the flow analysis wasn't smart enough to see that those two are identical. That's the main reason I propose supporting both forms: I think there is code that looks natural in both styles and it would feel annoying if only one worked.

I do agree the scoping is surprising in the negative form. I'm just not sure if it's weird enough to sink the proposal.

leafpetersen commented 4 years ago

@munificent I would like to explore what some actual code looks like under these various proposals. Here's an example piece of code that I migrated, which I think suffers from the lack of field promotion. This is from the first patchset from this CL - I subsequently refactored it to use local variables, the result of which can be seen in the final patchset of that CL. This was a fairly irritating refactor, and I don't particularly like the result. Would you mind taking a crack at showing how this code would look under your proposal?

// The type of `current` is `Node`, and the type of the `.left`  and `.right` fields is `Node?`.
while (true) {
      comp = _compare(current.key, key);
      if (comp > 0) {
        if (current.left == null) break;
        comp = _compare(current.left!.key, key);
        if (comp > 0) {
          // Rotate right.
          Node tmp = current.left!;
          current.left = tmp.right;
          tmp.right = current;
          current = tmp;
          if (current.left == null) break;
        }
        // Link right.
        right.left = current;
        right = current;
        current = current.left!;
      } else if (comp < 0) {
        if (current.right == null) break;
        comp = _compare(current.right!.key, key);
        if (comp < 0) {
          // Rotate left.
          Node tmp = current.right!;
          current.right = tmp.left;
          tmp.left = current;
          current = tmp;
          if (current.right == null) break;
        }
        // Link left.
        left.right = current;
        left = current;
        current = current.right!;
      } else {
        break;
      }
    }
munificent commented 4 years ago

OK, here's what I got, with comments on the changes:

while (true) {
  comp = _compare(current.key, key);
  if (comp > 0) {
    if (var current.left == null) break;  // add "var", negative form.
    comp = _compare(left.key, key);       // "current.left!" -> "left"
    if (comp > 0) {
      // Rotate right.                    // remove "tmp" (use "left" as temp)
      current.left = left.right;          // "tmp" -> "left"
      left.right = current;               // "tmp" -> "left"
      current = left;                     // "tmp" -> "left"
      if (current.left == null) break;
    }
    // Link right.
    right.left = current;
    right = current;
    current = current.left!;              // unchanged, "left" could be stale
  } else if (comp < 0) {
    if (var current.right == null) break; // add "var", negative form.
    comp = _compare(right.key, key);      // "current.right!" -> "right"
    if (comp < 0) {
      // Rotate left.                     // remove "tmp" (use "right" as temp)
      current.right = right.left;         // "tmp" -> "right"
      right.left = current;               // "tmp" -> "right"
      current = right;                    // "tmp" -> "right"
      if (current.right == null) break;
    }
    // Link left.
    left.right = current;
    left = current;
    current = current.right!;             // unchanged, "right" could be stale
  } else {
    break;
  }
}

I'm not 100% sure I did that right.

This is a really useful exercise. I might try to dig up some other migrated code and see how it would look if we had this.

jodinathan commented 4 years ago

this looks very interesting, however, I find the var in the sentence misleading.

In some other issue I commented about using a new keyword use to add variables to the scope. It is a little more verbose than the var this proposal, but I find easier to read as it explicitly tells what is happening and why it is needed. It could be also used in other situations like type promotion and any other kind of conditional expression.

In the code above, @munificent comments on the use of var in the if condition and uses the word use to explain how it works. // remove "tmp" (use "left" as temp)

If anyone found this interesting, help me creating a proposal.
I have ADHD so creating big, nice texted, unfortunately, is not my thing.

this is the same piece of code but tweaked to show some more usage of the use keyword:


while (someObject.someProp != null use obj) { // you can also use the keyword use here
  generic = obj._someMethod(current.key, key); // obj was added to this scope in the line above

  if (generic is Foo use current) { // add a current local variable that is Foo
    if (current.left == null use left) break;  // add a local var *left* within (comp > 0) scope

    comp = _compare(left.key, key);       // "current.left!" -> "left"

    if (comp > 0) {
      // Rotate right.                    // remove "tmp" (use "left" as temp)
      current.left = left.right;          // "tmp" -> "left"
      left.right = current;               // "tmp" -> "left"
      current = left;                     // "tmp" -> "left"
      if (current.left == null) break;
    }

    // Link right.
    right.left = current;
    right = current;
    current = current.left!;              // unchanged, "left" could be stale
  } else if (generic is Bar use current) { // current in this scope is set to a Bar type
    if (current.right == null use right) break; // add "var", negative form.

    comp = _compare(right.key, key);      // "current.right!" -> "right"

    if (comp < 0) {
      // Rotate left.                     // remove "tmp" (use "right" as temp)
      current.right = right.left;         // "tmp" -> "right"
      right.left = current;               // "tmp" -> "right"
      current = right;                    // "tmp" -> "right"
      if (current.right == null) break;
    }

    // Link left.
    left.right = current;
    left = current;
    current = current.right!;             // unchanged, "right" could be stale
  } else {
    break;
  }

}
jodinathan commented 4 years ago

I agree with @jakemac53 about the scope of the if-variable var, so I tweaked @leafpetersen's code to how I think is the most organized and easy to read option:


    while (true) {
      comp = _compare(current.key, key);

      if (comp > 0) {
        if (current.left != null use left) {
          if (_compare(left.key, key) > 0) {
            // Rotate right.
            current.left = left.right;
            left.right = current;
            current = left;

            if (current.left == null) { 
          break;
            }
          }

          // Link right.
          right.left = current;
          right = current;
          current = current.left!;
    } else {
      break;
    }
      } else if (comp < 0) {
        if (current.right != null use right) {
          if (_compare(right.key, key) < 0) {
            current.right = right.left;
            right.left = current;
            current = right;

            if (current.right == null) { 
              break;
            }
          }

          // Link left.
          left.right = current;
          left = current;
          current = current.right!;

    } else {
          break;
        }
      } else {
        break;
      }
    }

We could also give the option to only add the keyword use so the name of the variable keeps the same, ie:

if (current.right != null use) 

But I am really not sure if I like it. Seems too magical to me and I like explicit stuff.

*edit: I pondered about the alone use above and I liked it. One option is to add another keyword for it:

if (current.right != null useIt)

now I have the right variable without having to write down "right" and it is clear and concise.

Levi-Lesches commented 3 years ago

I love this idea, but I'm still not quite sold on the scope of negative if-vars. As mentioned before, anytime you have a variable declaration in parenthesis followed by curly brackets, the variable is always scoped to the curly brackets. Examples:

void print(String message) {
  // message only exists here
}

for (int i = 0; i < 5; i++) {
  // i only exists here
}

for (final int datapoint in dataset) {
  // datapoint only exists here
}

if (var json is String) {
  // json only exists here
}

if (var age is! int) {
}
// age exists here

Breaking this expectation seems unnatural and unintuitive. I'm not sure what a better alternative is, but if this whole feature is really just syntactic sugar, then maybe it doesn't need to support negative if-vars. IMO, the extra line to properly declare the variable would make the code easier to read.

Another angle, seeing as it is syntax sugar, you shouldn't lose anything from including it. In other words, the following examples are the same:

void validateResponses(List<Object?> responses) {
  for (final Object? response in responses) {
    if (response is String);      // doesn't use the feature
    if (var response is String);  // opt-in to use the feature, no error even if not used. 
  }
}

But consider this:

void validateResponses(List<Object?> responses) {
  for (final Object? response in responses) {
    if (var response == null) {  // compile-time error since this doesn't throw
      print("User chose not to respond");
    }
    if (response == null) {  // no error
      print("User chose not to respond");
    }
  }
}

I think the assumptions of negative if-vars, that the code should exit and the variable be out-of-scope, may not be intuitive enough.

jodinathan commented 3 years ago

@Levi-Lesches I agree and for now I am totally against negative-vars.

munificent commented 3 years ago

Yeah, that seems to be the consensus. I do think the negative-vars is a useful feature. Dart's existing type promotion syntax used to not promote in the rest of a block in cases where doing so was logical and it was always an annoying limitation. (We fixed it as part of null safety.) But it may be that the syntax proposed here is just too weird to be intuitive.

Levi-Lesches commented 3 years ago

So what's the status of the positive if-vars? It's a really cool feature I'd love to see in Dart. Overall, I love that Dart has these intuitive features that save a line or two where other languages would need us to spell it out.

munificent commented 3 years ago

So what's the status of the positive if-vars?

No current plans to work on it. We know the lack of field promotion is a pain point, but we'd like to get more real-world feedback on null safety in the wild to get a better sense of how acute a problem it is and thus how much it's worth adding complexity to the language to deal with it.

lrhn commented 3 years ago

We could change Dart's var declaration scope in general, so a var declaration is visible in all code that it dominates (that is, code where control flow cannot reach without going through the var declaration, based on static flow analysis where all flow is considered possible, even if a condition ends up always being true or false).

That would solve a bunch of issues right away:

do {
  var node = something.next();
} while (node != null); // `node` now visible in `while` condition. And after.

or

for (var i = 0; i < 100; i++) {
  if (test(i)) break;
}
firstNonValid(i);

allowing you to continue using the value at the end of the loop afterwards.

If we have

var i = 42;
for (var i = 0; i < 10 ; i++) something(i);
print(i); // conflict, so we probably want to prevent the inner declaration from flowing out into the outer scope.

It's obviously a breaking change since it can put a variable into a scope where it previously wasn't available, where it could shadow a top-level or instance variable

We already have this scope knowledge because we allow an assignment to promote a variable in precisely the scope dominated by the assignment.

Levi-Lesches commented 3 years ago

I'll refer to my earlier comment and say that people are very used to variables only existing where they define them. If the variables start leaking out, there might be issues, like you pointed out. That's why I opposed the negative if-vars, but the positive if-vars keep to this assumption quite well.

lrhn commented 3 years ago

The var obj expression, introducing a variable named obj and default initialized to the value of the expression obj (in a scope where the new variable isn't visible yet) seems like something which can be generalized.

It only works as the first operand of an is or == check. Why not work everywhere? There is no need to only allow it in tests. You should also be able to do [var defaultValue, defaultValue, defaultValue] where defaultValue is a getter which you don't want to compute multiple times (perhaps it increments a counter each time, or something). Basically, any occurrence of id should be able to be turned into var id. Tests are not special except for the actual testing part.

It's not clear whether it's intended to work with anything but instance variables. It should also work with getters (there must not be a distinction between getters and fields), so it should probably work with any expression. It's harder to rewrite when you can't just put this. in front of the name to avoid the naming conflict.

That would allow you to introduce a variable at any point in the code, as long as you have an existing single-identifier-expression with that same name to initialize it with. This still seems awfully specific, so I'd recommend #1420 declaration expressions like (var obj = obj) is int in general, and then allow var id as shorthand for var id = (some-scope-override::)id. (We will need to figure out the scopes since normally var id = id would be invalid because the declaration shadows the RHS.)

All in all, I think this proposal is probably too restricted. We could get more bang for the buck by making the feature more general.

For scope of the declared variable, I'd go with something like:

A variable declared inside a test expression is available in both branches of the condition. If the branches are themselves used in conditions (for conditional expressions or &&/|| combinations) the variable is also in scope for the branches of those conditions. If the test is a condition of an if statement with no else branch, and the then branch cannot complete normally, then the variable is also in scope in the rest of the statements, following the if statement, of the containing block.

That is, treat an always-escaping then branch with no else as a sign that the following code is the else branch. If you have an else branch, that's the limit of the variable. Nothing similar for loops (perhaps if we get loop-elses, #171!)

lrhn commented 3 years ago

@tatumizer That's precisely why I only wanted the scope to continue after the if if the then branch cannot complete normally (meaning it always returns, throws, breaks or continues, ... and I'll probably explicitly disallow label: if (test) break label;). That's when it would be equivalent to take the following code and wrap it into else { ... }.

nex3 commented 3 years ago

I'd like to urge that this be made available in ternary expressions as well, so you could write (var foo != null) ? fn(foo) : defaultValue. This would be particularly useful in combination with https://github.com/dart-lang/language/issues/361, where it would allow you to transform this frustrating pattern:

var foo = object.foo;
var result = foo == null ? null : fn(foo);

into the simple statement

var result = (var object.foo) ?! fn(foo);
lrhn commented 3 years ago

It should be available in any branch condition, meaning the condition of if and while (do/while probably won't work), the conditional expression (?/:), and the first operand of the short-circuit condition operators (&& and ||).

(I still think introducing variables is so useful it should be allowed everywhere, not just in tests, then tests would just be adding promotion to the existing feature).

esDotDev commented 3 years ago

I really appreciate the pragmatism of this approach, it is optimizing for the right use-cases here imo which is:

  1. Don't make the developer repeat themselves
  2. Don't forces us to come up with alternate names, for existing fields. It hurts readability: If I'm working on _image class field, I want the code to indicate that wherever possible. And quality of life: It's annoying having to create alternate names for things like label, image, url, controller etc.

Also really like that this doesn't introduce a new keyword, which should make it very easy for devs to grok.

esDotDev commented 3 years ago

So what's the status of the positive if-vars?

No current plans to work on it. We know the lack of field promotion is a pain point, but we'd like to get more real-world feedback on null safety in the wild to get a better sense of how acute a problem it is and thus how much it's worth adding complexity to the language to deal with it.

Has this question been somewhat settled yet? Is it still an open question if this pain point should be dealt with?

The issue is obviously the longer this takes to come, the more ! filled, potentially non-sound code devs will produce in the meantime.

leafpetersen commented 3 years ago

Has this question been somewhat settled yet? Is it still an open question if this pain point should be dealt with?

@esDotDev We are planning to spend time evaluating these proposals over the next few months. No promises, but we're hoping we can find something we feel is worth shipping.

lrhn commented 3 years ago

@tatumizer I have.

lrhn commented 3 years ago

If it prevents x := e from being used as a new variable in non-test contexts, then I'd be sorry. It's a very nice syntax, and restricting it to only work in a boolean context is a waste.

We could make x := e is T mean (x := e) is T. It probably has some consequences about which expressions can occur as the e without parentheses, and it might be confusing to people who'd expect it to mean x := (e is T). Using x := e as shorthand for (x := e) != null (which is what is T! where T is the static type of e would mean) either means that x := e is always a boolean expression, or means that x := e means different things depending on whether it's used in a boolean context or not. The latter is possible, but potentially confusing, especially if e has a boolean type. It's also non-compositional because then (x := e) itself behaves differently depending on whether it's followed by is T or not.

So, not a good syntax IMO. Too specific. Too unreadable.

Short is not a primary goal for me. Something short and highly tuned towards a single use-case is likely to become a liability in the long run, because it locks up the syntax and becomes a shorthand for just one particular thing. I'd rather have x := e to work everywhere as a fresh-variable introduction, even if I have to write if ((x := e) != null) with parentheses. Then when we introduce Flooming types in Dart 3.14, and need to do Floom tests three times for every nullability test, we won't have wasted our shorthand on nullability.

erf commented 3 years ago

In Swift you are able to unwrap nullable values using guard statements like this:

  guard final myNonNullStr = myObj?.someVar?.someString else {
    // was not able to declare myNonNullStr, something was null
    return;
  }
  print(myNonNullStr);

Would you be able to solve this using this proposal? I suggested the guard statement here

esDotDev commented 3 years ago

It's unfortunate that the negative if-var seems to be cut. The early exit use case seems like one of the higher % applications here.

void handleBtnPressed(){
  if(use counter == null) return;
  counter++;  
  // Do other stuff with counter
}
  1. Isn't this somewhat mitigated if we have another keyword than 'var'? use is a new keyword, it could carry with it new behavior?
  2. Semantically there is consistency here. In both cases the propagation is following the flow of the code. If a successful check causes it to enter the block, the type is promoted there, if a successful check results in not entering the block, then the type is promoted outside the block.

In the likely case that this convinces no-one, it would be nice if we could at least use this in this sort of way:

void increment(){
  use counter;
  if(counter == null) return;
  counter++;  
}
Levi-Lesches commented 3 years ago

What you're suggesting is almost identical to "shadows", being discussed in #1514. I've asked a lot of questions on that issue and many other similar ones, and personally I think that's the best way to handle a lot of issues (like null-safe promotion) with a very simple syntax. Your example would look like this:

class ButtonWidget {
  int? counter;

  void increment() {
    shadow counter;
    if (counter == null) return;
    counter++;  // promoted
  }
}

Which is equivalent, in today's code, to the following:

class ButtonWidget {
  int? counter;

  void increment() {
    int? counter = this.counter;
    if (counter == null) return;
    counter++;  // promoted
    this.counter = counter;
  }
}

void main() {
  final ButtonWidget widget = ButtonWidget();
  widget.increment();
  print(widget.counter);  // null
  widget.counter = 0;
  widget.increment();
  print(widget.counter);  // 1
}
esDotDev commented 3 years ago

We have run into a specific realworld use case that illustrates the issue with type promotion now, and may be a good test case for if-vars.

This is production code, taken from the Adobe-To-Xd plugin, for a PinStack widget. This widget has a bunch of double? because it is very flexible and supports many pinning configs (matching AdobeXD's internal capabilities).

Currently, legacy looks like this:

class Pin {  // Nullable fields
  final double start;
  final double startFraction;
  final double end;
  final double endFraction;
  final double size;
  final double middle;
}
Span { @required final double start; @required final double end; } 
...
_Span _calculateSpanFromPin(Pin pin, double maxSize) {
    double start = 0.0, end = 0.0;
    if (pin.size == null) {
      if ((pin.start ?? pin.startFraction) == null || (pin.end ?? pin.endFraction) == null) {
        // Empty pin, fill size:
        start = 0;
        end = maxSize;
      } else {
        // Size is unknown, so we must be pinned on both sides
        start = pin.start ?? pin.startFraction * maxSize;
        end = maxSize - (pin.end ?? pin.endFraction * maxSize);
      }
    } else if (pin.size >= maxSize) {
      // Exceeds max size, fill.
      // Note: this isn't exactly what XD does, but it's the closest we can get without overflow.
      start = 0;
      end = maxSize;
    } else if (pin.start != null || pin.startFraction != null) {
      // Pinned to start
      start = min(maxSize - pin.size, pin.start ?? pin.startFraction * maxSize);
      end = start + pin.size;
    } else if (pin.end != null || pin.endFraction != null) {
      // Pinned to end
      end = max(pin.size, maxSize - (pin.end ?? pin.endFraction * maxSize));
      start = end - pin.size;
    } else {
      // Not pinned at all, use middle to position
      start = pin.middle * (maxSize - pin.size);
      end = start + pin.size;
    }
    return _Span(start, end);
  }

It seems very challenging to migrate this to nnbd with out a ton of ! operators, any thoughts? Currently it is very readable and succinct, but it doesn't take much boilerplate for it to become unwieldy fast.

Would also be interesting to see how if-vars would clean this up.

Levi-Lesches commented 3 years ago

Well, for starters, regular final double? pinFoo = pin.foo; should work. But, after trying it myself, I hit a problem.

Interestingly, Dart is unable to prove that in the first else, that pin.startFraction can't be null if pin.start is null. In other words, you would get a "possible null" error using pinStart and pinStartFraction with the line:

start = pinStart ?? (pinStartFraction * maxSize);  // Error: * on possible null value

Following the logic, if both were null, then the if would run. So the else only runs if pinStart is not null or pinStartFraction is not null. I wonder if that's within Dart's power to realize that. Even rewriting it to simple && and || doesn't help:

if ((pinStart ?? pinStartFraction) == null || (pinEnd ?? pinEndFraction) == null)
if (
  (pinStart == null && pinStartFraction == null)
  || (pinEnd == null && pinEndFraction == null)
)

For an even simpler example:

void test(double? a, double? b) {
  if (a == null && b == null) {
    print("This is equal to: if (a ?? b) == null");
  } else {
    print("If a is null, then b can't be null");
    print(a ?? (b * 5));  // Still get an error here
  }
}

All that to say that if Dart can't promote these variables here, I don't think if-vars or any other syntax sugar will help much.

Also, can anyone with deep knowledge on nnbd tell me if this is intended behavior?

esDotDev commented 3 years ago

Ya we also found it confusing why the compiler can't follow pinStartFraction here, even with local caching:

if ((pinStart ?? pinStartFraction)) == null) {
...
} else {
  print(pinStart ?? pinStartFraction * maxSize); // ERROR: pinStartFraction could be null :'(
}
esDotDev commented 3 years ago

fwiw, here's where we landed after conversion:

_Span _calculateSpanFromPin(Pin pin, double maxSize) {
    // default to filling the space:
    double start = 0.0, end = maxSize;

    // copy all the values locally to support null-safety:
    double? pinSize = pin.size, pinMiddle = pin.middle;
    double? pinStartF = pin.startFraction, pinEndF = pin.endFraction;
    double? pinStart = pinStartF != null ? pinStartF * maxSize : pin.start;
    double? pinEnd = pinEndF != null ? pinEndF * maxSize : pin.end;

    // duplicate some of the asserts locally to support null-safety:
    if (pinStart != null && pinEnd != null) {
      // Pinned on both sides.
      start = pinStart;
      end = maxSize - pinEnd;
    } else if (pinSize != null && pinStart != null) {
      // Pinned to start
      start = min(maxSize - pinSize, pinStart);
      end = start + pinSize;
    } else if (pinSize != null && pinEnd != null) {
      // Pinned to end
      end = max(pinSize, maxSize - pinEnd);
      start = end - pinSize;
    } else if (pinMiddle != null && pinSize != null) {
      // Not pinned at all, use middle to position
      start = pinMiddle * (maxSize - pinSize);
      end = start + pinSize;
    }
    return _Span(start, end);
  }
arbitur commented 3 years ago

I would highly opt for the Swift style where you can change the variable name for something more descriptive as to what the casted variable is.

if (final post = object as? Post) post.doWhatPostsDoes(); // Highly preferable
if (object is Post) object.doWhatPostsDoes();

The only time I would agree with @munificent quote "But this forces you to write the name twice" is where you only want to unwrap a nullable variable.

if (post != null) post.doWhatPostsDoes();  // Preferable
if (final post = post) post.doWhatPostsDoes();

Though having both functionalities in the language might feel inconsistent or make it too difficult to implement so I would opt for the more versatile Swift-style option.

munificent commented 3 years ago

When I was migrating the scrape package to null safety, I stumbled onto some code that would have used this proposal. I started with this code:

    if (node.extendsClause != null) {
      record('Uses', 'extend');
      record('Superclass names', node.extendsClause.superclass.toString());
    }

    if (node.withClause != null) {
      for (var mixin in node.withClause.mixinTypes) {
        record('Uses', 'mixin');
        record('Mixin names', mixin.toString());
      }
    }

    if (node.implementsClause != null) {
      for (var type in node.implementsClause.interfaces) {
        record('Uses', 'implement');
        record('Superinterface names', type.toString());
      }
    }

Since fields don't promoted, the migrated code looks like:

    var extendsClause = node.extendsClause;
    if (extendsClause != null) {
      record('Uses', 'extend');
      record('Superclass names', extendsClause.superclass.toString());
    }

    var withClause = node.withClause;
    if (withClause != null) {
      for (var mixin in withClause.mixinTypes) {
        record('Uses', 'mixin');
        record('Mixin names', mixin.toString());
      }
    }

    var implementsClause = node.implementsClause;
    if (implementsClause != null) {
      for (var type in implementsClause.interfaces) {
        record('Uses', 'implement');
        record('Superinterface names', type.toString());
      }
    }

Each field is copied to a local variable which is then used. With the proposal here, the migration would have been:

    if (var node.extendsClause != null) {
      record('Uses', 'extend');
      record('Superclass names', extendsClause.superclass.toString());
    }

    if (var node.withClause != null) {
      for (var mixin in withClause.mixinTypes) {
        record('Uses', 'mixin');
        record('Mixin names', mixin.toString());
      }
    }

    if (var node.implementsClause != null) {
      for (var type in implementsClause.interfaces) {
        record('Uses', 'implement');
        record('Superinterface names', type.toString());
      }
    }

Note that the migrated code actually gets a tiny amount shorter than the original because we get to both check the value and bind to a shorter local variable in one step.

This is a cherrypicked example, but it is real code.

ykmnkmi commented 3 years ago

May be a bit confusing, shorter null cheking in ?::

final user = var id = form['id'] ? User.getBy(id: id) : User.current;
cedvdb commented 3 years ago
    var extendsClause = node.extendsClause;
    if (extendsClause != null) {
      record('Uses', 'extend');
      record('Superclass names', extendsClause.superclass.toString());
    }

Using the bloc library I have that kind of thing everywhere you have to check a type, which is everywhere using bloc:

  stopRecording() async {
    final state = this.state;
    if (state is Recording) {
      final path = await _recorderSrv.stop();
      emit(RecordCompleted(path, state.duration));
    }
  }