Fixed export package-definitions not including cached modules

[mine-tech-oficial]

This PR closes #2898

[mine-tech-oficial]

Do I need to write any tests? I didn't write any, just fixed the snapshots. I guess I could duplicate the already existing tests to take account cached modules.

[mine-tech-oficial]

The problem is that, to build the interface from the cache, we'd need to save the typed AST, which I think is less space-efficient than just saving the interface. That's why I rebuild the modules.

[lpil]

Neither of those approaches are what we want.

Does the cache not have enough information to build the module interface JSON from? What's missing?

[mine-tech-oficial]

As the package interface is built from various module interfaces, each of these being built from all the definitions, it needs to be built from the typed AST, which isn't cached

[lpil]

Why does it need to be built from typed AST? What information do they hold that the interface JSON needs that the cache does not have?

[mine-tech-oficial]

I tried adapting the from_module function of the ModuleInterface struct from the package_interface module to work with UncompiledModules (untyped ASTs) but, as it turns out, it needs the type info.
For example, here's the error cargo gives:

error[E0308]: mismatched types
   --> compiler-core/src/package_interface.rs:615:73
    |
615 | ...                   type_: from_type_helper(&arg.type_, &mut id_map),
    |                              ---------------- ^^^^^^^^^^ expected `&Type`, found `&()`
    |                              |
    |                              arguments to this function are incorrect
    |
    = note: expected reference `&type_::Type`
               found reference `&()`

[lpil]

The cache has all the type information for a compiled module, so you can use that.

[GearsDatapacks]

From when I looked into implementing this, it seems the main limitation is type aliases. Type alias information isn't stored in the cached interface, so that would need to be accessed somehow

[mine-tech-oficial]

After trying to adapt the `from_module` function of the `ModuleInterface` struct from the `package_interface` module to work with `UncompiledModule` and `ModuleInterface` (from the `type_` module) and, as Gears pointed out, it seems that the only thing missing in the cache is the type aliases.

[GearsDatapacks]

You don't need UncompiledModule. If you add type aliases to it, ModuleInterface should be enough

[mine-tech-oficial]

I think I might need add a HashMap of TypeAlias to the ModuleInterface struct. Is that okay?

[mine-tech-oficial]

I've been doing this and I've stumbled onto a problem: somehow, when the code will encode the modules to the cache, somehow the documentations have an extra newline? Do I have to somehow fix it or can I leave it like so?

[GearsDatapacks]

I would say if you can reproduce the bug without your changes, then open a separate issue and let that be fixed separately. If this PR introduces that bug, then please fix it

[mine-tech-oficial]

I can actually reproduce it separately, will open an issue for it then

[mine-tech-oficial]

Issue opened at #3712

[mine-tech-oficial]

Now it reuses as much of the cache as possible, although there is a test failing and I don't know why (need to fix). I'll open for review just to make sure any of the overall logic isn't wrong.

[lpil]

Looking good so far! Thank you

[mine-tech-oficial]

Uhm, apparently this kind of broke the LSP, as it isn't considering the type aliases anymore. Guess I'll also need to tweak it

[mine-tech-oficial]

I think it should be all done now!

[lpil]

Hello! Looking good. I've left some questions inline

[lpil]

What does this mean? Is it unfinished?

[mine-tech-oficial]

Forgot about this todo 😅. Is there a problem for the code to register the type alias as a non-opaque type, just for type analysis?

[lpil]

I'm not aware of any problems, but I'm not sure what you mean here.

[mine-tech-oficial]

Hey @lpil, left some answers to your questions. Waiting for some answers!

[mine-tech-oficial]

Hey @lpil, I've been trying to rewrite the type analysis so that it doesn't need to register the type aliases as types just for the analysis, but the analyze_custom_type function returns a TypeConstructor. Is it ok to return the TypeAliasConstructor (in case it's a type alias and not a "normal" type) as a non-opaque TypeConstructor?

[mine-tech-oficial]

Actually I still need to discover what the parameters of the TypeConstructor would be...

[mine-tech-oficial]

Hey @lpil, do you think it's ok for me to leave the PR with the odd way of registering type aliases as "normal types" just for the analysis, and removing it later (as I still couldn't find a good way to consider it separately), or do you want me to try and fix it anyway?

[GearsDatapacks]

Hi @mine-tech-oficial, are you still working on this PR? If not, I'm happy to pick up the work as it would be cool to get this fixed. Thanks!

[mine-tech-oficial]

@GearsDatapacks, actually for the past few weeks I didn't do much progress due to not having much time, although I was writing some code to get the labels of the parameters

[mine-tech-oficial]

@lpil, now I think I only need to find the documentation (not sure how though)

[lpil]

Function types don't contain names! Please remove this.

It would also be called a parameter. Things have parameters, and for those parameters they accept arguments.

[mine-tech-oficial]

So you mean that a parameter has a name/label, while an argument doesn't? In that case, I'd reuse the TypeValueConstructorParameter struct?

[mine-tech-oficial]

To be fair I wasn't happy with the approach I took 😅 . Tomorrow I'll try tackling this again.

[lpil]

Neither has names at the type level, but parameters can have names locally within a definition.

[mine-tech-oficial]

Hey @lpil, I've left a comment on your answers. Waiting for you to confirm it! (no rush by the way)