conjure-cp / conjure

Conjure: The Automated Constraint Modelling Tool
Other
96 stars 20 forks source link

Escape more characters in JSON output. Fixes #517 #519

Closed stylpe closed 1 year ago

stylpe commented 2 years ago

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 commented 2 years ago

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)

ozgurakgun commented 2 years ago

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
    |             ^^^^^^^^^
stylpe commented 2 years ago

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 commented 2 years ago

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

Screenshot_20220630-120138~2

stylpe commented 2 years ago

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.

ozgurakgun commented 2 years ago

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

stylpe commented 2 years ago

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])
ozgurakgun commented 2 years ago

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.

stylpe commented 2 years ago

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 commented 2 years ago

This time it should be OK!

ozgurakgun commented 1 year ago

Thanks for the contribution @stylpe!