odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.64k stars 583 forks source link

Success of evaluating a global `when` statements depends on file order #4239

Open zen3ger opened 2 weeks ago

zen3ger commented 2 weeks ago

Context

odin report:

    Odin:    dev-2024-09:244a4acfa
    OS:      openSUSE Tumbleweed, Linux 6.10.7-1-default
    CPU:     AMD Ryzen 9 5900X 12-Core Processor            
    RAM:     31999 MiB
    Backend: LLVM 18.1.8

If multiple files in a package have when statements in the global scope where one depends on the other, depending on the filename of the dependent file the when may or may not fail.

Expected Behavior

Package global evaluation of when statements is independent of file order in a package.

Current Behavior

When the dependent file is type checked after the dependee the when statement will successfully be evaluated. If it's done previously then the declaration will not be in scope at the time of type checking the file, resulting in failure.

Failure Information (for bugs)

/tmp/scratch/l.odin(3:6) Error: Undeclared name: FOO
    when FOO == .A { 
         ^~^ 

/tmp/scratch/l.odin(3:14) Error: Invalid type 'invalid type' for implicit selector expression '.A' 
    when FOO == .A { 
                 ^ 

Steps to Reproduce

Failure case:

// main.odin
package test

Foo :: enum { A, B }

when ODIN_OS == .Linux {
    FOO :: Foo.A
} else {
    FOO :: Foo.B
}

main :: proc() { b: Bar }

// -------------------------------------------
// l.odin
package test

when FOO == .A {
    Bar :: struct { x: int }
} else {
    Bar :: struct { x,y: f32 }
}

To successfully compile the code, rename l.odin to n.odin (or any name that would be alphabetically after main.odin).

zen3ger commented 2 weeks ago

Stack trace right before we hit the incorrect scope_lookup() call. Note, line numbers in some places are probably off as I had some trace prints to see where and when things get added to the scopes...

#0  check_ident (c=0x7fffffffca28, o=0x7fffffffc500, n=0x7fffee6378d0, named_type=0x0, type_hint=0x0, allow_import_name=false) at src/check_expr.cpp:1743
#1  0x0000555555623a36 in check_expr_base_internal (c=0x7fffffffca28, o=0x7fffffffc500, node=0x7fffee6378d0, type_hint=0x0) at src/check_expr.cpp:10989
#2  0x000055555562302d in check_expr_base (c=0x7fffffffca28, o=0x7fffffffc500, node=0x7fffee6378d0, type_hint=0x0) at src/check_expr.cpp:11350
#3  0x000055555563198d in check_expr_or_type (c=0x7fffffffca28, o=0x7fffffffc500, e=0x7fffee6378d0, type_hint=0x0) at src/check_expr.cpp:11433
#4  0x000055555563450a in check_binary_expr (c=0x7fffffffca28, x=0x7fffffffc500, node=0x7fffee637a20, type_hint=0x0, use_lhs_as_type_hint=true) at src/check_expr.cpp:3907
#5  0x000055555562534a in check_expr_base_internal (c=0x7fffffffca28, o=0x7fffffffc500, node=0x7fffee637a20, type_hint=0x0) at src/check_expr.cpp:11184
#6  0x000055555562302d in check_expr_base (c=0x7fffffffca28, o=0x7fffffffc500, node=0x7fffee637a20, type_hint=0x0) at src/check_expr.cpp:11350
#7  0x0000555555622ef9 in check_multi_expr (c=0x7fffffffca28, o=0x7fffffffc500, e=0x7fffee637a20) at src/check_expr.cpp:11384
#8  0x0000555555622eb5 in check_expr (c=0x7fffffffca28, o=0x7fffffffc500, e=0x7fffee637a20) at src/check_expr.cpp:11427
#9  0x00005555556fdc23 in collect_when_stmt_from_file (ctx=0x7fffffffca28, ws=0x7fffee638128) at src/checker.cpp:5159
#10 0x00005555556fd0b9 in collect_file_decl (ctx=0x7fffffffca28, decl=0x7fffee6380f0) at src/checker.cpp:5264
#11 0x00005555556fcb8e in collect_file_decls (ctx=0x7fffffffca28, decls=...) at src/checker.cpp:5303
#12 0x000055555560b203 in check_import_entities (c=0x7fffee6319b0) at src/checker.cpp:5508
#13 0x0000555555599159 in check_parsed_files (c=0x7fffee6319b0) at src/checker.cpp:6498
#14 0x0000555555581e7a in main (arg_count=5, arg_ptr=0x7fffffffe138) at src/main.cpp:3333
flysand7 commented 2 weeks ago

The compilation is deterministic, so it is not random. All of the files in the project are type checked in topological order. Within a single package, the files are sorted alphabetically, according to unicode rune ordering.

From what I received from @gingerBill on this topic a few months ago, was that this is the expected behavior, and if you're relying on the alphabetical sorting of the files, you may be doing something "weird".

In case you need it, there is a workaround. The following expression, if substituted will work.

FOO :: Foo.A when ODIN_OS == .Linux else Foo.B

The order of type checks depending on the ordering of files is expected behavior that works as intended. It's not causing bugs, because the file order in question is not platform-dependent, so it wouldn't break on one vs another machine. I suggest closing this issue

zen3ger commented 2 weeks ago

Why I understand the reasoning, it's quite unexpected considering you can define procs in whatever order and whatever files.

The issue is not about whether it's random or not and afaik nobody depended on specific order to make things work, quite the opposite it popped up as it didn't work and we had no clue why.

I'm guessing that delayed evaluation of when could be done similarly to how procedure bodies are delayed. It also counts as a delayed declaration when collected, it's just that all delayed whens computed at the end of processing a file and not the whole package.

omnisci3nce commented 2 weeks ago

It is confusing that when you use

FOO :: Foo.A

at the top level, when statements in all files work, but when you swap to the following

when ODIN_OS == .Linux {
    FOO :: Foo.A
} else {
    FOO :: Foo.B
}

it suddenly stops working in those other files saying that FOO is undeclared. I think it should at least have a more descriptive error.