modularml / mojo

The Mojo Programming Language
https://docs.modular.com/mojo/manual/
Other
22.8k stars 2.57k forks source link

[Feature Request] Consider Allow Shadowing of `let` Bindings #5

Open lsh opened 1 year ago

lsh commented 1 year ago

In Rust, shadowing of let bindings are allowed. I have found this feature relatively nice to have. One example is casting data types:

fn my_fn(i: F32):
    let i = I32(i)
    print(i)

edit: def -> fn to make clear this is about stricter semantics

nmsmith commented 1 year ago

Personally I'm not a fan of arbitrary variable shadowing — there's been at least two occasions in my life where I've had a bug that was caused by accidentally shadowing a local variable within a large function.

One compromise that can be made is to allow variable shadowing only when the RHS of the declaration references the variable that is being shadowed. For example, the following would be allowed:

let x = 0
...
let x = x + 1

But the following would not:

let x = 0
...
let x = y + 1

The first example is far less likely to be a source of bugs, because the person writing that line of code surely realizes that the variable they are defining is one that already exists. If Mojo adds variable shadowing, my personal preference would be to only allow shadowing of this kind.

(This restriction is supported by Rust's linter (Clippy). It's called shadow_unrelated.)

Mogball commented 1 year ago

because the person writing that line of code surely realizes that the variable they are defining is one that already exists

I definitely do not, because I make this mistake all the time in C++ get a compilation error.

boxed commented 1 year ago

I can report that Mojo today does have shadowing. Run this code in the playground:

def your_function(a, b):
    let c = a
    # Uncomment to see an error:
    # c = b  # error: c is immutable
    let d = 3.4

    if c != b:
        let d = b
        print(d)
    if c != b:
        let d = 'foo'
        print(d)
    print(d)

your_function(2, 3)

it prints

3
foo
3.400000

I think this is a mistake. Shadowing like this is not useful for anything, and make code harder to reason about. It also is different from how Python works.

Rebinding like this is a good idea though:

def foo():
     let a = 4
     let a = a + 5
lsh commented 1 year ago

I'm fine with the behavior you have in your example code, and don't find it harder to reason about. The shadowing I was asking for was in the same scope (as you mention in your final example). Seeing let as a showing variable means it is inherently not hoisting it to the higher scope (which is expected in most languages that allow this such as JavaScript). If one wanted d to change in the example above they would use var.

boxed commented 1 year ago

@lsh Shadowing in nested scopes doesn't really have a point other to make the code confusing. There's no upside and plenty of downside to variable names in a single function jumping around to pointing to different underlying variables.

Maybe C did it to make it easy write #define style macros or something, but I think we've moved on from those days now :P

I'm fine with the behavior you have in your example code, and don't find it harder to reason about.

I'm not 100% sure you understood what my first example shows. Did you see that d first means one thing, then another, than YET another, then back to the first thing? The "back to the first thing" is also implicit. With more lines of code in between this is gonna be hard to keep in your head what the scope of everything is.

The problem of scopes being hard to keep track of is why C++ code bases often has mFoo for "member variable foo" everywhere. We don't want a language where the second d in my example is properly written as nestedOneD for "variable called d but nested one level" :P

lsh commented 1 year ago

I did understand. In fact, you could have added another print between the if statements and the output of 3.4 would have still made sense. I tend to prefer expression oriented code either way, so I probably would have written that function like:

def your_function(a, b):
    c = a
    d = b if c != b else 3.4
    print(d)
    d = 'foo' if c != b else d
    print(d)

your_function(2, 3)

I also think the C++ mFoo probably is much more related to implicit assignment of members (as in when people do not use this->mFoo = foo)

nmsmith commented 1 year ago

@lattner Is it intentional that the Mojo compiler currently permits shadowing from within a nested scope, but not from within the same scope? i.e. is it intentional that the following is allowed:

let x = 1
if True:
    let x = 2

But the following is not:

let x = 1
let x = 2

If anything, I'd expect the opposite policy.

One approach that Mojo could take is to disallow all variable shadowing by default, and then if/when we identify coding patterns where variable shadowing is helpful for writing clean code, we could have the Mojo compiler accept just those particular patterns. This is probably a solution that everyone would be satisfied with.

