jakt icon indicating copy to clipboard operation
jakt copied to clipboard

parser+typechecker+compiler+tests: Fix unnamed namespaces

Open greytdepression opened this issue 3 years ago • 4 comments

After looking deeper into the issue of unnamed namespaces that I brought up in #703, there were some more issues that had to be fixed in order to make a test for this bug. Importantly I added a uid: u64 field to parsed namespaces and checked namespaces in order to identify unnamed namespaces in the typechecker. Before we did not even try and instead created a new scope each time we encountered the namespaces again in the typechecker (which naturally caused some bugs). Thanks to @jntrnr for making me look deeper into this issue. I didn't really think about it as much more than fixing a typo when I opened #703.

greytdepression avatar Jun 23 '22 17:06 greytdepression

It should be noted that while unnamed namespaces now are successfully typechecked, there is AFAIK no way to actually access any of its members from outside with the current state of Jakt. So I was not able to test if and how that might work.

greytdepression avatar Jun 23 '22 17:06 greytdepression

Also if this PR gets approved and merged, we should port over the fixes to the selfhosted compiler as it will have the same issues.

greytdepression avatar Jun 23 '22 17:06 greytdepression

@greytdepression - nice catch to see if we can normalise this thing a bit better. I looked through the code and had a couple questions:

Is uid the same as ScopeId? It feels like one way we went wrong with the rust-based compiler is that we have a lot of similar concept. Namespaces, scopes, modules. Makes me wonder if we should maybe tease apart the scopes from the namespace/module thing and then make their relationship a bit more clear...

We should definitely be clear about when we're creating a new scope - like you're saying. If we aren't doing that right we're going to get some weird behaviour for sure.

I wonder. Should we maybe just get rid of namespaces and just use modules? They already have ModuleIds we could use as the unique identifier. This also means we wouldn't have to do any additional work when we encounter the same one a second time as the work we did before would be interned like the other data structures.

sophiajt avatar Jun 23 '22 19:06 sophiajt

@jntrnr Technically, no. The namespace uid is not the same as the ScopeId. ScopeIds are a concept within the typechecker, whereas the namespace uid is introduced by the parser and the typechecker only makes use of it to differentiate them later. But yes, it is a bit of a hack to get things to work in the current framework of namespaces, modules, and scopes.

I agree it might be a good idea to rework the entire concept and unify namespaces and modules into one. But that might be big enough of an issue that we might want to discuss that publicly first.

greytdepression avatar Jun 23 '22 19:06 greytdepression

Closed as this is a change for the now-gone Rust-based compiler. Feel free to port your changes to the new compiler, if they're not already added, and open a new PR. :^)

AtkinsSJ avatar Aug 26 '22 22:08 AtkinsSJ