-
-
Notifications
You must be signed in to change notification settings - Fork 794
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
Conversation
2faacfc
to
4f0e631
Compare
There was a problem hiding this 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
compiler-core/src/type_/printer.rs
Outdated
/// 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>, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
compiler-core/src/analyse.rs
Outdated
} 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
...am_core__language_server__tests__hover__hover_contextual_type_annotation_aliased_module.snap
Show resolved
Hide resolved
I've addressed the immediate feedback you gave, and responded to your questions. I'll wait for you to look at those before continuing |
0789ce5
to
8112b4b
Compare
8112b4b
to
148b266
Compare
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 |
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. |
8112544
to
a7c29d0
Compare
There was a problem hiding this 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!
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 toTypeNames
, 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:
Write tests for these new cases
Merge the
TypeNames
struct with theValueNames
struct for pattern-printing, as they have very similar semantics and functionalityStore information about type variables in the
TypeNames
struct when they are created, so we can print these properlyCorrectly 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:
Questions
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?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?