In practice, I expect that most instances of shadowing follow the pattern I described in my earlier post, wherein a variable is "updated" by re-binding it, e.g. let x = x.unwrap() — possibly changing its type. So that's a pattern we could consider enabling in the short term.

Prior art:

lsh commented 1 year ago

The nested scope is at least intentional, as it is listed as a changelog feature.

boxed commented 1 year ago

I find Elms no rebinding to be annoying, but it goes with the language being a big graph and not going from top to bottom. But mojo and python ARE executed top to bottom so then rebinding makes much more sense.

nevdelap commented 1 year ago

Rebinding at the same scope is a wonderful thing (not hiding/shadowing, but replacing) when you are transforming something that changes types but is still conceptually the same thing and so should keep the same name. Being forced to give it a new name after each transformation is really not nice.

boxed commented 1 year ago

Even if the type is the same, which I would expect being the common case, it's a good thing. It's slightly less easy to reason about variables if they can be rebinded than not, but rebinding is easier to reason about than full mutability.

nevdelap commented 1 year ago

I agree. In Rust I also like that you can rebind to make something temporality mutable and then immutable again.

Mogball commented 1 year ago

@lattner Is it intentional that the Mojo compiler currently permits shadowing from within a nested scope, but not from within the same scope? i.e. is it intentional that the following is allowed:

let x = 1
if True:
    let x = 2

But the following is not:

let x = 1
let x = 2

If anything, I'd expect the opposite policy.

One approach that Mojo could take is to disallow all variable shadowing by default, and then if/when we identify coding patterns where variable shadowing is helpful for writing clean code, we could have the Mojo compiler accept just those particular patterns. This is probably a solution that everyone would be satisfied with.

This shadowing behaviour is intentional. We need a clear principle to variable shadowing, and not just allow it "in a small number of specific patterns/cases that make code cleaner".

elisbyberi commented 1 year ago

let should be immutable in its scope only, and possibly not visible outside its block scope. If we shadow it in its scope, it would not make sense to use let, use var instead.

However, it wouldn't be clear what the block scope is in this case.:

def func(): # function scope
    let d = b if c != b else 3.4 # function scope, or block scope?

vs

def func(): # function scope
    var d = 0
    if c != b: # block scope
        d = b
    else:
        d = 3.4
Mogball commented 1 year ago

let should be immutable in its scope only, and possibly not visible outside its block scope. If we shadow it in its scope, it would not make sense to use let, use var instead.

However, it wouldn't be clear what the block scope is in this case.:

def func(): # function scope
    let d = b if c != b else 3.4 # function scope, or block scope?

This is function scope. x if c else d is an expression.

elisbyberi commented 1 year ago

@Mogball Yes, it's a conditional expression (function scope), which is equivalent to a block of if/else statements (lexical scope). It does not make sense. That's not how Python works; it's not compatible with Python.

@boxed noted that shadowing a variable makes no sense in Python. That would make Python code incompatible in Mojo. It would be better to disallow var and let keywords, as well as lexical scoping, in Python code.

def func(a, b, c):
    d = a if a < b else b if b < c else c
    print(d)

func(2, 2, 1)

Is the same as (automatic transformation from conditional expressions into if/else statements):

def func(a, b, c):
    if a < b:
        d = a
    else:
        if b < c:
            d = b
        else:
            d = c
    print(d)

func(2, 2, 1)

It would be better not to mix Python syntax with Mojo syntax.

Mogball commented 1 year ago

The code you have shown will work in Mojo. In Mojo, implicitly declared variables are function-scoped, and implicitly declared variables are only allowed inside def functions. Explicitly declared variables with var and let inside both fn and def functions are allowed to be shadowed.

fn foo(c: Bool):
  let d = 5 if c else 6 # 'd' is declared at the function scope, which is the same as:
  let e: Int # lazily initialized 'let' declaration
  if c:
    e = 5
  else:
    e = 6
boxed commented 1 year ago

Having two different scoping rules seems like a bad thing.

What is the rationale for the non-python shadowing and scoping rules? What are the practical benefits?

elisbyberi commented 1 year ago

@Mogball Today, we often find ourselves programming in multiple languages. We all know that it's not easy to switch between programming languages. For example, after a long week of programming in Java or JavaScript, I sometimes find myself terminating Python statements with a semicolon.

Mixing Python with Mojo syntax is like speaking in different natural languages at the same time. It's a state of mind.

For example, this code will not work in Mojo (correct me if I am wrong):

fn foo(c: Bool):
  if c:
    let e = 5
  else:
    let e = 6
  print(e) # varable e doesn't exist in function scope.

That's not how we think in Python. We have to switch our mindset from Python mode to Mojo mode.

Mogball commented 1 year ago

We don't have multiple scoping rules for different "kinds" of variables, but rather it's where we choose to place implicitly declared variables. The rule is that implicitly declared variables are scoped/declared at the function level, and so are visible in all lexical scopes in the function. The practical benefit is that writing long and complex functions with lots of variables and nesting tends to be more tractable. It's the same benefit as allowing let declarations to shadow within the same scope.

The rule is that "if it looks like Python, it behaves like Python" (mostly). However, let and var are not Python, and so don't necessarily have to follow "Pythonic" rules about scoping. @elisbyberi You are correct in saying that the code above will not compile, but Mojo is not always confined to the Python way of doing things. There are many features in Mojo that are not Pythonic.

There is a more practical limitation around definitive initialization of variables. For instance, in Python the following code will throw a NameError if k is equal to 0.

def foo(k):
  for i in range(k): pass
  print(i)

Therefore, whether i is initialized at the print function call is a dynamic property of the function. Mojo requires definitive initialization, so this code cannot be allowed: i will be scoped to the for loop lexical block. (This behaviour is subject to change if we decide we really want to match the Python behaviour).

elisbyberi commented 1 year ago

@Mogball If you declare the variable i within the function scope, it will not be useful for the algorithm's logic. This is because you are printing the value of the variable i, which is never initialized. In this case, it would be appropriate to raise a "NotInitialized" exception. Printing a value when range() never generates a single value is a logical error. You are trying to print a value from an empty list, which doesn't make sense.

Python requires us to be explicit, so we have to handle the case when k is equal to 0.

def foo(k):
    if not k: return  # case when k == 0

    i: int  # declare variable i, but do not initialize it
    for i in range(k): pass
    print(i)

if not foo(0):
    print("Nothing happened!")
Mogball commented 1 year ago

As a statically compiled language with initialization guarantees, Mojo will never perfectly match the behaviour of Python when it comes to dynamic initialization. In addition, the code you showed will not compile because the Mojo compiler isn't going to perform the complex dataflow analysis and code transformation to ensure i is definitively initialized once, and then destructed+reinitialized in subsequent loop iterations. And it will never compile unless we change the rules around definitive initialization because it's too much effort.

The goal of Mojo is not to be exactly like Python.

elisbyberi commented 1 year ago

@Mogball I cannot see how this is going to help us:

def foo(k):
    i: int = 0
    for i in range(k): pass
    print(i)

if not foo(0):
    print("Nothing happened!")

We are shooting ourselves in the foot.

Mogball commented 1 year ago

Seems fine to me. It's a common pattern in C++ as other languages with "malleable" for loop semantics.

elisbyberi commented 1 year ago

@Mogball Just for the record, the code has a logical error. The case when k is equal to 0 must be handled.

def foo(k):
    i: int = 0
    for i in range(k): pass
    if k: print(i) # do not print anything if range() never generates a value

foo(0)
nmsmith commented 1 year ago

@Mogball I agree that the semantics for shadowing needs to be principled. There are at least two steps involved in justifying a semantics:

  1. Establish a need (e.g. readability) that can be fulfilled by shadowing.
  2. Find a principled semantics for shadowing that fulfils that need.

We should begin by considering shadowing as it is currently implemented. Currently, Mojo allows local variables to shadow each other, but only if they are defined at a different level of nesting. The semantics is clear, but what is the need that it addresses?

Perhaps we should take this as an opportunity to reset the discussion and understand shadowing from the ground up.

BreytMN commented 1 year ago

The issue goes beyond the shadowing rules in Mojo being differente from Python. If i uncomment the first line the code will run fine, but in Mojo it will give the following error:

