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

Renaming of indirect references (RecordFieldPuns) #3013

Merged
merged 15 commits into from
Jul 7, 2022

Conversation

OliverMadine
Copy link
Collaborator

@OliverMadine OliverMadine commented Jul 3, 2022

Issue

Consider this code snippet. While explaining this issue, I will refer to the occurrences of field on lines 3, 6, and 7 as A, B, and C respectively.

{-# LANGUAGE NamedFieldPuns #-}

newtype Foo = Foo { field :: Int }

unFoo :: Foo -> Int
unFoo Foo{field} 
    = field

References between these occurrences are not transitive since B consists of two names (as it is punned).
In this example, A references B and B references C but A does not reference C.

As a consequence, trying to rename A causes only A and B to be renamed, but we also want C to be renamed.


Proposed Solution

Doing another pass of the references to find any punned names should be sufficient.

I do not think that we can have doubly-indirect references, so we should only need to do one additional pass to find the transitive closure.

Question

Are there any other cases of having multiple names at the same position that I might have not properly considered?


Closes #2970

@michaelpj
Copy link
Collaborator

The idea of a reference in HieDB is not transitive

Should we make it transitive? This might be easier at the db level, since it would just be a join of the table with itself...

Are there other cases of having multiple names at the same position that I might have not properly considered?

The problem is that B is both a use of a name and a binding of a name. I can' think of any other instances of that...

@OliverMadine
Copy link
Collaborator Author

OliverMadine commented Jul 4, 2022

The idea of a reference in HieDB is not transitive

Should we make it transitive? This might be easier at the db level, since it would just be a join of the table with itself...

I think it is best not to do this in the DB. It seems like HieDB maintains the distinction between the name and the binding of the name. I do not think we want to join the table as it might be useful to keep this information.

In the example above, if I query for the references of B, HieDB gives two sets of references (one for the name and one for the binding). If we consider B as two distinct names, then references in this sense are transitive.

Maybe it is more accurate to say that the issue in this PR is references (between groups of names at the same position) are not transitive. But I think that is much less clear...

@OliverMadine OliverMadine changed the title Renaming of indirect references (RecordFieldPuns) [Draft] Renaming of indirect references (RecordFieldPuns) Jul 5, 2022
@OliverMadine OliverMadine marked this pull request as draft July 5, 2022 13:06
@OliverMadine OliverMadine marked this pull request as ready for review July 5, 2022 13:10
@OliverMadine OliverMadine changed the title [Draft] Renaming of indirect references (RecordFieldPuns) Renaming of indirect references (RecordFieldPuns) Jul 5, 2022
plugins/hls-rename-plugin/test/Main.hs Outdated Show resolved Hide resolved
@michaelpj michaelpj added the merge me Label to trigger pull request merge label Jul 7, 2022
@mergify mergify bot merged commit 0f6cd41 into haskell:master Jul 7, 2022
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.

Rename plugin doesn't work with NamedFieldPuns
3 participants