dart-lang / language

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

Allow shadowing local final variables #3322

Open knopp opened 1 year ago

knopp commented 1 year ago

With Dart 3.0 it is now more viable than ever to write functional-style code. However the lack of support for shadowing local variable makes it problematic to write code like

final value = SomeThing();
final value = transform(value);
final value = otherTransform(value);

Traditionally shadowing local variable in same scope may seem a bit contentious, but it is used in functional languages or even Rust. Right now after every transform it is necessary to come up with a separate name, with all previous values being in scope, which adds some friction and can be error-prone:

final value = SomeThing();
final valueTransformed = transform(value);
final valueTransformed2 = otherTransform(valueTransformed);

// ...

valueTransformed.doSomething(); // should have been valueTransformed2

I think it would be acceptable if this was only possible for final bindings. Shadowing rebindable variables could still be an error.

EDIT: Possibly a lint or an error could be introduced where the shadowing variable would need to reference the original variable in the initializer:

final value = getBytes();
final value = sanitize(value); // works
final value = decode(value); // works

final value = b; // fails

This would would make it possible to use shadowing during transformations, but would prevent redefining the variable by accident.

rrousselGit commented 1 year ago

Extensions are an alternative

We can write:

final value = SomeThing()
  .transform()
  .otherTransform();
jakemac53 commented 1 year ago

Or just don't make the variable final? What is the value of shadowing versus a mutable variable?

Imo allowing this would just make it easy to accidentally shadow final variables that weren't meant to change (and they would effectively now be changed in the rest of the body).

Note also that for most (but not all) uses of shadowing you can still access the shadowed variable (ie: this. or some_prefix.). These shadowed local variables would effectively disappear.

knopp commented 1 year ago

Or just don't make the variable final? What is the value of shadowing versus a mutable variable?

Shadowing is a completely different thing thing than rebindable declaration. Var will only allow rebinding with object of same type. With shadowing every transformation can result in object of different type. And each declaration is final and deliberate so you won't rebind it by accident.

Imo allowing this would just make it easy to accidentally shadow final variables that weren't meant to change (and they would effectively now be changed in the rest of the body).

They wouldn't change. They just wouldn't be accessible. I've seen this argument many times, but having used languages that permit this, such as Elixir and Rust, I can't remember a single instance where that would be an actual problem in practice.

Note also that for most (but not all) uses of shadowing you can still access the shadowed variable (ie: this. or some_prefix.). These shadowed local variables would effectively disappear.

Indeed. That's part of the pattern and often a desirable thing. In the example above there is no reason to access the original value or the one after first transformation steps. More often than not that would happen by accident and be a bug.

jakemac53 commented 1 year ago

With shadowing every transformation can result in object of different type.

The variable could be typed Object, so it is assignable by anything, but fair. I would just rename it though.

And each declaration is final and deliberate so you won't rebind it by accident.

But shadowing is almost the same effect (I get that it is different, but it can lead to the same problems as accidental rebinding). To me this removes much of the value of final for local variables.

In general I think shadowing is bad though, except in very narrow cases (such as shadowing an instance variable with a local one just to get field promotion, but even that can lead to issues). It is generally very error prone. So expanding it from my perspective is not a good thing.

They wouldn't change. They just wouldn't be accessible.

