-
-
Notifications
You must be signed in to change notification settings - Fork 368
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
[hls-explicit-record-fields-plugin] Expand used fields only #3386
Conversation
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
-- to 'Name' referring to the same entity is considered equal. In order | ||
-- to ensure that no false-positive is reported (in the case where the | ||
-- 'name' itself is part of the given list), the inequality of source | ||
-- locations is also checked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps an example? I couldn't quite figure out why this matters, and tbh it seems rather weird to have a match on the unique but not on the srcspan?
@@ -212,8 +273,13 @@ renderRecordInfo (RecordInfoCon ss expr) = RenderedRecordInfo ss <$> showRecordC | |||
-- as we want to print the records in their fully expanded form. | |||
-- Here `rec_dotdot` is set to `Nothing` so that fields are printed without | |||
-- such post-processing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment could do with some tweaks. It suggests that this is primarily about printing the records which makes it sounds like you always print them as they are, but now you're adding some additional logic to print something else depending on the fields in use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have revised this and a bunch of other comments, can you take another look to see if it looks better?
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great stuff!
This is a follow-up PR to #3304, which makes record wildcard expansion code action to only expand fields that are used.
How it works?
Since the plugin already operates on
RenamedSource
, we have access to all theName
s that GHC has resolved for us. We utilize this information by first collecting all theName
s within the source file by asyb
traversal, and then for every field of the record to be expanded we check if theName
of the field exists within this collected list ofName
s (except for the field itself).(@santiweight: you might be interested in this approach)