unescape printable characters #3140
Conversation
kokobd
requested review from
jneira,
berberman and
pepeiborra
as code owners
|
Is there not a more appropriate printing method from GHC we can use? Or are we just only given ones that escape? |
I'm not aware of one. Note that |
|
Then I think the approach seems reasonable so long as we document it. |
Done. |
| @@ -288,9 +289,13 @@ instance Outputable SDoc where | |||
|
|
|||
| -- | Print a GHC value in `defaultUserStyle` without unique symbols. | |||
| -- | |||
| -- This is the most common print utility, will print with a user-friendly style like: `a_a4ME` as `a`. | |||
| -- This is the most common print utility, and it will print with a user-friendly style like: `a_a4ME` as `a`. | |||
There was a problem hiding this comment.
Maybe also include the fact that it unescapes characters? That's quite an important difference from other outputable functions. In fact... maybe we should use hlint to ban use of other outputable functions apart from this one?
There was a problem hiding this comment.
Maybe also include the fact that it unescapes characters?
Done.
maybe we should use hlint to ban use of other outputable functions apart from this one?
TBH I don't know if this is configurable. I think the scheme is to use printOutputable by default, and also allow other print functions to be used when needed.
There was a problem hiding this comment.
I mean, we can use hlint to ban functions, so we could do it. I don't know if it's a good idea, though.
| escapedTextParser = do | ||
| xs <- P.many (P.try stringLiteral) | ||
| x <- P.manyTill P.anySingle P.eof -- consume characters after the final double quote | ||
| pure $ concat xs ++ x |
There was a problem hiding this comment.
It feels a little weird that you handle the content before the opening quote in stringLiteral but the content after the closing quote outside it. Maybe make that consistent?
There was a problem hiding this comment.
I rewrote stringLiteral to not include any content outside of the double quotes. I think that's more reasonable.
| stringLiteral :: TextParser String | ||
| stringLiteral = do | ||
| before <- P.manyTill P.anySingle (P.char '"') -- include any character before the first double quote | ||
| inside <- P.manyTill P.charLiteral (P.char '"') |
There was a problem hiding this comment.
What about "hell\"ooo"? i.e. escaped quotes probably shouldn't stop us
There was a problem hiding this comment.
Hmm, from the tests it looks like you made this work but I'm not sure why it works...
There was a problem hiding this comment.
P.charLiteral calls lookAhead internally. And this is an example from the haddock of charLiteral:
stringLiteral = char '"' >> manyTill L.charLiteral (char '"')
| [ unescapeTest | ||
| ] | ||
|
|
||
| unescapeTest :: TestTree |
This should fix #3115. It works. I will add tests later if the idea is okay.
Maybe a more proper fix is to modify the
Outputableinstance forIfaceTyLitin GHC?Demo:
