[JuliaLowering] Refactor scope resolution pass

[mlechu]

Fixes

Soft scope

We currently treat flisp's '(block (softscope true)) as a scope that "is
soft", but this isn't correct. It should behave more like a toggle, where if the
scope surrounding (softscope true) is the top-level thunk, neutral scopes not
protected by a hard scope become permeable to already-defined globals. I've
added K"softscope" for this.

Fixes JuliaLang/JuliaLowering.jl#101.
JuliaLowering.activate!() should be much more usable in the REPL now, as
globals won't be accidentally eaten by the "soft scope."

Shadowing behaviour

Found while cleaning up the lhs-resolution step with the change above. This PR
allows static parameters to be shadowed by globals and locals as long as they're
explicit (and not in the same scope). flisp allows this with globals, and the
explicit locals that desugar to local-def forms (which JuliaLowering doesn't
have).

This change is more permissive than flisp in the local case, since after looking
into why shadowing was disallowed I realized it was just was just to prevent
assignment to the static parameter (#32623). The flisp fix leads to some funny
behaviour:

julia> function f(x::T) where T
     let
         global T # remove this T and the other two will fight
         let; local T; end
     end
     end
f (generic function with 1 method)

Bindings/LambdaBindings

In figuring out how to use the variable bookkeeping system, I ran into inaccuracies.

LambdaBindings is currently a per-lambda map from unique variable to four
flags: captured, read, assigned, and called. I think (but correct me if
I'm wrong @c42f) this was a misinterpretation of the holy text: in flisp
lowering, a local variable captured by some other lambda does show up in both
lambdas' variable lists, but is the same underlying object, and flag mutations
on one variable are seen by all lambdas.

I tried to think of other reasons for tracking vinfo per lambda within lowering,
but if we're doing something about the capture-boxing issue, we need something
more complex anyway.

This PR moves all vinfo to BindingInfo and deletes the incorrect bookkeeping
in LambdaBindings. We still need to have a per-lambda flag for capturedness
(different from the variable-level capt vinfo flag). With the added flags,
I've just made BindingInfo mutable since our previous workflow (BindingId is an
index into a vector; mutate the vector) doesn't give us the benefits of
immutability anyway.

Enhancements

• Scopes are retained until the end of the pass, so consumers like JETLS can
  answer questions like "what names are available at my cursor?" Recreating
  this previously-discarded information was a bit hacky! [1] [2]
• Hopefully enough tests and explanatory comments to make up for the large diff

TODO

• Our use of K"local" in desugaring is dubious in some places
• I've added expr_compat_mode to the scope analysis context, but we still need
  to implement flisp hygiene exemptions for globals (see note in test/scopes.jl).
• Reviewing the IR changes, the #self# argument still has extra flags set in
  some cases. This is an existing desugaring bug with a comment that took me
  too long to find: we shouldnn't be using the same #self# binding for
  multiple methods defined by one function body

[topolarity]

I still need to do another once-over of the core pass logic, but this should be useful (mostly very minor) feedback in the mean time.

[topolarity]

Another micro-optimization, but Vector{IdTag} should probably be BitSet
(since these are small and contiguous)

[topolarity]

Is this identical to the sequence of scopes you get if you follow the parent(ctx, scope) chain?

[mlechu]

(discussed in meeting) I'm mirroring our handling of macro scope layers but agree doing parent would be cleaner.

[topolarity]

Seems like the right approach (and indeed closer to flisp) to update shared bindings across all lambdas simultaneously.

Changes are looking good to me. Nice work @mlechu!

Feel free to take or drop the nits / ideas, as desired.

[c42f]

I think we already test these cases more comprehensively in scopes_ir.jl (it's more comprehensive because we also test the error messages there)

So I think we can remove these. Generally my rule of thumb has been to avoid @test_throws LoweringError because the IR tests give a convenient way of asserting that the error message has the correct provenance.

Side note - I think it would be nice to bring our IR tests and eval() tests closer together with a bit of test infrastructure refactoring at some point. The aim being twofold:

• Reduce confusion and duplication in cases like this
• Reduce duplication for cases where we want to both test the structure of the IR and the value that it eval()'s to.

[c42f]

Maybe it would also help if we simply added a bit more structure to the IR tests. Eg, allow structuring the IR test files so that we can group the content into test sets?

[mlechu]

So I think we can remove these.

It feels like more effort to selectively delete cases from an "every combination" testset than to just have the full matrix there.

Side note - I think it would be nice to bring our IR tests and eval() tests closer together with a bit of test infrastructure refactoring at some point

The split we have now is nice in that IR tests don't need to run (no setting up globals) and eval() tests don't need to be limited to readable amounts of IR. My main gripe with our tests isn't duplication, it's that we need to write more of them :)

[c42f]

My main gripe with our tests isn't duplication, it's that we need to write more of them :)

Haha! It's the bane of any new project when switching from "this is going fast and everything is changing all the time" to "oh this about to be production code".

Having done several big new development projects, there's always this transition period, and it's critical to the longer term success. So I really appreciate your efforts - and likewise I'll try to do my best to beef up tests whenever I see any missing.

If I find a way to make the tests "more fun" to write I'll also put in the effort there. I am happy that we have IR tests at all - in contrast to existing lowering. They're still a bit clunky to work with - perhaps we may yet find a way to improve that.

[c42f]

Eek. I can't recall right now whether I intentionally tried to depart from the weird flisp behavior or not. seems potentially breaking to be different (even though it's consistent) - may need to be changed via syntax evolution I guess.

[mlechu]

Eek indeed. I'd be willing to call it a julia bug given how strange it would be to call it part of the language, but we can see what pkgeval says. I've seen someone ask if decl keywords do something to an existing variable, which is confusion to prevent.