Effectively, they have changed. All references later on now refer to a different value (sure it's an entirely different variable, but that is splitting hairs imo, the effect when reading the code is the same).

If I see final x = 'y'; in a function, I can today make a very easy assumption about all references to x in that function. That is the value final provides, in fact I would say its entire purpose. Allowing shadowing breaks that assumption.

I have fixed bugs in the past that were a direct result of shadowing (in particular, local function parameters can shadow local variables which gets extremely confusing).

More often than not that would happen by accident and be a bug.

I am not sure how you would accidentally use a variable with a totally different type? If they have the same type just make it mutable. Or just use a transformation style like what @rrousselGit suggested via extensions or top level methods if you want to avoid polluting the namespace with variables that are only used once.

knopp commented 1 year ago

With shadowing every transformation can result in object of different type.

The variable could be typed Object, so it is assignable by anything, but fair. I would just rename it though.

And each declaration is final and deliberate so you won't rebind it by accident.

But shadowing is almost the same effect (I get that it is different, but it can lead to the same problems as accidental rebinding). To me this removes much of the value of final for local variables.

In general I think shadowing is bad though, except in very narrow cases (such as shadowing an instance variable with a local one just to get field promotion, but even that can lead to issues). It is generally very error prone. So expanding it from my perspective is not a good thing.

Does this come from a practical experience? I'm asking because that's exactly what my assumption was until I've actually used languages that permit this and I can easily say that for me the ability to write functional style code with multiple shadowing transformations is much more useful than knowing that a final variable can never be shadowed.

They wouldn't change. They just wouldn't be accessible.

Effectively, they have changed. All references later on now refer to a different value (sure it's an entirely different variable, but that is splitting hairs imo, the effect when reading the code is the same).

If I see final x = 'y'; in a function, I can today make a very easy assumption about all references to x in that function. That is the value final provides, in fact I would say its entire purpose. Allowing shadowing breaks that assumption.

I have fixed bugs in the past that were a direct result of shadowing (in particular, local function parameters can shadow local variables which gets extremely confusing).

That is confusing, but arguably more confusing and error prone than shadowing local variable.

More often than not that would happen by accident and be a bug.

I am not sure how you would accidentally use a variable with a totally different type? If they have the same type just make it mutable. Or just use a transformation style like what @rrousselGit suggested via extensions or top level methods if you want to avoid polluting the namespace with variables that are only used once.

Using extensions is much more limiting than shadowing variables, especially after adding destructuring and switch expressions.

knopp commented 1 year ago

Extensions are an alternative

We can write:

final value = SomeThing()
  .transform()
  .otherTransform();

To a degree. This does not really play nicely with destructuring or switch expressions.

mateusfccp commented 1 year ago

An alternative would be #1246. Although not the same thing, I think they achieve the same?

final value = SomeThing()
            ▷ transform
            ▷ otherTransform;
knopp commented 1 year ago

FWIW, rust clippy has a clippy::shadow_unrelated lint, that warns when shadowing an unrelated variable. So for example

let data = get_data();
let data = base64::decode(data);

is fine while

let data = get_data();
let data = c;

prints a warning.

mateusfccp commented 1 year ago

FWIW, rust clippy has a clippy::shadow_unrelated lint, that warns when shadowing an unrelated variable. So for example

That's very nice and would basically solve all potentials issues regarding shadowing.

munificent commented 1 year ago

Shadowing makes a lot of sense in Rust because the language also has ownership semantics, basically a form of linear typing. In Rust, you can have functions that consume a value (which means the variable it's bound to can no longer be used). To play nice with that, it's idiomatic for functions to consume values and return new ones, instead of mutating values in place.

When you have a lot of functions that consume values and produce new ones that represent the "same thing" after some transformation, it gets hard to keep coming up with new names for the thing. And, also, you can't use the untransformed one anymore either, since the borrow checker has told the compiler the old version is now dead.

Given that, it makes a lot of sense for the language to allow you to shadow the previous variable with another of the same name. But none of that applies to Dart. There is no ownership system or linear types. Mutation is common and idiomatic. It's relatively rare for you to need multiple names for the same thing because most APIs let you just update the actual thing itself.

Shadowing can lead to confusing code where a reader might not realize that a closure is accessing a different variable than one later in the block:

test() {
  var x = 'before';

  closure() {
    print(x);
  }

  var x = 'shadow';

  x = 'assigned';
  closure(); // Prints 'before'.
}

You restrict shadowing to only final variables to avoid this, like the proposal suggests. But that would mean that changing a variable from final to non-final might require you to also rename it to prevent shadowing.

Overall, I just don't think it's a net improvement in usability to allow shadowing within the same scope.

knopp commented 1 year ago

Mutation is common and idiomatic. It's relatively rare for you to need multiple names for the same thing because most APIs let you just update the actual thing itself.

Quite a bit of my code consists of immutable classes. Quite a bit of Flutter API is immutable. Immutable objects don't seems to be an uncommon paradigm.

When you have a lot of functions that consume values and produce new ones that represent the "same thing" after some transformation, it gets hard to keep coming up with new names for the thing.

If I wasn't experiencing this with Dart i wouldn't have opened the issue.

And, also, you can't use the untransformed one anymore either, since the borrow checker has told the compiler the old version is now dead.

It may be dead in Rust, but it's not dead in elixir. I don't see borrow checker or linear types being preconditions to be able to shadow local variables.

Consider a hypothetical example:

final value = getBytes();
final value = sanitize(value);
final value = decode(value);

Currently I need to write it like this:

final value = getBytes();
final valueSanitized = sanitize(value);
final valueDecoded = decode(valueSanitized); 

It's very easy to call decode(value) instead, which might be a pretty serious bug. That can not happen with the first version, as unsanitized value, while still existing, can not be referenced anymore.

hamsbrar commented 1 year ago

Shadowing help if I can't think of better names. This is the only thing that it solves. It has well known problems and all of them exists in Elixir. They're OK with them because Erlang's variables are invariable and immutable. They stripped of the invariable part because that was easy to strip and that's what their shadowing is. There's nothing functional about it. It tries to justify issues related with shadowing as "Well we've at least less problems than Erlang" which by the way is subjective because Erlang's problems are different and no-shadowing isn't one of them, and this statement cannot be used to justify why it's OK to give problems to users who don't even want shadowing.

Enabling shadowing on final variables will defeat the purpose of making a variable final because users can easily introduce a rebinding even in the presence of the lint suggested in this thread.

Enabling shadowing on var variables is a good idea. These are variables that can exhibit this behavior without an issue. But I think there are many users using var to mean final because final is too damn difficult to type so keep them in mind.

Or Make shadowing explicit using a sigil or a modifier like recyclable/modifiable. Allow variable rec var a = 1;(or mod var a = 1;) to be recycled(redeclared) exactly once and make it an error if it's not recycled. Allow users to use rec modifier on redeclarations so they can keep recycling a single variable as long as they want. It takes away simplicity from assignments but only for users who want shadowing or whoever reads their code. It's better. No lints needed No need to change scoping rules which is something a language should change once in a centuary.

In general, shadowing is helpful when I want to get things done fast but it also trips me up regularly because there's no way to disable it. Slapping problematic features on every single user is what most languages like to do. Erlang is one of the few languages in which you get both immutability and no-shadowing(it has function scope vars but there are warnings for them too). Erlang was never a popular one and it never will be one but it knows that shadowing gets in your way as much as it helps so it should be either explicit or completely disallowed.

knopp commented 1 year ago

Shadowing help if I can't think of better names. This is the only thing that it solves.

Solving naming is not small thing. But it's also not the only problem. With every transformation state being in scope it is easy to reference wrong state later in code by accident.

Enabling shadowing on final variables will defeat the purpose of making a variable final because users can easily introduce a rebinding even in the presence of the lint suggested in this thread.

Even with shadowing you can't rebind final variable, only declare a new one. With the lint above you could only redeclare final variable when the initializer references previous variable with same name. That should be a pretty robust solution for redeclaring variable by accident.

Enabling shadowing on var variables is a good idea. These are variables that can exhibit this behavior without an issue. But I think there are many users using var to mean final because final is too damn difficult to type so keep them in mind.

var variables can be rebound at any time later in code. That's not desired outcome.

hamsbrar commented 1 year ago

Solving naming is not small thing.

I don't underestimate the problem so I should rephrase it: Shadowing doesn't solve anything. All it does is 'You can't think of names? use the same ones but at your own risk'.

With every transformation state being in scope it is easy to reference wrong state later in code by accident.

Having 'Alicek03ros2c' and 'Alice' in the same room is better than having two 'Alice' then. You'll never remember the one you don't want to talk to. (I know shadowing help write your example clearly but having it everywhere implicitily is problematic)

Even with shadowing you can't rebind final variable, only declare a new one.

Redeclaring a final variable isn't much different from rebinding it.

final v = 1;
// ... some code
useV1(v);

And later:

final v = 1;
// ...

// someone else added code(they didn't realise that there's a 'v' below)
final v = plus(v);        // int
final v = plusPlus(v);    // int
useV2(v);                 // int

// ...
useV1(v); // now v is messed up

var variables can be rebound at any time later in code. That's not desired outcome.

That's what you're asking for and some more.

benthillerkus commented 1 year ago

Shadowing doesn't solve anything. All it does is 'You can't think of names? use the same ones but at your own risk'.

I like to use shadowing to keep my scope of accessible variables small. IE make variables I don't need anymore effectively inaccessible. This in combination with simpler names makes code easier to understand, not harder.

Redeclaring a final variable isn't much different from rebinding it.

As already mentioned it's entirely different, because you can have different types while maintaining type safety + your IDE can understand that this is actually a different variable.

Shadowing help if I can't think of better names

"There are only two hard problems in Computer Science: …"

hamsbrar commented 1 year ago

A child might find balancing on a bicycle as a "hard problem". There's nothing wrong with the child; rather, it's a design limitation in bicycles that everyone has to overcome with practice. You can add extra training wheels to mitigate the problem but making them permanent on every bicycle is problematic. It make bicycles inflexible and slow, especially for people who don't need them.

Explicit shadowing(as roughly described in this thread) doesn't solve the problem because it won't help you come up with better names but at least it isn't taken from someone's book. It's mine and it's better because it not only mitigate the problem but also ensure that training wheels aren't permanent. I can extend it to functions, classes and every single thing where it's difficult to name but I lack the motivation to do that.

"There are only two hard problems in Computer Science: …"

I don't read, enough. I just look for motives behind the first force before Newton see a force, action and reaction. If you've a motive, then you'll see a "prime mover" aka "unmoved mover" that'll either eliminate the problem or die trying. And if it turns out that the problem is inherent in your design, then you must be willing to accept radical changes to your design or they'll label you as the only problem in this universe.

Saying 'X is hard' is easy and shows that "My only dream is to become hopeless".

hamsbrar commented 1 year ago

No doubt, training wheels is exactly what I need every now and then and they help in cases where one actually need them(like the example in this issue). In dynamic languages, one can go without them easily but that's a different topic. I've no issue if language decides to support them or not(I'm going to vote +1 because it's a valid request whether I like it or not), and all the different arguments appears to be valid from the person's prespective who is making them. I need to focus at work so going to unsub, apologies if I don't reply back.


I'd like to briefly mention that if language is going to enable this in future(today or after ten years), then something worth considering is to enable shadowing on var variables and then allow those variables to be redeclared as final. ```dart // #1 --------------------------- final value = getBytes(); final value = sanitize(value); // ERROR, final isn't allowed // #2 --------------------------- var value = getBytes(); var value = sanitize(value); // ALLOWED, 'value' is shadowed // #3 --------------------------- var value = getBytes(); var value = sanitize(value); final value = decode(value); // 'value' is shadowed as final value = 1; // ERROR var value = ddecode(value); // ERROR final value = ddecode(value); // ERROR ``` It's slightly less-problematic than shadowing final variables.
eernstg commented 5 months ago

We didn't get a new pipe operator (as proposed in https://github.com/dart-lang/language/issues/1246), but something like the following might suffice, especially in a codebase where the given construct (here: ~(...)) is well known:

// ----- Pipelines.

extension<X1, X2> on (X1, X2 Function(X1)) {
  X2 operator ~() => $2($1);
}

extension<X1, X2, X3> on (X1, X2 Function(X1), X3 Function(X2)) {
  X3 operator ~() => $3($2($1));
}

// And so on up to tuples with k components for some k.

// ----- Example.

String transform(int i) => '$i';
bool otherTransform(String s) => s.length < 3;

void main() {
  final value = ~(
    10,
    transform,
    otherTransform,
  );
  print(value); // 'true'.
}

This doesn't allow for interleaving with other things:

// Assuming that the proposal of this issue is accepted.
void main() {
  final value = 10;
  // ... lots of stuff.
  final value = transform(value);
  // ... lots of stuff.
  final value = otherTransform(value);
}

However, too much interleaved stuff would also make it hard to read, which would presumably exacerbate the dangers associated with shadowing.

benthillerkus commented 5 months ago

exacerbate the dangers

Typically after each transformation my value is of a different type, so I'm prevented at compile time from confusing my local variable with one of the shadowed ones of the same name.