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

Display qualified type names on LSP hover #3599

Merged
merged 13 commits into from
Sep 23, 2024

Conversation

GearsDatapacks
Copy link
Member

@GearsDatapacks GearsDatapacks commented Sep 8, 2024

Closes #2993
This uses the Printer implemented in #3007 to print correctly qualified or aliased type names when hovering with the language server. I've had to slightly rework the printer so it doesn't take a mutable reference to TypeNames, because that wasn't really feasible in the context of the language server.

TODO

Done
  • Fix the case where a type alias's generics don't map one-to-one with the aliased type, for example:

    type LocalResult = Result(LocalType, LocalError)
    let a: LocalResult = local_fallible_function()
    //  ^ hovering here shows `LocalResult(LocalType, LocalError)`
  • Write tests for these new cases

  • Merge the TypeNames struct with the ValueNames struct for pattern-printing, as they have very similar semantics and functionality

  • Store information about type variables in the TypeNames struct when they are created, so we can print these properly

  • Correctly qualify prelude types when shadowed

  • When importing a type alias, treat is as a local alias rather than a local custom type

  • Don't alias types when hovering over the annotation for an alias itself:

type Number = Int
//            ^ Hovering here shows `Number`, because we've aliased it, but it should probably print `Int`

Questions

  1. Currently, this change is for hover and document symbols, which are the two things which use the pretty printer in engine.rs. There's also signature help, which could probably benefit from this. Any other places we would want this? Maybe in the little hint for value completions?
  2. How should aliases other than the trivial case of type A = B be handled? Should we try to print alias names for complex types like #(fn(Int) -> Float, Result(String, BitArray)), or just leave it as is?

@GearsDatapacks GearsDatapacks marked this pull request as ready for review September 10, 2024 15:08
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice work!!! I've left a bunch of stylistic notes and questions

/// Types which exist in the prelude, and haven't been shadowed by a local type.
/// These are a special case, because they are unqualified by default, but can be
/// shadowed and then must be qualified.
unshadowed_prelude_types: HashSet<EcoString>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do prelude types get a special case instead of being inserted into the scope like any other?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because unlike other types, types in the prelude can be shadowed, and need to be printed differently. The local_types map stores a map from module + type names to their local alias. This means that if we have ("gleam", "Int") map to "Int", but also ("some_mod", "Int") map to "Int", both will be printed as Int. I can do this in a different way if you would like; this is just the first way that came to mind.

This is different from the value equivalent of this, since any value can be shadowed, not just prelude values. But I could implement a similar system as there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have the special case for the prelude at all? Could they be in local_types by default?

Copy link
Member Author

@GearsDatapacks GearsDatapacks Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can, but unlike other types, prelude types can be shadowed, meaning the types gleam.Result and current_module.Result would be printed as the same. We still need some way to distinguish those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would they be printed the same? If prelude result has been overridden with the locally defined result then the code would print the prelude result as gleam.Result and the local one as Result, wouldn't it?

Copy link
Member Author

@GearsDatapacks GearsDatapacks Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, they should't be printed the same. But if we just stick prelude types in local_types, there's nothing stopping them from being printed the same. That's the entire point of this piece of code. We need some way to keep track of which prelude types have been shadowed, so that we can print them differently.

Copy link
Member

@lpil lpil Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, could you explain what the problem is?

The prelude types shouldn't have any special printing, they should behave as if every module started with this:

import gleam.{type Nil, type Result, type Int, type Float, ...}

From reading the code it looks like if the special case is removed it works as intended, but presumably I'm missing something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll remove the special case and we'll see

} else {
// We need to make sure we register the type as existing, even if we don't define
// the underlying type for printing, because these types do still affect printing,
// for shadowing prelude types, even if they aren't printed at all.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what does this mean? How does it affect printing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to do with the way prelude types are handled, which are discussed above. I'll wait until further feedback on that to elaborate, because this may become redundant.

@lpil lpil marked this pull request as draft September 10, 2024 16:42
@GearsDatapacks
Copy link
Member Author

I've addressed the immediate feedback you gave, and responded to your questions. I'll wait for you to look at those before continuing

@GearsDatapacks GearsDatapacks marked this pull request as ready for review September 10, 2024 17:48
@GearsDatapacks GearsDatapacks force-pushed the qualified-types branch 4 times, most recently from 0789ce5 to 8112b4b Compare September 20, 2024 19:53
@ascandone ascandone mentioned this pull request Sep 23, 2024
1 task
@GearsDatapacks
Copy link
Member Author

I have removed the special case for prelude types and as you can see, the tests are failing. Hopefully that will demonstrate the problem, because I can't seem to explain it properly with words

@GearsDatapacks
Copy link
Member Author

As for solutions to this, I went for an explicit special case for prelude types, because unlike values, which can be shadowed anywhere, types (except for prelude types) cannot.
Perhaps the best way is to do it in the same way as the values track them (see here). This would remove the special case for prelude types, although it would store some redundant information since most types cannot be shadowed at all.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I see now, thanks, that was is helpful. I've added eviction on shadowing which covers the prelude without a special case for that one module.

Really excellent work here! Thanks again!

@lpil lpil merged commit 9cecfe0 into gleam-lang:main Sep 23, 2024
12 checks passed
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.

Display qualified type names on LSP hover
2 participants