Skip to content
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

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

Closed
lsh opened this issue May 3, 2023 · 63 comments
Closed

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

lsh opened this issue May 3, 2023 · 63 comments
Labels
enhancement New feature or request mojo Issues that are related to mojo mojo-lang Tag for all issues related to language. mojo-repo Tag all issues with this label

Comments

@lsh
Copy link
Contributor

lsh commented May 3, 2023

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
Copy link
Contributor

nmsmith commented May 3, 2023

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
Copy link

Mogball commented May 4, 2023

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
Copy link

boxed commented May 5, 2023

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
Copy link
Contributor Author

lsh commented May 5, 2023

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
Copy link

boxed commented May 5, 2023

@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
Copy link
Contributor Author

lsh commented May 5, 2023

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)

@goldiegadde goldiegadde added enhancement New feature or request mojo-external labels May 6, 2023
@nmsmith
Copy link
Contributor

nmsmith commented May 6, 2023

@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:

  • Some programming languages, such as Elm and Zig, don't support variable shadowing at all. IMO, that might be a bit too extreme.
  • C# and D don't allow local variables to shadow each other, but allow other kinds of shadowing.

@lsh
Copy link
Contributor Author

lsh commented May 6, 2023

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

@boxed
Copy link

boxed commented May 6, 2023

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
Copy link

nevdelap commented May 6, 2023

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
Copy link

boxed commented May 6, 2023

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
Copy link

nevdelap commented May 6, 2023

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

@Mogball
Copy link

Mogball commented May 6, 2023

@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
Copy link

elisbyberi commented May 6, 2023

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
Copy link

Mogball commented May 6, 2023

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
Copy link

elisbyberi commented May 6, 2023

@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
Copy link

Mogball commented May 6, 2023

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
Copy link

boxed commented May 6, 2023

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
Copy link

@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
Copy link

Mogball commented May 6, 2023

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
Copy link

elisbyberi commented May 6, 2023

@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
Copy link

Mogball commented May 6, 2023

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
Copy link

@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
Copy link

Mogball commented May 6, 2023

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

@elisbyberi
Copy link

elisbyberi commented May 6, 2023

@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
Copy link
Contributor

nmsmith commented May 7, 2023

@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
Copy link

BreytMN commented May 7, 2023

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
Copy link
Contributor

nmsmith commented May 7, 2023

@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.

@Philip-S-Martin
Copy link

Philip-S-Martin commented Sep 22, 2023

Is there a reason that shadowing needs to be conflated with the type of function you're using? I want to clean up scope in some cases without having to wrap things in a fn.

I'd much rather scope everything like python by default unless I explicitly say otherwise. This would:

  • make def functions more portable to fn functions
  • allow us to clean up scope in top-level code and def functions without having to refactor to fn.
  • avoid creating inconsistent behavior between def and fn

You resolve this by adding a new keyword like shadow to enable shadowing on any scope.

Here's an example with a for loop:

let a = 1
shadow for i in range(0,3):
    let a = i + 1
    print(a)
print(a)

Prints:

1
2
3
1

Don't allow this to compile. Shadowing should be explicit, since it differs from standard python.

let a = 1
for i in range(0,10): # lacks explicit shadow keyword 
    let a = i + 1
    print(a)
print(a)

This would allow consistent behavior on for/with/try/except and any other scopeable context. Hell, you could even just have a raw shadow context that provides nothing else.

let a = 1
shadow with open('file.txt', 'rb') as file:
    let a = 3
    print(a)
print(a)

shadow try:
    let a = 4
    print(a)
    buggy_call()
shadow except:
    let a = 5
    print(a)
print(a)

shadow:
    let a = 6
    print(a)
print(a)

Prints:

3
1
4
5
1
6
1

I'd love this for being able to manage scope more cleanly in notebooks and def functions.

JoeLoser pushed a commit to JoeLoser/mojo that referenced this issue Mar 28, 2024
…2290)

This continues to remove the pop operations from the dtype checks
without getting recursive elaborator errors

E.g

```
no work left, no deferred search, and no recursion?
UNREACHABLE executed at /Users/abduld/code/modular/KGEN/lib/Elaborator/Elaborator.cpp:2703!
PLEASE submit a bug report to [Internal Link] and include the crash backtrace.
 #0 0x0000000102c4ca08 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/abduld/code/modular/.derived/build-release/bin/mojo-prime-package-cache+0x1000c4a08)
 #1 0x0000000102c4ab68 llvm::sys::RunSignalHandlers() (/Users/abduld/code/modular/.derived/build-release/bin/mojo-prime-package-cache+0x1000c2b68)
 modularml#2 0x0000000102c4d0a8 SignalHandler(int) (/Users/abduld/code/modular/.derived/build-release/bin/mojo-prime-package-cache+0x1000c50a8)
 modularml#3 0x00000001880e02a4 (/usr/lib/system/libsystem_platform.dylib+0x1803fc2a4)
 modularml#4 0x00000001880b1cec (/usr/lib/system/libsystem_pthread.dylib+0x1803cdcec)
 modularml#5 0x0000000187feb2c8 (/usr/lib/system/libsystem_c.dylib+0x1803072c8)
 modularml#6 0x0000000102be03a0 llvm::install_out_of_memory_new_handler() (/Users/abduld/code/modular/.derived/build-release/bin/mojo-prime-package-cache+0x1000583a0)
```

