Skip to content

Fix renaming data constructors with fields (resolves #2915, resolves #4083) #4635

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

Merged
merged 7 commits into from
Jun 20, 2025

Conversation

jhrcek
Copy link
Collaborator

@jhrcek jhrcek commented Jun 16, 2025

Attempt at resolving #2915 and #4083

Copy link
Collaborator

@soulomoon soulomoon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing it.

@jhrcek
Copy link
Collaborator Author

jhrcek commented Jun 16, 2025

Wait, it will require some work. It doesn't work as is, yet 😸

@soulomoon
Copy link
Collaborator

soulomoon commented Jun 16, 2025

My bad, I saw all the tests have been passed and thought it is ready. Forget to check if the new test have been supplied😆
We can pick up the test from https://github.com/haskell/haskell-language-server/pull/4089/files, I think.

@jhrcek
Copy link
Collaborator Author

jhrcek commented Jun 16, 2025

Looks promising, but still not 100% done.

There's an issue with Record wildcards - the current impl. somehow removes the .. from record wildcard pattern matches, so renaming constructor does something like this:

OriginalConstructor {..} -> RenamedConstructor {}

@jhrcek jhrcek changed the title Prevent renaming record fields whenever record constructor is renamed Fix renaming data constructors with fields (resolves #2915, resolves #4083) Jun 17, 2025
@jhrcek jhrcek marked this pull request as ready for review June 17, 2025 04:52
@jhrcek jhrcek requested review from michaelpj and wz1000 as code owners June 17, 2025 04:52
@@ -24,6 +24,11 @@ tests :: TestTree
tests = testGroup "Rename"
[ goldenWithRename "Data constructor" "DataConstructor" $ \doc ->
rename doc (Position 0 15) "Op"
, goldenWithRename "Data constructor with fields" "DataConstructorWithFields" $ \doc ->
rename doc (Position 1 13) "FooRenamed"
, knownBrokenForGhcVersions [GHC96, GHC98] "renaming Constructor{..} with RecordWildcard removes .." $
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fyi this is a separate new issue I discovered while testing this PR that affects ghc 9.6 and 9.8.
I opened a separate issue to track that: #4636

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM

@fendor fendor added the merge me Label to trigger pull request merge label Jun 20, 2025
@mergify mergify bot merged commit 0a9b1cb into master Jun 20, 2025
37 checks passed
@fendor
Copy link
Collaborator

fendor commented Jun 20, 2025

@jhrcek Just randomly found https://github.com/haskell/haskell-language-server/blob/master/ghcide/session-loader/Development/IDE/Session.hs#L226, do we need to increase this to invalidate existing hiedb databases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants