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

Extend ImportCompletion with declarationType #3997

Merged
merged 6 commits into from Jan 24, 2021
Merged

Extend ImportCompletion with declarationType #3997

merged 6 commits into from Jan 24, 2021

Conversation

i-am-the-slime
Copy link
Contributor

@i-am-the-slime i-am-the-slime commented Jan 23, 2021

Description of the change

This change extends the Completion datatype in the IDE. More specifically, it adds declarationType as a field to import completions. This type represents what type of declaration (value, type, typeclass, operator, type operator, etc.) the IDE will add to the file if applied.

This addresses #3083 which currently prevents users from importing identifiers if there is more than one candidate with the same name in a file (for example the type-level /\ and the value level /\ from Data.Tuple.Nested). Today, attempting to import /\ causes psc-ide to throw an exception.
It's already possible to refine the import command with a namespace filter to make the import succeed, however users (and clients) currently lack the necessary information to make a decision about the namespace. This PR adds this information.

You can also read a longer background story


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution

@i-am-the-slime i-am-the-slime changed the title Ide improvements Extend ImportCompletion with declarationType Jan 23, 2021
@kritzcreek
Copy link
Member

This is probably my favourite PR I've gotten so far :) Thank you for the detailed write-up.

I don't think it's a big deal, but we might want to think about how before this PR it doesn't matter if you pick the type or data constructor completion for a newtype, because you'll always get the import that includes the constructor. Once we merge this users will get the choice which is nice, but it also means they have to think about which of the two completions to pick. You can always automatically fix the compiler warning if you didn't need the constructor, but failing to import it when you need it produces a compiler error, that requires more thinking than just applying a suggestion.

Code-wise you can use the DeclarationType from here, instead of re-creating it:

data DeclarationType
= Value
| Type
| Synonym
| DataConstructor
| TypeClass
| ValueOperator
| TypeOperator
| Module
deriving (Show, Eq, Ord)

That way the clients could also just use that field and plug it directly into a declaration type filter, rather than moving back and forth between namespaces and declaration types: https://github.com/purescript/purescript/blob/7ecc42669c69682996f2196ba2eef6c4ca827348/psc-ide/PROTOCOL.md#declaration-type-filter

Also thanks for the note about how the ide servers "Can't parse your command" errors aren't helpful when you're hacking on it, I know how to fix that problem but just forgot about doing it.

@i-am-the-slime
Copy link
Contributor Author

Thanks for the praise for the PR, it makes me very happy!

I changed the code to use the existing declaration type. It's much simpler that way!

Concerning potential confusion for imports, I completely agree with you that there's no need to confuse users when there's really one good option of imports for an identifier. However, since we have these edge cases I prefer exposing this right away here, and then deciding how to present the right information to the user in the language server.

Something along the lines of: If there are duplicates where not specifying a type declaration filter will lead to an exception we add the information to the import label. Otherwise, we could merge multiple non-conflicting suggestions into just the one for the Identifier(..).

Having said that, we could use the same logic that we use for always importing Identifier(..) instead of Identifier to simply import both (/\), type (/\) and then remove unused imports (I guess the chance that a user might want both is high anyway). Somehow I feel it'd be better to allow the LS to make these decisions and keep psc-ide as dumb and literal as possible.

Copy link
Member

@kritzcreek kritzcreek left a comment

Choose a reason for hiding this comment

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

Just one more nit, thanks again, this looks great!

src/Language/PureScript/Ide/Imports.hs Outdated Show resolved Hide resolved
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.

None yet

3 participants