-
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
Shadowing a non-local variable #1514
Comments
Would this also allow assigning to the field (without
|
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 |
I think you're right. I like it. |
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.
}
}
} |
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 (#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. I think this proposal is aiming too low. Fields (or even static/top-level variables) are not the only problem! So, what if you could shadow any assignable expression. Let's say void foo(List<int?> foos) {
shadow s = foo[1];
if (s != 0) s += 42;
} where 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 Maybe it should be |
@rrousselGit wrote:
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 |
@tatumizer wrote:
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 |
@tatumizer wrote:
I love it. 😁 |
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 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 |
@lrhn wrote:
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;
...
}
}
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.
As you mention, we'd probably just make the local variable late as well.
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? ;-)
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.
That's an interesting idea! It does require the shadowed expression to denote a property (so 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'!
}
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
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 ( |
We definitely wouldn't. If we do 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.
} |
Cool! Sounds like that could work. |
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? |
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. This means @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. |
#1518 outlines an idea, stable getters, which would allow us to promote a getter invocation or even a whole path of getter invocations. |
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. |
Couple thoughts to toss in:
|
@Levi-Lesches mentioned the 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 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. ;-) |
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. |
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. |
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. if (list.isNotEmpty) something(list.last); where A marker to enable unsound access means something like #1187 (not #1188, which is implicit). The only question then is which syntax to use. @leafpetersen suggested
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 |
@esDotDev wrote:
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 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:
Let me plug this one again: #1187 (comment) 😄 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 " |
We all know that Other than that, I think it makes a big difference that 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. |
@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 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? (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). |
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)
My example has the user declare a |
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 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. ;-) |
@eernstg okay so I see from your example that we should only allow Out of all the "declaration expression"/if-vars/etc. proposals I've read (like 6 of them), I support this one since it most closely resembles how this would be done without today, which makes it essentially just sugar, and is more readable IMO than declaring variables inline. To further readability, I would support forcing |
#1980 was closed as a duplicate of this. 1980 had suggested using the keyword |
Really looking forward to a solution for this. Since I don't like using the final amount = this.amount;
if(amount != null) {
return execute(amount);
} I really like the Swift if-let optional chaining approach which is really simple and doesn't introduce complex new concepts (like For example, this would give : if final amount = this.amount {
return execute(amount);
} Here, the scope is clear for the local shadowing variable, in my opinion. |
Where's the null check, is it implicit somehow? |
I have an alternative proposal for shadowing. Instead of using with amount;
if (amount != null) {
return execute(amount);
} In cases where you don't want to pollute your scope with random "with" imports, they can be nested within expressions: if (with amount: amount != null) {
return execute(amount);
} Or potentially it can be used as a prefix inside an expression to lift it into a variable binding? (trying to think of a way to avoid duplicating "amount"): if (with amount != null) {
return execute(amount);
} In general, with bindings attempt to use the last identifier in the expression as the name of the introduced variable. If this isn't possible or desired, with amount.value as amount; Also, when used as an expression prefix or a statement, multiple bindings can be introduced with with widget.child, widget.leading, widget.trailing;
return Row(children: [if (leading != null) leading, /* etc. */]); Perhaps cascade syntax can be overridden within the with binding to introduce bindings: with widget..child..leading..trailing, foo, bar; For mutability, it makes sense to allow with bindings to be mutable, where they refer to an associated setter. For example: with json..["foo"]..["bar"];
foo = "foo value";
bar = "bar value"; Of course, I think it would be wise to make the syntax equivalent to a lazy & locally cached variable if used like this. Otherwise you risk unnecessary calls to getters. Anyways to be honest, I'm just throwing things at a wall to see what sticks here. Would like to see improvements here around variable shadowing so there's less work needed when promoting a field. |
I think @aloisdeniel refers to using the conditional to check the value for not being null. currently in Dart you need to conditionally check a variable or use the ! notation. i like the solution that @aloisdeniel proposed and swift already implements a lot |
Zig like if expression test for null: if (/* this. */ amount != null) |amount| {
return execute(amount);
} |
The in-progress pattern matching proposal has two features that when combined should help a lot of cases like this:
Combining those two features would let you write: if (this.amount case var amount?) {
return execute(amount);
} (Technically, you could even omit the if (amount case var amount?) {
return execute(amount);
} You still have to say the name twice, once to refer to the field and once to refer to the name of the new variable being bound, but it's otherwise pretty terse. |
This looks quite confusing to me, gotta be honest, especially the approach omitting |
The proposal to support patterns is definitely a very powerful bundle of features, so it's not a surprise that many things can be expressed using patterns. However, patterns wouldn't serve the same purpose as the mechanism proposed here (that is, A pattern is capable of introducing new variables, and it is capable of initializing those variables (e.g., by calling getters). So we would get the same effect using a pattern as we would get from the initialization of a shadow variable. However, that's only half of the semantics of shadow variables, and we won't get the other half. A shadow variable is designed to shadow something, and this notion is supported by implicitly generating code to write back the new value whenever there is an assignment to the shadow variable, and to report a compile-time error at every assignment to the shadow variable if the shadowed entity doesn't support assignments (in other words, a shadow of a final variable is a final local variable). It is not reasonable to use the same implicit write-back semantics for a pattern, because it cannot be assumed in the general case that it is correct to perform this write-back operation. For instance, we could certainly use a pattern to declare a local variable It's simply wrong to assume that every local variable is intended to shadow any given variable which is used to initialize it, and that's the reason why I think we would need an explicit keyword like This is good for correctness because we will never get the write-backs unless we have explicitly asked for them by writing |
Oops, I accidentally left off a if (this.amount case var amount?) {
return execute(amount);
} Is that clearer? |
Now that patterns have been fully implemented for a while, I think it'd be good to revisit this issue and decide whether it's worth pursuing.
class C {
num n;
C(this.n);
bool get condition1 => n < 0xFF;
bool get condition2 => n < 0x0F;
void foo() {
shadow n;
if (n is! int) return;
if (condition1) n &= 0x0F;
if (condition2) n &= 0xF0;
}
} In this use case, having a However: The current Dart 3 features work beautifully under slightly different circumstances. class C {
final Iterable n;
C(this.n);
bool get condition1 => n.isNotEmpty;
bool get condition2 => n.length >= 2;
void foo() {
if (n case List<int> intList) {
if (condition1) intList.add(0xFF);
if (condition2) intList[1] &= 0x0F;
}
}
} If a goal can be accomplished via mutation rather than reassignment, then My opinion is that we now have an abundance of ways to parse objects and handle different types. There are scenarios where the ability to shadow a non-local variable would be very helpful, but many (perhaps all) of these cases can be handled with pattern matching, or just by adjusting the class structure. |
@nate-thegrate AFAIK this feature proposal exists primarily to ease the issues around type-promotion and null-checks since the release of NNBD. Can you use pattern matching to solve the canonical use case here? int? i;
void f() {
if (i == null) return;
print(i.isEven); // Compiler Error
}
|
@esDotDev this is a good point: I didn't mention nullable types at all, but they're another viable use case for shadowing. Having a way to promote a non-local variable within the scope of a function would be awesome: n &= 0x0F; // with type promotion
n = (n as int) & 0x0F; // without type promotion, gross But when compared to the clunkiness of type casting, adding a single null check character isn't that bad in my opinion. print(i.isEven); // with type promotion
print(i!.isEven); // without type promotion If the function needs to reassign a nonlocal variable (i.e. But if you just need to read a value, or if updating the value can be done with a method rather than reassignment, then you can accomplish it with pattern matching: int? i;
void f() {
if (this.i case int i) {
print(i.isEven);
// more non-nullable code can go here
}
} Depending on what My opinion is that we aren't missing out on much without the
That being said, if the Dart language implemented shadowing, I would happily use it in several places. |
Yup: int? i;
void f() {
if (i case var i?) {
print(i.isEven);
}
} Now that patterns are out, I find myself using this idiom quite often. |
If we get something like "guard-let" (#2537) then I wouldn't have any need for shadowing. My main use case for shadowing is if I need to do multiple operations on the variable and my dislike for over-nesting. current: int? i;
void f() {
if (i case var i?) {
print(i.isEven);
print(i / 2);
print(i / 4);
}
} proposed: int? i;
void f() {
shadow i;
if (i == null) return;
print(i.isEven);
print(i / 2);
print(i / 4);
} guard (my preference, probably): int? i;
void f() {
guard let i = i? else {
return;
}
print(i.isEven);
print(i / 2);
print(i / 4);
} |
@Reprevise The best code from the three you showed was the first. For the second one, we can already do it without int? i;
void f() {
final i = this.i;
if (i == null) return;
print(i.isEven);
print(i / 2);
print(i / 4);
} There's no advantage to |
@mateusfccp I am well aware that it's possible without shadow as it would just desugar to a non-final form anyway. My preference for 3 just comes from my dislike of over-nesting, especially when there are multiple |
I'm confused how this is checking for non-null. This makes sense Is there anything in the docs that discusses this sort of usage? [Edit] nm, I found it https://dart.dev/language/pattern-types#null-check |
It's cool that you can utilize pattern matching and a dedicated null-check pattern to do this, but it really feels like a step backwards in readability. It's not semantically intuitive and additionally will be quite hard to developers to lookup implementation details on When you compare it to the dedicated keyword there is no comparison in terms of readability:
It's semantically intuitive and also very easy for a developer to lookup what |
But if we had local variable declarations, that could also be: if ((var i = this.i) != null) {
// Use local i
} That can be used for many more things, where It won't do write-through like |
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:
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;
andshadow 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 nameshadow
, but that would be really rare anyway because user-written type names in Dart are almost universally Capitalized), then we could consider a syntax likevar this.n;
.The text was updated successfully, but these errors were encountered: