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

Feat: basic record dot completions #3080

Merged
merged 10 commits into from
Sep 26, 2022

Conversation

coltenwebb
Copy link
Contributor

@coltenwebb coltenwebb commented Aug 6, 2022

When a user dots a record type with OverloadedRecordDot enabled, shows all possible record selectors for that type.

Todo:

  • write test
  • make completion items more complete

Notes

  • Requires fresh HieAst to work, which shouldn't be a problem since we are dotting an actively worked on file
  • Doesn't support HasField (virtual fields)
  • Moves getCompletionPrefix, PosPrefixInfo in from LSP.Language.VFS (and patches them to support records in addition to modules)

@coltenwebb coltenwebb force-pushed the completions-work branch 2 times, most recently from 898e577 to 5f4be06 Compare August 12, 2022 13:28
@coltenwebb coltenwebb force-pushed the completions-work branch 2 times, most recently from b0ad658 to d61dac3 Compare September 5, 2022 23:49
@coltenwebb coltenwebb marked this pull request as ready for review September 8, 2022 18:58
@coltenwebb
Copy link
Contributor Author

@pepeiborra Would you mind doing another review? It should be good to go.

ghcide/src/Development/IDE/Plugin/Completions.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Plugin/Completions/Logic.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Plugin/Completions/Logic.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Plugin/Completions/Logic.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Plugin/Completions/Logic.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Plugin/Completions/Logic.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Plugin/Completions/Logic.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Plugin/Completions/Logic.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Plugin/Completions/Logic.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Plugin/Completions/Types.hs Outdated Show resolved Hide resolved
test/testdata/completion/RecordDotSyntax.hs Show resolved Hide resolved
let uses_overloaded_record_dot _ = False
#endif
ms <- fmap fst <$> useWithStaleFast GetModSummaryWithoutTimestamps npath
astres <- case ms of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly stupid question, but why guard here? What happens if we just guard getting the HIE AST on whether or not we have record dot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand you correctly, that's exactly what I'm trying to do here. Maybe there's a simpler way to do this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it was a stupid question: I missed that the uses_overloaded_record_dot predicate needs the module summary!

case (pfix, completionContext) of
(Just (VFS.PosPrefixInfo _ "" _ _), Just CompletionContext { _triggerCharacter = Just "."})
((PosPrefixInfo _ "" _ _), Just CompletionContext { _triggerCharacter = Just "."})
Copy link
Collaborator

Choose a reason for hiding this comment

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

OOI, what is happening in this case? I guess this is basically preventing us from giving completions when people write . which usually corresponds to function composition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how I read this too. getCompletionPrefix doesn't find a prefix word before the cursor so it returns emptiness.

, typeText = Nothing
, label = label
, isInfix = Nothing
, docs = emptySpanDoc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, fields have haddock usually, you know

data Foo {
  hello :: Int -- ^ This is the doc for foo
}

I would expect to get the field haddock in the completion doc!

This might get automatically sorted out for us with #3204, not sure.

@michaelpj
Copy link
Collaborator

Looks like the new test failed on 9.0.2?

@coltenwebb coltenwebb force-pushed the completions-work branch 2 times, most recently from 4231579 to e021ae1 Compare September 25, 2022 22:33
@michaelpj michaelpj merged commit cdbef3e into haskell:master Sep 26, 2022
@michaelpj
Copy link
Collaborator

🎉

@michaelpj
Copy link
Collaborator

Last thing: could you make some issues for any remaining user-facing problems, like maybe the documentation one?

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