# %%python
def your_function(a, b):
    c = a

    if c != b:
        d = b
        print(d)
    if c != b:
        d = 'foo'
        print(d)
    print(d)

your_function(2, 3)
error: Expression [17]:14:11: use of unknown declaration 'd'
    print(d)
          ^

Wasn't it supposed to be a superset of Python?

nmsmith commented 1 year ago

@BreytMN As you say, that has nothing to do with shadowing. Perhaps it is worth starting a different thread on the topic of variable initialization in defs.

lsh commented 1 year ago

To be clear, I opened this ticket with the intention of behavior within an fn context. I believe within a def things should work as always, but fn has stricter rules about initialization.

BreytMN commented 1 year ago

@BreytMN As you say, that has nothing to do with shadowing. Perhaps it is worth starting a different thread on the topic of variable initialization in defs.

Yes, it isn't apparently directly related to shadowing question raised here but it actually is totally related to it if you add the stubbornness shown here of not adhering to all Python rules.

The promise was to solve the "two-language approach" in Jeremy Howard words for deploying fast models in Python... But shadowing won't work like Python because variable initialization at all won't work like Python.

In other words: it is trying to solve the "two-language problem" with another "two-language problem". So, if it is supposed to be a superset, it should be a superset, not an intersection with Python.

Of course, I won't dictate how you guys should shape the language you envisioned, but the promises aren't aligning. If this continue, I predict many more issues following this reasoning will be raised.

nmsmith commented 1 year ago

I understand your disappointment that your Python program doesn't currently compile in Mojo, but at this point, we now have two completely different conversations occurring in this thread. The first conversation is about variable shadowing of let bindings in fns (neither of these exist in Python, and thus they are allowed to have a unique semantics), and the second conversation is about a Python program (a def) not compiling in Mojo. I recommend opening a different Github issue for the latter conversation.

elisbyberi commented 1 year ago

@nmsmith The OP was talking about def, not fn.

def my_fn(i: F32):
    let i = I32(i)
    print(i)
lsh commented 1 year ago

You're right, that was my mistake

nmsmith commented 1 year ago

Whether we use def or fn is orthogonal to my claim that there are two unrelated conversations happening.

My understanding is that the original focus of this thread was about the shadowing behaviour of let bindings. The semantics of let bindings should obviously be the same regardless of whether they appear at the top level, in a def, or in a fn. The alternative would be nightmarish.

let bindings are new to Mojo, so it's valuable to have a discussion about their semantics.

But half of this thread is talking about the semantics of Python-style bindings, not let bindings. An especially pertinent issue that has been raised is that certain Python programs (that use Python-style bindings, obviously) do not currently compile in Mojo. That's a different conversation, deserving of its own thread.

elisbyberi commented 1 year ago

@nmsmith They are related by usage of let and var keywords. Anyway, it was good to clear things up for def too. Mojo has both its own def and the fn keyword, while Python only has def. So, there are two types of def, and a fn.

boxed commented 1 year ago

