conjure icon indicating copy to clipboard operation
conjure copied to clipboard

Escape more characters in JSON output. Fixes #517

Open stylpe opened this issue 2 years ago • 10 comments

#517

Disclaimer: This was implemented entirely using GitHub Codespaces (without sufficient RAM to build the whole project) on my mobile phone during toilet breaks with zero experience in Haskell. Please be gentle!

stylpe avatar Jun 29 '22 19:06 stylpe

Ah, small mistake in the commit message, I meant to note that it only addresses the first point about the backslashes.

Also, I haven't checked if there's anywhere else where escaping is performed, which could lead to double escaping. Trusting the unit tests will catch that. (Maybe I should add my own unit test even)

stylpe avatar Jun 29 '22 19:06 stylpe

Can you see the CI output @stylpe ? After clicking a few times, I get to this page: https://dev.azure.com/conjure-testing/conjure/_build/results?buildId=442&view=logs&jobId=b227660a-cc34-513e-56b6-a239a911c194&j=b227660a-cc34-513e-56b6-a239a911c194&t=778185b6-24a8-5c4e-9ea7-ecd373f7924f

Which gives a compilation error:

/home/vsts/work/1/s/src/Conjure/Language/Pretty.hs:124:13: error:
    • Variable not in scope: singleton :: Char -> Text
    • Perhaps you meant one of these:
        ‘T.singleton’ (imported from Data.Text),
        ‘V.singleton’ (imported from Data.Vector),
        ‘M.singleton’ (imported from Data.HashMap.Strict)
    |
124 | jsonEsc c = singleton c
    |             ^^^^^^^^^

ozgurakgun avatar Jun 30 '22 08:06 ozgurakgun

Yep, already made one amendment based on the CI logs :)

The error message is particularly helpful this time, it correctly suggests T.singleton as a fix! Will commit and push in a moment.

stylpe avatar Jun 30 '22 09:06 stylpe

And look, the language service finished installing overnight so I can see it in the editor now!

Screenshot_20220630-120138~2

stylpe avatar Jun 30 '22 10:06 stylpe

Ha, speaking of insufficient RAM, you might need to size up the builders:

ghc.exe: getMBlocks: VirtualAlloc MEM_COMMIT failed: The paging file is too small for this operation to complete.

stylpe avatar Jun 30 '22 10:06 stylpe

The windows builder on Azure has been unreliable, yes. The linux/max ones should work fine I am hoping.

ozgurakgun avatar Jun 30 '22 11:06 ozgurakgun

Here's a minimal failing repro model, but I'm not 100% sure where to add it, is tests/pretty_print/issues/ ok?

$ The variant triggers refinement output that is vulnerable to the bug
find x : variant {i:int(1..3),b:bool}
$ The backslash at the end of the next line doesn't get eacaped, producing "... /\"
such that active(x,i) -> x[i]>2 /\
x[i]<5

$ Simpler cases that fail since `\ ` isn't a valid JSON escape sequence
find y:bool such that y /\ true
find z:bool such that and([z,true])

stylpe avatar Jun 30 '22 16:06 stylpe

The parse_print directory is quite specialised for testing the roundtrip: parse/print/parse is the same as parse.

You can put it under tests/custom/issues/519 if you like? You'll have to put a run.sh file in there as well.

ozgurakgun avatar Jun 30 '22 16:06 ozgurakgun

I learned how to use git rebase -i, so I fixed up the branch a bit. The added test is a blind commit again, so relying on CI for feedback, otherwise I consider this ready!

stylpe avatar Jul 04 '22 18:07 stylpe

This time it should be OK!

stylpe avatar Sep 13 '22 18:09 stylpe

Thanks for the contribution @stylpe!

ozgurakgun avatar Nov 03 '22 09:11 ozgurakgun