Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Record Dot Hover Types #3016

Merged
merged 5 commits into from
Jul 19, 2022
Merged

Record Dot Hover Types #3016

merged 5 commits into from
Jul 19, 2022

Conversation

coltenwebb
Copy link
Contributor

@coltenwebb coltenwebb commented Jul 4, 2022 •
edited
Loading

Working towards #2732 (reopened since I closed and force-pushed my last PR, which github dislikes)

This PR patches the way HieAST is generated by GHC. For performance reasons, not all nodes in the AST get expanded and checked for types. This caused the nodes corresponding to record-dot-syntax to not be included in the HieAST. This patch adds them back.

Notes

This is what type info on hover looks like:

Code_ps0jgEHy0v

@coltenwebb coltenwebb force-pushed the support-record-dot-syntax branch from c9185bc to 22b64b7 Compare July 4, 2022 11:51
@coltenwebb coltenwebb changed the title Record Dot Syntax Record Dot Hover Types Jul 4, 2022
@coltenwebb coltenwebb mentioned this pull request Jul 4, 2022
Open
@coltenwebb coltenwebb marked this pull request as ready for review July 4, 2022 16:44
@kokobd
Copy link
Collaborator

kokobd commented Jul 5, 2022

Do we have to introduce the large file with 2000+ lines? It looks similar to GHC/Iface/Ext/Ast.hs, could you explain it a bit?

@coltenwebb
Copy link
Contributor Author

You're right, the HieAst.hs in src-ghc92 is the same one in 9.2.x GHC except I inserted the line 789, to ensure the record dot syntax gets desugared. I agree that adding so much to the codebase is less than ideal. Maybe this explanation belongs in a banner comment at the top of the file? I'll also try to get this patch directly into GHC, but it may be better to keep this here for backwards compatibility with 9.2.{1,2}

@michaelpj
Copy link
Collaborator

I think that's the nature of the hie-compat packages, they mostly consist of wholesale copies of files from GHC with small changes. I'm not sure if we can easily improve that, but comments are definitely a good idea.

@wz1000
Copy link
Collaborator

wz1000 commented Jul 5, 2022

We would want a patch that is accepted for GHC HEAD before we can merge such changes to hie-compat. We have followed this policy for all the previous patches we have in hie-compat.

@coltenwebb coltenwebb force-pushed the support-record-dot-syntax branch from 01c4426 to 0199547 Compare July 16, 2022 12:36
@coltenwebb
Copy link
Contributor Author

coltenwebb commented Jul 16, 2022 •
edited
Loading

Should be good to go. I've added the comments to HieAst.hs, and the patch is merged into GHC master.

@wz1000
Copy link
Collaborator

wz1000 commented Jul 16, 2022 via email

@coltenwebb coltenwebb force-pushed the support-record-dot-syntax branch from e70846e to a4ec3ec Compare July 17, 2022 19:05
wz1000
wz1000 approved these changes Jul 18, 2022
View reviewed changes
Copy link
Collaborator

@wz1000 wz1000 left a comment

Choose a reason for hiding this comment

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

Thanks, looks all good now.

@coltenwebb coltenwebb force-pushed the support-record-dot-syntax branch from a4ec3ec to 46ae097 Compare July 18, 2022 12:33
@michaelpj
Copy link
Collaborator

Just needs a test, I think?

@coltenwebb coltenwebb force-pushed the support-record-dot-syntax branch from 46ae097 to ef64513 Compare July 19, 2022 02:37
@coltenwebb coltenwebb requested a review from pepeiborra as a code owner July 19, 2022 02:37
@coltenwebb
Copy link
Contributor Author

I added three tests, there are additional tests for the record dot syntax types on the GHC side too.

@coltenwebb coltenwebb force-pushed the support-record-dot-syntax branch from ef64513 to a516302 Compare July 19, 2022 12:06
@coltenwebb
Copy link
Contributor Author

✨

@coltenwebb coltenwebb force-pushed the support-record-dot-syntax branch from a516302 to 2082248 Compare July 19, 2022 12:30
@michaelpj
Copy link
Collaborator

Sweet!

@michaelpj michaelpj merged commit b7c4274 into haskell:master Jul 19, 2022
michaelpj
michaelpj reviewed Jul 19, 2022
View reviewed changes
ghcide/test/exe/Main.hs
, tst (getTypeDefinitions, checkDefs) aL20 sourceFilePath (pure [ExpectNoDefinitions]) "Polymorphic variable"]

recordDotSyntaxTests
| ghcVersion == GHC92 =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, too late: should this check be more generous? We expect these tests to work on later versions also, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's correct. I'll make a PR for that

sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request Sep 12, 2022
@coltenwebb @sloorush
Record Dot Hover Types (haskell#3016)
fc7a5a7 
* patch hieast

* add comments

* add hlint ignore

* update readme

* add tests
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@michaelpj michaelpj michaelpj left review comments

@wz1000 wz1000 wz1000 approved these changes

@pepeiborra pepeiborra Awaiting requested review from pepeiborra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@coltenwebb @kokobd @michaelpj @wz1000