The discussion since my message has little concrete substance :(

I ask again: what are the practical use of shadowing? Can someone show a case where shadowing leads to better and more readable code than without?

Scoping is a different matter imo. I understand that controlling the lifetime of objects is important for tight performance goals.

lsh commented 1 year ago

Off the top of my head, unwrapping values is incredibly common. Pattern matching with an optional type will be allowed, and in languages like Rust, it is common to see

if let Some(value) = value {
  // do_something()
}

Otherwise, code could be littered with optional as a prefix or postfix on optional types, when the type should be just as descriptive.

lsh commented 1 year ago

Actually, thinking more on this, I can imagine ergonomics issues erupting without shadowing and with pattern matching. If one has a variable name, and a field in a type being matched against also has that name, they would either have to rebind the variable, or the field within the pattern match.

Another example are parser combinators. It is very common to return some parsed data, and the rest of the data as a slice. Usually it would look something like:

name, rest = parse_string("name state year_of_birth")
state, rest = parse_state(rest)
year_of_birth, rest = parse_date(rest)
assert(len(rest) == 0)

What should rest be named in each intermittent case?

boxed commented 1 year ago

@lsh hmm. Maybe (pun intended). In Swift that syntax is different in a way where you could allow a much narrower case of shadowing for this unwrap without adding the entire foot gun of free shadowing to the language 🤷‍♂️

lsh commented 1 year ago

I also challenge that this is the foot gun you claim it is. Your example above does not resemble real production code, so it is hard to reason about

boxed commented 1 year ago

Well.. the rest cases in your example is rebinding, currently disallowed, not shadowing. Shadowing to me means there's a hidden stack of variables that pop back into relevance. In the case of rest in your case that's not the case.

The foot gun is when a name jumps from one variable to another. It's the restore from the hidden stack that I'm arguing against.

lsh commented 1 year ago

Fair enough, I still challenge you to show a real example where that's a problem.

nmsmith commented 1 year ago

@lsh Surely you're not questioning whether variable shadowing, in general, is a source of bugs. Many people have reported experiencing such bugs during their my programming career, myself included. The last time I encountered a shadowing bug was a few months ago, where I had a large function with several scopes going on. I can't reconstruct the exact scenario here — I'd have to go find the code I was working on. Shadowing bugs almost always occur in large programs. That's why it's hard to give examples.

The creators of several well-known programming languages such as C# and D have banned shadowing of local variables precisely because of their experience with the kinds of bugs that shadowing can cause.

Update:

Here's the testimony of Eric Lippert, one of the designers of C#:

...this really does address a real source of bugs. I have myself accidentally-and-not-on-purpose introduced a local variable in one scope that shadows another and it took me literally hours of debugging to discover why it was that what I thought was one variable was suddenly changing values halfway through a function.

And here's the testimony (from March this year) of Walter Bright, one of the designers of D:

I added shadowing protection to D a couple decades ago. It has saved me from numerous difficult-to-find bugs, especially if the function gets a bit large. It's a big win.

elisbyberi commented 1 year ago

@lsh Out of curiosity, would changing the data type of the i parameter in the "my_fn" function be similar to what you're suggesting(?) This would affect callers of the function, and if shadowing is allowed, it would also impact the functions called by "my_fn". Having this feature when defining new fn would be useful, but I don't believe it would be helpful in an existing codebase.

I believe this is the issue at hand. Please correct me if I'm wrong.

I typically use a struct to hold all the parameters (emulating object-oriented programming) when I code in C. Handling all the data type changes in the control-flow graph of the function can be a never-ending task.

boxed commented 1 year ago

I have trouble recalling the shadowing bugs I've had... because I've done python for 12 years 😉

lsh commented 1 year ago

@nmsmith I will take you at your word that in your circles this is a common concern. I can only vaguely remember this biting someone once in the codebases I've worked on and the communities I participate in. I do not mean to minimize your concern, just adding another point to the data and perspectives. I don't follow C# or D, or their creators, so I'm not particularly sure what to make of those statements. I will say that several languages which have been listed as direct precedents (and have much more similar type systems) do have shadowing, such as Swift, Rust, and Scala.

@boxed Similarly, Python does not have shadowing, so I'm not sure what to make of your comment (e: ah I get it, can't have bugs for a feature if you don't have the feature). One more point I would like to note is that normal Python semantics can also cause bugs. What if in the nested scope of those methods I did not want to mutate d in the outer scope, but accidentally reused a name? This has also bitten me in the past, and shadowing would have protected against the bug in that case.

@elisbyberi After talking with Anders in the thread, I think the feature I cared more about in that case was rebinding.

Either way, I don't want to spend too much more time in this thread, so I will probably bow out. I will say, Mojo currently supports shadowing and that was intended. Also, any proposed solution will need to address patterns if the plan would be to remove it. If the Modular team is convinced by any comments in this thread (or internally) that allowing it is a mistake, they can and should remove it. Otherwise, it will remain as is, and they can close this thread and potentially come back to it another day.

elliotwaite commented 1 year ago

@lsh, you mentioned potentially closing this issue, but your original feature request to add support for rebinding or shadowing within the same scope still seems to be relevant.

Based on the discussion above, it appears that several people have expressed openness to this idea.

@nmsmith:

One compromise that can be made is to allow variable shadowing only when the RHS of the declaration references the variable that is being shadowed. For example, the following would be allowed:

let x = 0
...
let x = x + 1

@boxed:

Rebinding like this is a good idea though:

def foo():
     let a = 4
     let a = a + 5

@nevdelap:

Rebinding at the same scope is a wonderful thing (not hiding/shadowing, but replacing) when you are transforming something that changes types but is still conceptually the same thing and so should keep the same name. Being forced to give it a new name after each transformation is really not nice.

I personally have also found this type of rebinding to be convenient. I got used to it while learning Rust. It's commonly used in the Rust community, probably because it's one of the unique features of Rust that newcomers learn about early on. It's described in Chapter 3 of the Rust book, where they give the following example of its usefulness, which is similar to what @nevdelap described (note: in the book they refer to it as shadowing):

Shadowing is different from marking a variable as mut because we’ll get a compile-time error if we accidentally try to reassign to this variable without using the let keyword. By using let, we can perform a few transformations on a value but have the variable be immutable after those transformations have been completed.

The other difference between mut and shadowing is that because we’re effectively creating a new variable when we use the let keyword again, we can change the type of the value but reuse the same name. For example, say our program asks a user to show how many spaces they want between some text by inputting space characters, and then we want to store that input as a number:

    let spaces = "   ";
    let spaces = spaces.len();

The first spaces variable is a string type and the second spaces variable is a number type. Shadowing thus spares us from having to come up with different names, such as spaces_str and spaces_num; instead, we can reuse the simpler spaces name. However, if we try to use mut for this, as shown here, we’ll get a compile-time error:

    let mut spaces = "   ";
    spaces = spaces.len();

The error says we’re not allowed to mutate a variable’s type ...

Now, just because Rust supports it, doesn't necessarily mean Mojo should, but Rust is generally considered to be a well-liked and safe language, so I think that does bear some weight.

That said, there's been a lot of discussions above and I may have missed some of the points that were made, so please correct me if I'm wrong about this still being a feature worth considering.

nmsmith commented 1 year ago

Okay, let's roll with the term "rebinding".

(Update: A while after writing this post, I realized that "rebinding" already has an established meaning, related to the phenomenon of name binding. Accordingly, I no longer believe that my usage of the term "rebinding" in the below post is appropriate.)

From what has been discussed so far, it's clear that rebinding is a useful kind of shadowing. In contrast, I'm not convinced that the other kind of shadowing, where there is a stack of meanings for each variable, is useful. I believe that all of the shadowing bugs I have experienced during my career were from stack-based shadowing.

@Mogball says that Mojo needs a clear principle for variable shadowing — not just a set of special cases. So here's a potential principle:

Rationale:

  1. Forcing a rebindable variable to be labelled as a var makes it clear to the reader that its value may change later in the control flow. This gives readers the guarantee that let variables are truly immutable. (In contrast, rebinding a let variable is a kind of "mutability hack" that subverts the purpose of let.)
  2. Using x = .. syntax for rebinding (as opposed to var x = ..) makes it clear that an existing variable is being rebound. It prevents users who are attempting to define a new variable from accidentally rebinding something that already exists.

In short: "rebinding" is nothing more than mutation. The novel part of what I am proposing is that we allow variables to be mutated to a different type.

Some example programs:

var state = foo()
state = update(state)
-----------------------------------
var x = parse(thing)
x = x.unwrap()
-----------------------------------
var items = foo()
items = len(items)
if items > 0:
    ...

Some complexity arises when a value is being conditionally mutated to a different type, for example:

var x: Optional[Thing] = parse(thing)
if foo(x):
    x = x.unwrap()
# What is the type of x?

I doubt that if a user wrote such a program, they would want to access the "potentially unwrapped" x after the if statement, so Mojo could probably make it a compile-time error—similar to the error one would receive when trying to access a moved value. But if Mojo had union types, x could be given the type Optional[Thing] | Thing.

Is this a good solution? Does it meet everyone's needs?

elliotwaite commented 1 year ago

Wouldn't allowing a var to be assigned a value of any type undermine the safety of the type system?

nmsmith commented 1 year ago

Thankfully, no 🙂. The compiler just needs to make note of the type of the variable after each assignment operation. So at one point in the control flow, a variable might have type A, but after an assignment, it might have type B. This is easily accounted for.

The compiler logic would be essentially the same as for shadowing. Remember: when a variable is shadowed, its type is allowed to change.