modular-orig-commit: ebb7c424801291198503fde0ebdd6fd28da8f2e9
@ematejska ematejska added the mojo-repo Tag all issues with this label label Apr 29, 2024
modularbot pushed a commit that referenced this issue May 29, 2024
This PR introduces nondeterminism into the testsuite. `test_dict.mojo`
nondeterministically fails with

```
[M] ➜  modular git:(1853f9d3e9) mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
Test test_basic ...PASS
Test test_multiple_resizes ...PASS
Test test_big_dict ...PASS
Test test_compact ...PASS
Test test_compact_with_elements ...PASS
Test test_pop_default ...PASS
Test test_key_error ...PASS
Test test_iter ...PASS
Test test_iter_keys ...PASS
Test test_iter_values ...PASS
Test test_iter_values_mut ...PASS
Test test_iter_items ...PASS
Test test_dict_copy ...PASS
Test test_dict_copy_add_new_item ...PASS
Test test_dict_copy_delete_original ...PASS
Test test_dict_copy_calls_copy_constructor ...PASS
Test test_dict_update_nominal ...PASS
Test test_dict_update_empty_origin ...PASS
Test test_dict_update_empty_new ...PASS
Test test_mojo_issue_1729 ...PASS
Test test dict or ...PASS
Test test dict popteim ...get: wrong variant type
Please submit a bug report to https://github.com/modularml/mojo/issues and include the crash backtrace along with all the relevant source codes.
Stack dump:
0.      Program arguments: mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
 #0 0x00000001043a10b0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c90b0)
 #1 0x000000010439f210 llvm::sys::RunSignalHandlers() (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c7210)
 #2 0x00000001043a1750 SignalHandler(int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c9750)
 #3 0x00000001ab1b2a24 (/usr/lib/system/libsystem_platform.dylib+0x18042ea24)
 #4 0xffff8002a81b8510
 #5 0x00000001047c1608 M::KGEN::ExecutionEngine::runProgram(llvm::StringRef, llvm::StringRef, llvm::function_ref<M::ErrorOrSuccess (void*)>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1004e9608)
 #6 0x00000001042f8270 executeMain(mlir::ModuleOp, mlir::SymbolTable const&, M::KGEN::ExecutionEngine*, M::LLCL::Runtime&, llvm::ArrayRef<char const*>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x100020270)
 #7 0x00000001042f7cb8 run(M::State const&) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x10001fcb8)
 #8 0x00000001042df774 main (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x100007774)
 #9 0x00000001aae2bf28
[1]    44318 trace trap  mojo
```

MODULAR_ORIG_COMMIT_REV_ID: ee1c665669902106df680fe6c6d2599897665ff5
modularbot pushed a commit that referenced this issue Jun 7, 2024
This PR introduces nondeterminism into the testsuite. `test_dict.mojo`
nondeterministically fails with

```
[M] ➜  modular git:(1853f9d3e9) mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
Test test_basic ...PASS
Test test_multiple_resizes ...PASS
Test test_big_dict ...PASS
Test test_compact ...PASS
Test test_compact_with_elements ...PASS
Test test_pop_default ...PASS
Test test_key_error ...PASS
Test test_iter ...PASS
Test test_iter_keys ...PASS
Test test_iter_values ...PASS
Test test_iter_values_mut ...PASS
Test test_iter_items ...PASS
Test test_dict_copy ...PASS
Test test_dict_copy_add_new_item ...PASS
Test test_dict_copy_delete_original ...PASS
Test test_dict_copy_calls_copy_constructor ...PASS
Test test_dict_update_nominal ...PASS
Test test_dict_update_empty_origin ...PASS
Test test_dict_update_empty_new ...PASS
Test test_mojo_issue_1729 ...PASS
Test test dict or ...PASS
Test test dict popteim ...get: wrong variant type
Please submit a bug report to https://github.com/modularml/mojo/issues and include the crash backtrace along with all the relevant source codes.
Stack dump:
0.      Program arguments: mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
 #0 0x00000001043a10b0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c90b0)
 #1 0x000000010439f210 llvm::sys::RunSignalHandlers() (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c7210)
 #2 0x00000001043a1750 SignalHandler(int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1000c9750)
 #3 0x00000001ab1b2a24 (/usr/lib/system/libsystem_platform.dylib+0x18042ea24)
 #4 0xffff8002a81b8510
 #5 0x00000001047c1608 M::KGEN::ExecutionEngine::runProgram(llvm::StringRef, llvm::StringRef, llvm::function_ref<M::ErrorOrSuccess (void*)>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x1004e9608)
 #6 0x00000001042f8270 executeMain(mlir::ModuleOp, mlir::SymbolTable const&, M::KGEN::ExecutionEngine*, M::LLCL::Runtime&, llvm::ArrayRef<char const*>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x100020270)
 #7 0x00000001042f7cb8 run(M::State const&) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x10001fcb8)
 #8 0x00000001042df774 main (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo+0x100007774)
 #9 0x00000001aae2bf28
[1]    44318 trace trap  mojo
```

MODULAR_ORIG_COMMIT_REV_ID: ee1c665669902106df680fe6c6d2599897665ff5
@lattner
Copy link
Collaborator

lattner commented Nov 14, 2024

Let bindings have been removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mojo Issues that are related to mojo mojo-lang Tag for all issues related to language. mojo-repo Tag all issues with this label
Projects
None yet
Development

No branches or pull requests