Skip to content

Rename a constructor renames all record fields #2915

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.

Sign up for GitHub

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

Jump to bottom
Open
deemp opened this issue May 20, 2022 · 14 comments
Open

Rename a constructor renames all record fields #2915

deemp opened this issue May 20, 2022 · 14 comments
Assignees
@wz1000 @OliverMadine
Labels
component: hls-rename-plugin priority: high High priority item status: blocked Not actionable, because blocked by upstream/GHC etc. type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@deemp
Copy link

deemp commented May 20, 2022 •
edited
Loading

Your environment

Which OS do you use:
Ubuntu 20.04
Which LSP client (editor/plugin) do you use:
VS Code, Haskell extension v2.2.0, ghc 9.2.0, HLS 1.7.0.0
Describe your project (alternative: link to the project):
A .yml parser.

Steps to reproduce

See examples in #3106 (comment)

Expected behaviour

Only the constructor name is updated

Actual behaviour

All fields of a record are renamed the same as the new consructor name

Include debug information
@deemp deemp added status: needs triage type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. labels May 20, 2022
@OliverMadine
Copy link
Collaborator

Hi,

Thanks for the report!

Please could you update the link to the commit for the steps to reproduce? It looks like the repository might be private.

@br4ch1st0chr0n3

@deemp
Copy link
Author

deemp commented May 20, 2022

Hi, sorry for that. Should be public now, @OliverMadine.

@OliverMadine
Copy link
Collaborator

OliverMadine commented May 22, 2022 •
edited
Loading

It seems like the issue is hie files including record field names as references to the constructor.
This is only happening with ghc >= 9.

@wz1000 maybe you have some more insight?

@OliverMadine OliverMadine added the status: blocked Not actionable, because blocked by upstream/GHC etc. label May 22, 2022
@July541 July541 mentioned this issue Jun 19, 2022
Closed
OliverMadine added a commit to OliverMadine/haskell-language-server that referenced this issue Jul 3, 2022
@OliverMadine
test: ignore record field tests for ghc92 (haskell#2915)
d8a5b23
OliverMadine added a commit to OliverMadine/haskell-language-server that referenced this issue Jul 3, 2022
@OliverMadine
test: ignore record field tests for ghc90 (haskell#2915)
80c3290
This was referenced Jul 5, 2022
Merged
Open
@pepeiborra
Copy link
Collaborator

It seems like the issue is hie files including record field names as references to the constructor.
This is only happening with ghc >= 9.

Is this a bug or a feature?

If it is a bug, we will need to report it in the GHC bug tracker and ideally contribute a fix. And back port the fixes to hie-compat: https://github.com/haskell/haskell-language-server/tree/master/hie-compat

mergify bot added a commit that referenced this issue Jul 7, 2022
@OliverMadine @mergify
Renaming of indirect references (RecordFieldPuns) (#3013)
0f6cd41 
* test: add tests for record puns

* feat: rename indirect references
refactor: remove "safe" from function names

* test: ignore record field tests for ghc92 (#2915)

* test: ignore record field tests for ghc90 (#2915)

* fix: update record field test ignore message

* expand comment about indirect reference renaming

* fix: find all punned references

* test: ignore record field pun test for ghc > 9

* docs: mention test in indirect pun explaination

* link issue for ignored record field rename tests

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@wz1000 wz1000 self-assigned this Aug 10, 2022
@July541 July541 mentioned this issue Aug 15, 2022
Closed
@michaelpj
Copy link
Collaborator

Is this still a problem?

@konn
Copy link
Collaborator

konn commented Jan 16, 2024

Yes. I've gotten bitten by this yesterday at least with HLS 2.5.0.0.

@konn
Copy link
Collaborator

konn commented Jan 17, 2024

I've just tested and confirmed that it still occurs with HLS 2.6.0.0.

@theGhostJW theGhostJW mentioned this issue Feb 17, 2024
Closed
@soulomoon soulomoon mentioned this issue May 2, 2024
Merged
@soulomoon
Copy link
Collaborator

soulomoon commented May 2, 2024 •
edited
Loading

Perhaps the type RefMap a = Map Identifier [(Span, IdentifierDetails a)] should contains the NodeOrigin to indicate if it is evidence variable or not

@jhrcek
Copy link
Collaborator

jhrcek commented May 2, 2024 •
edited
Loading

Note that I took a stab at fixing this in #4089

That fix works, but requires changes on hiedb side (skipping indexing of all Identifiers whose NodeOrigin is GeneratedInfo).
We agreed with @michaelpj that instead of that approach, we should rather add something like a boolean column for indexed identifiers saying whether that identifier is generated or not.
This would then be used by the rename plugin which would filter out all generated occurrences of given identifier, thus avoiding this bug via different route.

@soulomoon
Copy link
Collaborator

soulomoon commented May 2, 2024 •
edited
Loading

Yes that fix looks good

I also make an issue for the type change as well https://gitlab.haskell.org/ghc/ghc/-/issues/24751

aavogt added a commit to aavogt/haskell-language-server that referenced this issue Jul 3, 2024
@aavogt
test case for haskell#2915 (currently fails)
a9bc6df
@jhrcek jhrcek mentioned this issue Jul 10, 2024
Draft
@fendor fendor mentioned this issue Jul 11, 2024
Closed
@soulomoon soulomoon mentioned this issue Oct 21, 2024
Open
@vst
Copy link

vst commented Feb 20, 2025 •
edited
Loading

Is wz1000/HieDb#77 sufficient to solve this? Or do we still need a solution on GHC-side?

Edit: Sorry. I wanted to refer to wz1000/HieDb#78.

@soulomoon
Copy link
Collaborator

Is wz1000/HieDb#77 sufficient to solve this? Or do we still need a solution on GHC-side?

Yes, I think so

@SmartHypercube
Copy link

Is wz1000/HieDb#77 sufficient to solve this? Or do we still need a solution on GHC-side?

Yes, I think so

(English is not my native language, please excuse me if the answer is obvious.) Does this reply mean "Yes, I think ... is sufficient to solve this" or "Yes, I think we still need a solution on GHC-side"?

@soulomoon
Copy link
Collaborator

@SmartHypercube . Former, but better if we do it in ghc too.

@soulomoon soulomoon added the priority: high High priority item label May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@wz1000 wz1000

@OliverMadine OliverMadine

Labels
component: hls-rename-plugin priority: high High priority item status: blocked Not actionable, because blocked by upstream/GHC etc. type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests