Make compiler hint to import unqualified types/values #4304
Conversation
|
The hints should be done now, although I don't if its possible to find the arity (suppose someone wrote |
There was a problem hiding this comment.
Thank you! I've added some comments inline, and snapshot tests will need to be added to test all the new paths through the code.
It would be great to also check the arity of the constructors. Suggesting a zero-arity Wibble when it is being used in a way that takes 2 arguments would be unhelpful.
compiler-core/src/type_/error.rs
Outdated
| if is_type { | ||
| format!("Did you mean to import `{module}.{{type {name}}}`?") | ||
| } else { | ||
| format!("Did you mean to import `{module}.{{{name}}}`") |
There was a problem hiding this comment.
I don't want to encourage unqualified importing of values. Perhaps we should only make these suggestions for types and record constructors.
There was a problem hiding this comment.
Okay! Tying with my point below, we could write the "generic" value case as "Did you mean to reference module.{value}" and, in case it isn't already imported, "Did you mean to import module and reference module.{value}". What do you think?
...tests/snapshots/gleam_core__type___tests__imports__imported_constructor_instead_of_type.snap
Outdated
Show resolved
Hide resolved
compiler-core/src/error.rs
Outdated
| title: "Unknown variable".into(), | ||
| text, | ||
| hint: None, | ||
| hint: suggestions.first().map(|suggestion| suggestion.suggestion(name, false)), |
There was a problem hiding this comment.
There's a vector of suggestions, but only the first one is used. Seems like only one should be given in that case.
There was a problem hiding this comment.
I thought this was fine, as that is what the current code for module suggestions does. Anyway, I'll change it!
There was a problem hiding this comment.
Actually, having a vector of suggestions allows the LS to give all possible quick fixes when a code action to do so is done (I have one basically ready, will do a PR after this one is pushed)
|
The code should now consider arity, but it is an |
|
Hey @lpil, I've answered some of your questions, waiting on clarifications! |
|
I have implemented the code actions locally. Should I push them to this PR or wait for a future one? |
...tests/snapshots/gleam_core__type___tests__imports__imported_constructor_instead_of_type.snap
Outdated
Show resolved
Hide resolved
...src/type_/tests/snapshots/gleam_core__type___tests__errors__same_imports_multiple_times.snap
Outdated
Show resolved
Hide resolved
…import suggestions
|
@lpil, I think I have addressed all of your points! Right now I just need to comment and write tests, and I'm waiting on some clarifications. |
|
Should be all done now! |
|
Now it should be ready! Also, the way I've refactored, it allows for the normal module suggestion to consider arity in the future |
|
@lpil, now the hint is better (thanks Gears for the suggestion) and I have removed the indents in tests, so I think there should be nothing left to do! |
|
@lpil, I'm thinking on redoing this on the latest Gleam version, to facilitate merging, although it might take a bit. What do you think? |
|
Sounds good to me! |
|
Closed, continued in #4602 |
This PR closes #4297
Currently the way it decides which module to hint is by preferring imported modules over importable ones, and sorting by name length.