-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
If-variables #1201
Comments
@leafpetersen @eernstg @lrhn @jakemac53 @stereotype441 @natebosch, thoughts? |
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. |
That seems slightly surprising to me since |
I'm not too fond of making the name of the new variable be implicitly the name of the existing variable. I'd be more inclined towards a full declaration-as-expression: |
FWIW, one thing that I do like about this (and about @lrhn's suggestion elsewhere for an |
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.
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.
Yeah, I think pattern matching should cover cases like these where you do want to come up with a different name.
My hope is that |
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. |
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? |
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; |
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 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 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. |
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 For example, I don't think 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 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. |
I personally like the way this works in swift:
with dart syntax maybe something like
i think
|
For static class variables we can write |
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'. |
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 |
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. |
hmm.. it's just repeated variable name basically, but if you want to save on typing something like kotlins |
Right, this is basically what I would expect. Basically, it would allow you to think about
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. |
Yeah, that's definitely an option. We could simply not support the 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. |
@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;
}
} |
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. |
this looks very interesting, however, I find the In some other issue I commented about using a new keyword In the code above, @munificent comments on the use of If anyone found this interesting, help me creating a proposal. this is the same piece of code but tweaked to show some more usage of the 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;
}
} |
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 if (current.right != null use)
*edit: I pondered about the alone if (current.right != null useIt) now I have the |
+1 to the idea of if-variables. One thought: would 'final' be more aligned given its existing meaning, or would 'var' and 'final' both be usable, where 'var' would allow writing to the local variable of the restricted type? Having 'a' be assigned inside the statement seems like it could lead to confusing code. if (final a != null) {
// Use a as non-null, but assignment to 'a' is not allowed.
} |
The proposal allows both |
If 'var' is used, does writing to it affect the underlying value, or a shadow copy of it? From the proposal, it seems like a local copy. if (var someObject.a != null) {
a = 1; // Seems like this just writes to a local 'a'.
} In this case, any thoughts on how private variable names should work? if (final _fieldValue != null) {
// `_fieldValue` or `fieldValue` here?
} Using a local |
You introduce a new variable with the chosen name. That means it shadows the original thing you read. The new variable has the same name as the variable (or getter) that was read to create it. |
Perhaps going off on a tangent, but might class MyClass {
MyClass.positional(this.value);
MyClass.optionalPositional([this.value]);
MyClass.named({required this.value});
MyClass.optionalNamed({this.value});
final int value;
} But for privates: class MyClass {
MyClass.positional(this._value);
MyClass.optionalPositional([this._value]);
// Not possible: MyClass.named({required this._value});
// Not possible: MyClass.named({required this.value});
MyClass.named({required int value}) : _value = value;
// Not possible: MyClass.optionalNamed({this._value});
// Not possible: MyClass.optionalNamed({this.value});
MyClass.optionalNamed({int value}) : _value = value;
final int _value;
} Note: Field parameters shadowing other variables can happen, even in the current proposal. var a = 0;
if (var otherObject.a != null) {
// No way of accessing var a above here. Though I readily admit keeping underscore for the local is probably easier conceptually and implementation wise.... just writing some random thoughts. |
I do wonder whether we want to either make these variables always final by default, or only allow the
|
That sounds a lot like the class C {
Object obj;
void test() {
shadow obj; // references to obj are local, but are synced to the field
if (obj is int) obj++; // increments both the local variable AND the field
}
} |
Quick thinking this seems an exception instead of the common case. |
I liked the idea but this is so weird to read:
I think we should make it a suffix because that is how our brain separates actions, ie: do this then do that. if (obj is int use obj && arg != null use safeArg) {
// the new local obj is final and an int
// the new local safeArg is final and non null
} Because of the word But it could be var or final: if (obj is int var obj && arg != null final safeArg) {
// the new local obj is variable and an int
// the new local safeArg is final and non null
} |
Used C/C++ a lot 15 years ago. I wouldn't choose all things from it blindly.
Would you mind exposing the advantages here? For me is basically the same amount of work that we already do: final o=obj, o1=obj1;
if (o is int && o1 is int) {
// use o, o1
} In fact, reading again, I find the current Dart syntax better to read than the C++ version. |
Similar to Python and Go walrus operator, but shorter, like labeled blocks: if (final user: object is User && /* var */ data: user.data /* != null */) {
data = callback(data);
// ...
send(user.email, messageFor(data));
} |
IMO it looks like it returns a |
C# apparently has a pretty tight syntax for this sort of thing that allows the user to specify a variable name:
|
Yeah, I considered that. Like @tatumizer notes, it doesn't extend gracefully to |
With pattern matching coming out, could we also extend this proposal to allow Example: enum Difficulty {
easy(1, "Easy"),
medium(2, "Medium"),
hard(3, "Hard");
static Difficulty? fromId(int id) => Difficulty.values.firstWhereOrNull((d) => d.id == id);
const Difficulty(this.id, this.designation);
final int id;
final String designation;
}
class Game {
final Difficulty difficulty;
final int levels;
final String name;
Game(this.difficulty, this.levels, this.name);
}
void main() async {
final json = jsonDecode('{"name": "name, "levels": 100, "difficultyId": 1}');
if (json case {
'name' : final String name,
'levels' : final int levels,
'difficultyId': final int difficultyId,
} when (final difficulty = Difficulty.fromId(difficultyId) != null)) {
final game = Game(difficulty, levels, name);
// insert game and return 200 ok
}
//return 400 bad request
} |
A more generic way, could be something more like C# 'out' parameter modifier For example instead of: final uri = Uri.tryParse(str);
if (uri != null) {
...
} you will have something like: if (Uri.tryParse(str, out uri)) {
... play with uri
} and the tryparse signature: bool tryParse(String str, out Uri uri) best regards, Alexandre |
The C# out parameter is just a reference parameter, with the added requirement that the method does assign to the variable. They allow you to declare the variable in-place. Which means that it wouldn't work for An In the next Dart, I'd use patterns: if (Uri.tryParse(source) case var uri?) {
// Use uri
} |
@lrhn Thank you for pointing me this pattern matching solution. |
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:
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:
Basically, take the code you would write today that doesn't promote and stick a
var
(orfinal
) in front of theif
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: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:Promoting getters
The tested value can be any expression as long as it ends in a named getter:
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:
When using
is!
and== null
, the then branch of theif
statement must exit byreturn
,throw
, etc. The newly-bound variable goes into the block scope surrounding theif
statement and continues to the end of the block. In other words, the desugaring is something like:Proposal
There are basically two separate statements here:
A positive
if-var
that usesis
or!= null
in the condition and scopes the newvariable only inside the then branch. It's somewhat like
if-let
in Swift.A negative
if-var
that usesis!
or== null
in the condition and scopes the new variable to the code after the if statement. It's akin toguard-let
in Swift.Here is a somewhat more precise description. We change the grammar like so:
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
inifValue
. 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
:ifValue
to a valuev
.null
test in thepositiveTest
. If the result istrue
:identifer
fromifValue
tov
.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 theidentifier
inifValue
. The scope of the declared variable begins before theif-var
statement and ends at the end of the surrounding block. The variable is considered definitely unassigned inside the then branch of theif-var
statement and definitely assigned afterwards.To execute a
negativeIfVariable
:identifer
fromifValue
.ifValue
to a valuev
.null
test in thenegativeTest
. If the result istrue
:4. Execute the then statement.
5. 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:
Or:
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. Thevar 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:
This isn't particularly useful. You can always swap the then and else cases and turn it into a positive conditional variable.
Also:
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:
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.
The text was updated successfully, but these errors were encountered: