goldfirere
Support as-patterns in pattern synonyms with view patterns
I guess a bit of a WIP, Maybe needs neatening and tests
See https://github.com/goldfirere/th-desugar/issues/174
ok, I think I've addressed the comments, as well as added view pattern support everywhere (it fell out very naturally)
I've keps dsPatX around for a deprecation cycle if we care about that.
The bug here is that now dsPatOverExp return a less fallible pattern, having moved most of the matching on as-patterns into the subsequent let expression. The consequence of this is apparent in the following:
do
let xs = [1,2]
x@1 <- xs
pure x
This used to desugar to the equivalent of:
do
let xs = [1,2]
1 <- xs
let x = 1
pure x
But now it desugars to the equivalent of:
do
let xs = [1,2]
x <- xs
let 1 = x
pure x
Apart from it being incorrect, I do prefer the second one as there isn't any pattern -> expression transmutation happening.
OK. I could take a look, but there is currently a build failure in the patch, as caught by CI:
[ 9 of 14] Compiling Language.Haskell.TH.Desugar.Core ( Language/Haskell/TH/Desugar/Core.hs, /__w/th-desugar/th-desugar/dist-newstyle/build/x86_64-linux/ghc-9.2.2/th-desugar-1.14/build/Language/Haskell/TH/Desugar/Core.o, /__w/th-desugar/th-desugar/dist-newstyle/build/x86_64-linux/ghc-9.2.2/th-desugar-1.14/build/Language/Haskell/TH/Desugar/Core.dyn_o )
Language/Haskell/TH/Desugar/Core.hs:806:22: error:
Variable not in scope:
dsPatWithViewPatterns :: Pat -> q (DPat, t0 (DPat, DExp))
|
806 | (pat', subPats) <- dsPatWithViewPatterns pat
| ^^^^^^^^^^^^^^^^^^^^^
Language/Haskell/TH/Desugar/Core.hs:950:19: error:
Variable not in scope:
dsPatWithViewPatterns :: Pat -> q (DPat, [(DPat, DExp)])
|
950 | (pat', vars) <- dsPatWithViewPatterns pat
| ^^^^^^^^^^^^^^^^^^^^^
Ah, I didn't mean that I had uncovered an existing bug; I've introduced a new one. Before the "extra patterns" were designed/assumed to be infallible, but now they're not which leads to problems in do notation.
It shouldn't be the end of the world to fix, although it's tempting just to reinstate the old "no view patterns" behaviour.
It shouldn't be the end of the world to fix, although it's tempting just to reinstate the old "no view patterns" behaviour.
OK. For what it's worth, I would be fine with reinstating the old behavior, opening a separate issue to track the remaining tasks.
Ok, I reverted to the previous behaviour and opened https://github.com/goldfirere/th-desugar/issues/177
Thank you for taking the time for a thorough review @RyanGlScott!
For the tests, it would be really nice to be able to describe them like so
test_t175 :: [Assertion]
test_t175 =
fmap (uncurry (@=?))
$(do
Syn.lift =<< (traverse
(\(a, e) -> (,) <$> (sweeten @_ @[DDec] <$> (desugar =<< a)) <*> e)
[ ( [d| pattern P = (id -> ()) |]
, [d| pattern P = (\x -> case id x of () -> () -> ()) |]
)
, ( [d| pattern P x <- x@3 |]
, [d| pattern P x <- (\x -> case x of 3 -> x -> x) |]
)
, ([d| pattern P = ()|], [d| pattern P = ()|])
])
)
But with something instead of @=? which checks up to alpha-equivalence. I couldn't find anything in th-desugar for this kind of test. Would this be something worth adding or is there a simpler test-setup you'd recommend?
Most of the unit tests are located here:
https://github.com/goldfirere/th-desugar/blob/92c07bd88b74efae0d1b6b7bb5d52049723a009a/Test/Run.hs#L77-L154
Where each test_* function is located here:
https://github.com/goldfirere/th-desugar/blob/92c07bd88b74efae0d1b6b7bb5d52049723a009a/Test/Splices.hs#L136-L352
The problem with this approach is that it tests things by splicing in expressions. However, it's not entirely straightforward to define an expression containing a pattern synonym definition. You can't do something like let { pattern P = (id -> ()) } in f P = 42, as pattern synonyms must be defined at the top level.
One way we could accomplish this would be to define the pattern synonym definitions elsewhere, and then define expressions in terms of those pattern synonyms. We would need two copies of each pattern synonym: one using the original pattern synonym syntax, and another using the desugared syntax. We could define them in separate modules (similar to how we have both Test/Decs.hs and Test/DsDecs.hs) and import them qualified to avoid name clashes.
Testing alpha-equivalence would be another way to accomplish this, but nothing in th-desugar quite implements that. The closest that we have is the eqTH function, which checks that two things are Equal to each other after removing the numeric suffixes from each type variable.
Other life things have gotten in the way a bit! I would like to finalize this, but it's not at the top of my plate at the moment, sorry.
I think there's also a bug, along the lines of: (foo -> Left x) being transformed to (\a -> case foo a of Left x -> x) (something like that, where fallible matches on the rhs of a view pattern were transformed into infallible (runtime pattern match error) case analysis in the lhs. So very annoyingly we need to add a catch all case to these and handle everything in Maybe resultTuple (or some unboxed variant to avoid any allocation).
Thanks for the update! (I was worried that this was in a finished state and only being delayed due to figuring out how to add tests, in which case I could pick it up from there.)