anno: introduce annotationck phase that initializes modifiers

[soc]

This shows that the approach of initializing the modifiers in the individual *ck-phases might work if it wasn't for the X_accessible_from access-checks:

If the X is later than the phase we already checked, then it fails because is_public is false, because the modifiers haven't been initialized yet. So e. g. class → struct works, but struct → class does not.

[soc]

@dinfuehr ideas?

[soc]

@dinfuehr if there aren't any ideas how to deal with this, I'll try the way of moving the bits and pieces that do outline parsing in *ck phases to globaldef, and add an annotation phase after it.

[dinfuehr]

Hmm, not sure I understand what you mean with that?

[soc]

We already create the "empty" annotation (like class/function/etc.) in globaldef and then right afterwards resolve builtin annotations. Let's pick Class::is_pub as an example. Right now we set that field during globaldef, IIRC what I meant was that we always set it to false in GlobalDef and then during clsdefck iterate the annotations on the class and if we find @pub set it to true here.

So this hits the issue mentioned above. So instead of doing this, the idea is to have an annotation-phase that initializes the modifiers rather early (~after outline parsing and import processing).

But to do this, it first requires moving the creation of the constructors, methods, fields etc. from classdeck, structdefck to globaldefck (where the majority is already created).

[dinfuehr]

I would rather initialize annotations for classes/structs/functions in a separate phase right after globaldef. We can't define e.g. functions/methods in globaldef anyways since not all types exist at that point. That design with globaldef exists for a reason (I didn't start with this design from the start but had to switch to it). I suspect there are more problems/dependencies like this.

[soc]

I would rather initialize annotations for classes/structs/functions in a separate phase right after globaldef

Yes, that's what I had in mind. globaldef is doing outline parsing (parsing declarations/definitions without looking at their bodies), but there are bits and pieces missing from it.

What I'm proposing is completing the missing parts of the outline phase and initializing the annotations in a phase after it.

I was thinking of doing it after import resolution, such that it is easier in the next step to switch from a name-based approach ("any annotation @pub sets the modifier") to working on the resolved symbols ("only @pub annotations referring to std::pub set modifiers").

We can't define e.g. functions/methods in globaldef anyways since not all types exist at that point.

Can you expand on that?

I didn't start with this design from the start but had to switch to it

So you started out with doing all outline parsing in globaldef, and moved things to later phases – or you did all things in later phases first and then moved things to globaldef?

[dinfuehr]

Ah right, fields/methods can have annotations as well. Not sure though, we want to move creation of those into globaldef (it's globaldef and not globaldefck btw - globaldefck is just for global variables. Naming is definitely unfortunate atm). When we create fields/methods all annotations should already be created and all internal ones should be known to the compiler already, so we should just be able to create fields/etc. directly with initialized is_pub/etc.

[dinfuehr]

I like that globaldef only defines top-level elements. I would try to avoid adding fields/methods in globaldef.

I think (without actually looking it up) initially I started by roughly creating functions with argument and return types first (forward declarations were never necessary and Dora only had builtin types). And a second pass that then checked the function body.

When adding classes I just added an additional pass that created classes with field and method types just before the first function pass. I think I only realized a bit later that classes needed forward declarations that way (e.g class Foo(x: Bar) class Bar(x: Foo) didn't work afair). I think this and other new stuff forced me to introduce globaldef.

Type parameters are needed for read_type (we assume that it only returns valid types) to work: in class Foo[T: Bar] we need the trait Bar to be created already. Even more tricky if traits can be generic as well (that's not supported atm), something like class Foo[T: Bar[Foo[Int]]] might be legal.

[soc]

it's globaldef and not globaldefck

Apologies, I always meant to refer to the former.

Not sure though, we want to move creation of those into globaldef

Ok, then I'll move only those created in globaldef required to the new, earlier phase.

I like that globaldef only defines top-level elements.

I believe that having an outline phase (i. e. a phase that enters definitions, regardless of the level) would be more reliable and would avoid having to special-case things (this feature is likely not the last time this problem will come up). Though given the workaround, this is hopefully not something that needs to be addressed before this PR.

[dinfuehr]

it's globaldef and not globaldefck

Apologies, I always meant to refer to the former.

No need to apologize. I just wanted to make sure we are talking about the same thing. Naming is definitely problematic for globaldef and globaldefck.

[soc]

Ok, the earlier phase looks good except that I can't set modifiers for namespaces, because I can't get mutable values from namespaces: Vec<NamespaceData> in VM.

Is this intentional? (What's the general logic whether something is held as GrowableVec<RwLock<X>>, GrowableVec<X>, Vec<RwLock<X> or Vec<X>?)

[soc]

@dinfuehr see https://github.com/dinfuehr/dora/pull/287, that PR would be a prerequisite to further work here.

[dinfuehr]

Ok, the earlier phase looks good except that I can't set modifiers for namespaces, because I can't get mutable values from namespaces: Vec<NamespaceData> in VM.

Is this intentional? (What's the general logic whether something is held as GrowableVec<RwLock<X>>, GrowableVec<X>, Vec<RwLock<X> or Vec<X>?)

I tried to use the simplest type possible, since NamespaceData wasn't mutable so far, RwLock wasn't necessary.

[soc]

Ah, thanks understood.

[soc]

@dinfuehr Could you have a look? Next step would be to switch the comparisons of names to a comparison of the actual thing.

Not sure what's the best way to replace the modifier tests that previously happened in parser.rs.

[soc]

@dinfuehr ping?

[soc]

CI failure is due to Ruby install: Error: Version 2.6.x not found

[dinfuehr]

Hey, I am not going to land this PR as-is. It's way too big to properly review it.

[soc]

@dinfuehr I pulled out the addition of supporting methods into its own commit, please have a look.

[soc]

@dinfuehr I rebased the commits on master such that the fixed CI could run again, can you have a look?