Use pretty printing context when expanding records#4917
Use pretty printing context when expanding records#4917crtschin wants to merge 5 commits intohaskell:masterfrom
Conversation
ozkutuk
left a comment
There was a problem hiding this comment.
Thanks for taking care of this! I have a question and a few nitpicky remarks, but otherwise everything looks good.
| pretty (RecordInfoPat _ p) = pretty (printOutputable p) | ||
| pretty (RecordInfoCon _ e) = pretty (printOutputable e) | ||
| pretty (RecordInfoApp _ (RecordAppExpr _ _ fla)) | ||
| = hsep (map (pretty . printOutputable) fla) |
There was a problem hiding this comment.
Is there a fundamental reason to remove these, apart from the fact that printFieldName now requires a NamePprCtx? I used to find these helpful for debugging, so I think we can preserve them by inlining the old definition of printFieldName instead:
| pretty (RecordInfoPat _ p) = pretty (printOutputable p) | |
| pretty (RecordInfoCon _ e) = pretty (printOutputable e) | |
| pretty (RecordInfoApp _ (RecordAppExpr _ _ fla)) | |
| = hsep (map (pretty . printOutputable) fla) | |
| pretty (RecordInfoPat ss p) = pretty (stripOccNamePrefix (printOutputable ss)) <> ":" <+> pretty (printOutputable p) | |
| pretty (RecordInfoCon ss e) = pretty (stripOccNamePrefix (printOutputable ss)) <> ":" <+> pretty (printOutputable e) | |
| pretty (RecordInfoApp ss (RecordAppExpr _ _ fla)) | |
| = pretty (stripOccNamePrefix (printOutputable ss)) <> ":" <+> hsep (map (pretty . printOutputable) fla) |
There was a problem hiding this comment.
Good one! No this was me being lazy.
| nfp <- getNormalizedFilePathE uri | ||
| pragma <- getFirstPragma pId ideState nfp | ||
| CRR {crCodeActionResolve, nameMap, enabledExtensions} <- runActionE "ExplicitFields.CollectRecords" ideState $ useE CollectRecords nfp | ||
| (CRR {crCodeActionResolve, nameMap, enabledExtensions}, pprCtx) <- runActionE "ExplicitFields.CodeAction" ideState $ do |
There was a problem hiding this comment.
I admit this is nitpicking at this point, but why are we renaming the debug name of just at this specific use site? (i.e. from "ExplicitFields.CollectRecords" to "ExplicitFields.CodeAction"). If the idea is to differentiate between different such calls, I can see the value in that, but in that case we should rename all of them to unique names IMO. Also, technically there are two code actions registered by this plugin, so renaming just one of them to "ExplicitFields.CodeAction" might cause some confusion down the line, I imagine.
There was a problem hiding this comment.
I'll give each one a unique name 👍🏽
| formatOutputable :: Outputable a => NamePprCtx -> a -> Text | ||
| formatOutputable pprCtx a = T.pack $ printSDocQualifiedUnsafe pprCtx (ppr a) |
There was a problem hiding this comment.
The printOutputable function we used to use was doing some "unescaping" in the presence of printable escape sequences within double quotes. Is this not necessary anymore with formatOutputable?
There was a problem hiding this comment.
Perhaps helpful adding a regression test?
There was a problem hiding this comment.
It appears this is only relevant for pretty printing type-level literals. So the current implementation didn't suffer from show-polluted strings as it's all value-level expressions.
To be sure, I've added a test-case with some unicode fields and expressions. I've also refactored it a bit and moved this formatOutputable next to the functions and changed it so it also did the unescaping, as it's probably more-wildly applicable.
9967898 to
f4df874
Compare
f4df874 to
e54e898
Compare
Closes #4903.
There were some existing helpers for determining whether to print qualified/unqualified, use those when printing the expanded records.
An alternate approach would be to get the ranges from the expressions and use those to extract the expressions from the source files. That would have the benefit that record expansions with erroneous expressions would be nicely transplanted, but I think that would also require detaching
CollectRecordsfrom type checking, so I didn't pursue.