Fix decls in value position without assignment to return nothing

[c42f]

Fix #50

[mlechu]

I think we should try and match the flisp behaviour where no-assignment decls are still not allowed in value position in general (a user could reasonably expect y = global x to do the assignment), but they're OK as a top-level statement.

As for local outside of local scope, I think we're OK to disallow it. No one has implemented the error in flisp yet, but everyone I've spoken to wants it. ref JuliaLang/julia#57483, cc @JeffBezanson

[c42f]

Oh yes indeed, this PR doesn't seem like the correct fix.

So. We currently have some weird behaviors:

julia> local x

julia> begin
           local x
       end  # returns nothing

julia> begin
           x = 10
           local x
       end # also returns nothing :sweat_smile: 

julia> global z  # ok

julia> begin
           global z # looks like a statement, but is an error
       end
ERROR: syntax: misplaced "global" declaration
Stacktrace:
 [1] top-level scope
   @ REPL[36]:1

I think this occurs because global x has special logic in jl_toplevel_eval_flex (jl_needs_lowering) and flisp toplevel-only-expr? which means global x never goes through the normal IR generation.

julia> Meta.@lower global x
:(global x)

julia> Meta.@lower local x
:($(Expr(:thunk, CodeInfo(
    @ none within `top-level scope`
1 ─     Core.NewvarNode(:(x))
└──     return nothing
))))

To be honest, I don't love this about the existing system and I'm not 100% sure what to do about it. There's a certain sense in not lowering some constructs but it's also a fairly weird oddity that certain expressions never get turned into IR and are instead interpreted with their own special set of rules (which might diverge from IR generation!)

I guess we can fix the error message just by detecting unadorned global x up front in desugaring and wrapping it in a block which returns nothing.

[mlechu]

detecting unadorned global x up front in desugaring and wrapping it in a block which returns nothing

Sounds like an OK patch to me; I think this is what ends up happening in flisp with global x::Type, which doesn't go through the cursed special jl_toplevel_eval_flex logic.

Also ref JuliaLang/julia#58279, which would improve all of this, but I believe requires a bunch of packages to be updated.

[c42f]

We could probably adopt the unused-only mechanism from JuliaLang/julia#58279 without the other lowering changes, for now. It seems like that change allows global in tail position but not in value position which is an interesting intermediate choice.

[xal-0]

RE Exprs that bypass lowering: I'd really like to eliminate the special expression heads in lower-toplevel-expr--. We've allowed some weird expressions that cannot be produced by the parser to have "useful" effects when evaled. It would be nice to be stricter about passing lowered syntax to eval to avoid any more of these. These two appear in the Julia test suite (unsure if they were found by PkgEval):

julia> # Relic from when "const" was a flag on normal bindings
       Core.eval(Main, Expr(:const, :x))

julia> # It is now an error to assign to x without const
       x = 1
ERROR: invalid assignment to constant Main.x. This redefinition may be permitted using the `const` keyword.
Stacktrace:
 [1] top-level scope
   @ REPL[2]:2

julia> const x = 2
2

julia> module M end
Main.M

julia> # Declare globals from the wrong module
       Core.eval(Main, :(global $(GlobalRef(M, :x))))

julia> GlobalRef(M, :x).binding
Binding Main.M.x
   38492:∞ - weak global binding declared using `global` (implicit type Any)
   0:38491 - undefined binding - guard entry

[c42f]

I'd really like to eliminate the special expression heads in lower-toplevel-expr--

Yes! It's problematic to have the rules for validity of top level expressions scattered in various places in the runtime.

I've noticed that you've been progressively turning the special forms in IR into calls to runtime functions and I'm very happy about that.

In case you didn't notice, this is something I've also been factoring out in JuliaLowering - if you look in src/runtime.jl you'll see there's various calls to Core.eval() - eg in eval_module() - because that's currently the only way we can access the appropriate C code. But ideally we'd replace such calls with a ccall or a call to a function in Base.

[c42f]

Ok, I've revised this significantly.

For now, I've somewhat followed the logic in JuliaLang/julia#58279 by allowing global declarations in tail position but not value position, inspired by the unused-only mechanism. Additionally, I've only allowed it inside top level thunks not function scope so the "value" of global x is only observable via eval(). (@xal-0 I think this is more restrictive than 58279 but achieves the same desired outcomes?)

@mlechu I've also tackled the "local x outside a scope" problem by making it an error to use it at top level when completely outside any construct which could serve as a scope. I did this before macro expansion as I feel it's the only case where it's almost completely useless syntax (the right hand side of an assignment to the local could still have side effects ... hence only "almost" completely useless 😅 )

See tests for the current cases which are considered errors.

I suspect that local x being allowed in value position is a bug in existing lowering. But I'm not sure we can fix that without breaking code. Perhaps another candidate to consider for syntax evolution.

[c42f]

And here I was, yesterday, thinking "oh this is a cute and simple little bug I'll fix it in half an hour". haha!

[mlechu]

Looks great, thanks for fixing this.

Effects on the RHS is a good catch. I'd take the triage decision to mean that we should make it an error anyway to prevent confusion, but I'm also OK leaving potentially-intentionally breaking stuff until after we've fixed our real bugs so as to not mix pkgeval failures. (Or I could make the change in flisp and try it there first.)

I'll start keeping a list of possibly-breaking syntax improvements like "try to make local non-assigning declaration throw an error in value position" and "try to make uncontained local assigning declaration at top level throw an error".

[mlechu]

Good error message

[c42f]

Thanks!

In a lot of these cases it could be useful to know "why are we in value position?" as well, so we could say "the value is used here" (eg, in an assignment) - see #8. But for now this will do I guess :)

[mlechu]

I'm OK moving this to desugaring, as a macro that does nothing but declaring an unenclosed local escaped into the enclosing scope is probably being misused at top level anyway.

[c42f]

I just feel it's more possible to generate trivial code from a macro without it necessarily being a user error or an error by the macro writer. So on balance I think I'm preferring it where it is.