[c42f]

Sounds reasonable.

[c42f]

LambdaBindings is currently a per-lambda map from unique variable to four flags: captured, read, assigned, and called. I think (but correct me if I'm wrong @c42f) this was a misinterpretation of the holy text: in flisp lowering, a local variable captured by some other lambda does show up in both lambdas' variable lists, but is the same underlying object, and flag mutations on one variable are seen by all lambdas.

LambdaBindings is information about "how is a binding used by the current lambda", as opposed to "how is this binding used by all lambdas".

This distinction can be important as soon as we start work on unboxing captures, so yes I did this on purpose and at first glance I don't like removing this infrastructure - even though it's clearly incomplete at this stage.

This PR moves all vinfo to BindingInfo and deletes the incorrect bookkeeping

Can you point me to examples of the inaccuracies? Obviously if they affect semantics of evaluation this is a problem to fix!

But if they're internal differences you should expect that we'll be diverging a lot from flisp once we properly start on unboxing.

[c42f]

As another more concrete example of why LambdaBindings is useful, consider the is_called flag.

This is metadata which applies to how argument slots are used within the body of the CodeInfo - it eventually gets mapped into the CodeInfo's called field and used within gf.c for specialization heuristics. These heuristics shouldn't care about the use of a function argument in other lambdas it happens to be captured into, but merely about its direct use in the body of statements. I would argue that flisp is wrong if it tracks this information globally. To make this concrete:

function f(g)
    function h()
        g()  # should not set `is_called` on `g`'s slot flag in `f`
    end
end

You're certainly right to notice a disconnect with flisp - because I didn't actually port the flisp implementation of vinfo closely, but instead referred to the runtime C code's actual usage of the metadata which is being produced by lowering as the source of truth.

[topolarity]

These heuristics shouldn't care about the use of a function argument in other lambdas it happens to be captured into, but merely about its direct use in the body of statements.

I don't know if I agree with that - Calling h() inside of f() would suffer (it will not infer well) if g is not marked as called. The heuristic could try to chain this reasoning explicitly by checking whether h is called before propagating the flag, but we really do need to match whatever flisp does for specialization behavior to be the same.

[mlechu]

This distinction can be important as soon as we start work on unboxing captures

I'll elaborate on my claim "we need something more complex anyway". Almost every lambda-vinfo-flags-based unboxing proof I can think of requires other machinery complex enough to make the flags unnecessary.

Right now (ignoring flisp's lambda-optimize-vars, which we don't do yet) lowering makes boxes assuming nothing about (1) what parts of the function (including inner functions) are executed or (2) what order they're executed in. vinfo per lambda lambda seem to give us the former until we start trying to use them---when is an inner lambda actually called? We could call the defining expression, or assign it to a global, or pass it as an argument to map, or only call it after a dominating assignment.

The one case the flags give us anything new is when the inner lambda is never called, but proving this in anything other than a trivial case requires basic-block-level or maybe even statement-level analysis. But with basic-block-level analysis, why are we tracking one "assume everything executes" flag per inner lambda in the first place?

[c42f]

Calling h() inside of f() would suffer (it will not infer well) if g is not marked as called

Hmm. I was convinced it would be the job of the optimizer to deal with inlining the implementation of h, after which the call of g() would be self-evident... the same behavior as if h were a manually defined closure:

struct _h_type{G}
    g::G
end

function (h::_h_type)()
   h.g()
end

function f(g)
    h = _h_type(g)
    h()   # lowering cannot know that g is called
end

But perhaps I'm mistaken and this is a case where the compiler can use the fact that h is a closure to do better than the hand written version?

We also have the following example where the heuristic guesses wrong because the closure isn't called. But I'll concede it's a less likely case and it's high probability that any closure will be called during the lifetime of the enclosing function.

function f(g)
    function h()
        g()
    end
    return h
end

[c42f]

Basically - I admit I thought this was an outright bug which I was fixing but it's not actually simple 😅

[c42f]

I'll elaborate on my claim "we need something more complex anyway". Almost every lambda-vinfo-flags-based unboxing proof I can think of requires other machinery complex enough to make the flags unnecessary.

I agree that information global to the body of a lambda isn't very powerful. It can help, but the cases it helps with are rather trivial - and I don't think very useful. We need some simple data flow analysis to do unboxig as best we can within lowering. And "as best we can" is not very powerful because our escape analysis will be extremely pessimistic without information from the optimizer.

Ok, so I'm a bit happier with deleting this stuff for now, assuming that we may need to completely refactor it in the next step anyway. Would still like to know whether the is_called heuristic's behavior with respect to captures was intended or a mere side effect of the way vinfo happens to work currently.

[topolarity]

I'm going to merge this as-is

I am a fan of @c42f 's ambitions to improve the specialization heuristics / tighten up the flags here. Nonetheless, I think we'll want to save that for "Phase 2" of the JuliaLowering initiative, after we have already gotten parity with flisp 👍

This is definitely an improvement in terms of bugs / parity, so let's get it in!

[c42f]

Nonetheless, I think we'll want to save that for "Phase 2" of the JuliaLowering initiative, after we have already gotten parity with flisp

I think that's pretty fair in this case - even if it's a bug (I'm now unsure about this) - keeping consistency with flisp is reasonable because changing specialization could have subtle side effects and need its own review.

Side note - there's several other probable-bugs which I came up against in working through the flisp code for the lowering port as a whole. I mostly chose to fix them when they seemed obviously buggy so we'll be able to close a few existing issues once the integration work is done. Obviously we could reimplement the bugs but that's a bit depressing so hopefully we'll be able to get away with just leaving them fixed 😅