Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Context aware type pprint #3312

Closed
wants to merge 15 commits into from

Conversation

ascandone
Copy link
Contributor

@ascandone ascandone commented Jun 22, 2024

Implements #2993

TODOS

  • handle type aliases

Open questions

Should hovered exported types be context-aware pprinted? e.g.

// Suppose X has type T
import {type T as Renamed, X}
// What should we show when hovering on `X` ?

Specs

Handled cases (assume the current file is my_module.gleam:

1) types in imported modules

import some_module
some_module.Type // <- pprinted type

2) types in module that are not imported

// let's say that the type is in the `some_module` module
some_module.Type // <- pprinted type

3) types in module that are imported and renamed

import some_module as renamed_module
renamed_module.Type // <- pprinted type

4) types in modules that are imported and exposed as unqualified imports

import some_module.{type Type}
Type // <- pprinted type

5) types in modules that are imported and exposed as unqualified imports, and renamed

import some_module.{type Type as RenamedType}
RenamedType // <- pprinted type

6) types defined locally

type Type {}
Type // <- pprinted type

7) types from the gleam prelude

Int // <- pprinted type

@lpil
Copy link
Member

lpil commented Sep 20, 2024

Hello! Are you still working on this?

@ascandone ascandone force-pushed the context-aware-type-pprint branch from 18dfb5a to a3af907 Compare September 20, 2024 19:53
@ascandone ascandone force-pushed the context-aware-type-pprint branch from bf2677e to 2e4409f Compare September 20, 2024 20:45
@ascandone
Copy link
Contributor Author

ascandone commented Sep 20, 2024

Hello! Are you still working on this?

👋 I am blocked due to the seemingly complex problem of printing aliases of types instead of the original definition.
Here's an example of the issue I'm referring to: 2e4409f

I think the desidered behaviour would be to pprint the alias instead of the actual type definition. This is useful for the pattern of re-exporting private types as alias, like in lustre:
Screenshot 2024-09-20 at 22 35 45
As you can see, the attribute will be pprinted as lustre/internals/vdom.Attribute instead of lustre/attributes.Attribute

However, fixing this doesn't look trivial:
here's the value of the AliasedType of the commit that I linked before:
Named { publicity: Public, package: "app", module: "html/internal", name: "Attribute", args: [] }
This value is passed as type in the fn print<'a>(&mut self, type_: &Type) -> Document<'a> function so I don't even know if it's possible to workaround it

Proposed solution

pretty::print(t) returns TypeAlias(p1', .., pn') if exists a type (either defined locally or imported)

type TypeAlias(p1, .., pn) = MyType(a1, .., an)

such that unify(MyType(a1, .., an), t) yields the substitution { p_i ' => t_i forall i in 1, .., n } (note that Gleam currently forbids p_inot to occur inside MyType(a1, .., an)).
If such substitution does not exist, just print t normally

example 1

// internal
pub type PubAttribute(a) =
  internal.Attribute(a)

pub fn checked() -> PubAttribute(String) { todo }

when printing PubAttribute(String), we actually receive internal.Attribute(a). This value can be unified with the value of the PubAttribute alias, with the a => String substitution. Therefore we'll print PubAttribute(String), as expected.

example 1 bis

// internal
pub type PubAttribute(a) =
  internal.Attribute(a)

pub fn checked() -> internal.Attribute(String) { todo } // <- note we aren't using the alias

This behaves in the same way as previous example. This might likely be counter-intuitive; however it looks like ocaml behaves similarly, and in some cases it might even be handy

example 2

// internal
pub type PubAttribute(a, b) =
  internal.Attribute(Result(b, a))

pub fn checked() -> PubAttribute(Int, Bool) { todo }

Applying the alias definition, we get internal.Attribute(Result(Bool, Int)). This value can be unified with the value of the PubAttribute alias, with the a => Bool, b => Int substitution. Therefore we'll print PubAttribute(Bool, Int)

example 3

// internal
pub type PubAttribute(a) =
  internal.Attribute(Option(a))

pub fn checked() -> internal.Attribute(Int) { todo }

In this case, internal.Attribute(Int) cannot be unified with the alias definition, therefore is printed normally

@ascandone
Copy link
Contributor Author

Oh and btw, would it be possible to try to merge this one first? It's done except for this couple of doubts that I mentioned, could you give me an hint with that?

@ascandone
Copy link
Contributor Author

Another solution could also consist in actually adding a "TypeAlias" variant to the Type enum
Roughly like

enum Type {
  // ...
  TypeAlias {
       // same as "Named"
        publicity: Publicity,
        package: EcoString,
        module: EcoString,
        name: EcoString,
        args: Vec<Arc<Type>>,
     
        // but also have the type it resolves to
        // so that unify() can recur on it
        value: Arc<Type>,
    },
}

I think that's the way Elm manages to have very good messages regarding type aliases
https://github.com/elm/compiler/blob/2f6dd29258e880dbb7effd57a829a0470d8da48b/compiler/src/Type/Type.hs#L93

@lpil
Copy link
Member

lpil commented Sep 21, 2024

I think the desidered behaviour would be to pprint the alias instead of the actual type definition.

We don't want to do this as it results in very poor error messages and hover information- the programmer can't see what the type is so they don't know how to debug their program.

Context aware type printing is implemented here so that code can be reused here.

@ascandone
Copy link
Contributor Author

I think the desidered behaviour would be to pprint the alias instead of the actual type definition.

We don't want to do this as it results in very poor error messages and hover information- the programmer can't see what the type is so they don't know how to debug their program.

Context aware type printing is implemented here so that code can be reused here.

Got it! Just another thing, isn't this other PR implementing the same thing? Should I close mine?

@GearsDatapacks
Copy link
Member

I started working on it since this PR hadn't seen any changes in several months. I'm not sure what to do if you want to continue working on this

@GearsDatapacks
Copy link
Member

The other PR has now been merged, so I guess this one should be closed now

@ascandone
Copy link
Contributor Author

yeah sorry about the mess :)

@ascandone ascandone closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants