GHC: Ticket #4430: Better support for resolving infix expressions in template haskell
http://ghc.haskell.org/trac/ghc/ticket/4430
<p>
Consider writing a quasiquoter to parse haskell (for example, the <tt>parseHaskell</tt> quasiquoter mentioned in Part D of <a class="ext-link" href="http://hackage.haskell.org/trac/ghc/blog/Template%20Haskell%20Proposal"><span class="icon"></span>Simon's TH blog post</a>
</p>
<p>
How is the quasiquoter supposed to handle infix expressions, such as
</p>
<pre class="wiki">foo = [parseHaskell| 3 * 5 + 4 |]
</pre><p>
In order to parse this expression properly, the quasiquoter needs to know the fixities of the operators mentioned, and this information is only available at the call site. We could try to use <tt>reify</tt>, but according to the description of "what reify sees" in the above-linked blog post, this would fail if, for example if the fixity is defined later in the file than the splice is done. Another obstacle to the <tt>reify</tt> approach is <a class="closed ticket" href="http://ghc.haskell.org/trac/ghc/ticket/4429" title="feature request: Ability to specify the namespace in mkName (closed: fixed)">#4429</a>
</p>
<p>
For context, the Hackage package <a class="ext-link" href="http://hackage.haskell.org/package/haskell-src-meta"><span class="icon"></span>haskell-src-meta</a> provides a haskell parser which produces TH, similar to the <tt>parseHaskell</tt> quasiquoter discussed above. This package does not, in general, parse infix expressions correctly.
</p>
<p>
One option for supporting fixities better is to improve reify. In particular, reify could be made to return an operator's fixity even if typechecking hasn't been performed.
</p>
<p>
A second option would be to move responsibility of fixity parsing away from the quasiquoter and back to GHC, by adding the following constructor (or similar) to template haskell's <tt>Exp</tt> datatype:
</p>
<pre class="wiki">...
| UnresolvedInfixE [(Exp, Exp)] Exp
...
</pre><p>
with the intention that <tt>UnresolvedInfixE [("a","+"), ("b", "*")] "c"</tt> (here I take strings to be expressions) denote the (unparenthesised) expression <tt> a + b * c </tt> with the expectation that GHC will apply the correct fixities when splicing.
</p>
en-usGHChttp://ghc.haskell.org/trac/ghc/chrome/site/ghc_logo.png
http://ghc.haskell.org/trac/ghc/ticket/4430
Trac 1.0.1simonpjMon, 25 Oct 2010 08:45:57 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:1
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:1
<p>
Very good point. GHC's own strategy is very like your second option. The parser parses all infix applications as left-associative (I think) with no priorities. Then the renamer re-associates everythign based on priorities.
</p>
<p>
All TH-generated code goes through the renamer so the existing operator-precedence re-association code will do the job nicely. So this looks to me like a more attractive route than expecting the quasi-quoter to do it, which is in general very hard. (Hard because even knowing which operator is meant by <tt>(***)</tt> pretty much means implementing a rename, dealing with imports, shadowing, and whatever.)
</p>
<p>
The renamer is not re-associating TH expressions at the moment, because when converting from TH syntax to <tt>HsSyn</tt> the conversion function adds <tt>HsPar</tt> parens around every operator application, and the renamer respects <tt>HsPar</tt> parens of course.
</p>
<p>
In short, I quite like your <tt>UnresolveInfixE</tt> solution. It's different to GHC's design, which has
</p>
<pre class="wiki">data LHsExpr id
= ...
| OpApp (LHsExpr id) -- left operand
(LHsExpr id) -- operator
Fixity -- Renamer adds fixity; bottom until then
(LHsExpr id) -- right operand
| HsPar (LHsExpr id)
...
</pre><p>
plus the expectation that all <tt>OpApp</tt> are unresolved until after renaming, except where guided by <tt>HsPar</tt>. But that's probably less appropraite for TH.
</p>
<p>
I'd welcome more ideas.
</p>
<p>
Simon
</p>
TicketreinerpFri, 05 Nov 2010 08:05:47 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:2
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:2
<p>
What is the status of this? I would be willing (and, I think, capable) to write a patch in the next few days to add the <tt>UnresolvedInfixE</tt> constructor, as I wrote it above, to Template Haskell. Would that be useful to do, or should I just wait for you to implement all of the "New directions in Template Haskell"?
</p>
<p>
Reiner
</p>
TicketsimonpjFri, 05 Nov 2010 10:26:14 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:3
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:3
<p>
Reiner, I think it's quite orthogonal to all the "new directions" stuff, so do not wait for that. I think it'd be great if you could do it.
</p>
<p>
The first thing is to write down exactly what the proposal <em>is</em>. What can users expect? For example if they write
</p>
<pre class="wiki"> x <- [| a + b * c |]
</pre><p>
can will the <tt>TH.Exp</tt> bound to <tt>x</tt> be correctly associated? (Answer: yes.) What about this?
</p>
<pre class="wiki"> x <- return (UnresolvedInfixE ... UnresolvedInfixE ...)
</pre><p>
Answer, obviously no.
</p>
<p>
Is any arbitrary tree of <tt>UnresolvedInfixE</tt> applications ok? Eg
</p>
<pre class="wiki"> UnresolvedInfixE
(UnresolvedInfixE e1 op1 e2)
op2
(UnresolveInfixE e3 op3 e4)
</pre><p>
I think this should be OK, and should be correctly re-associated. But GHC's renamer currently expects operators to show up left-associated (because that's what the parser does). You'd need to look at the renamer to check it would do the Right Thing for arbitrary trees of <tt>UnresolvedInfixE</tt>.
</p>
<p>
What happens when there's an ordinary <tt>InfixE</tt> mixed in? (Answer: don't reassociate.)
</p>
<p>
Are sections allowed for <tt>UnresolvedInfixE</tt>? In <tt>InfixE</tt> the arguments are <tt>(Maybe Exp)</tt> to allow sections, but I think we don't what that here.
</p>
<p>
Anyway, you get the idea. You could use the Trac wiki to write out the design, so that it's of longer-term utility than an ephemeral email. Then send the proposal to the libraries list.
</p>
<p>
Meanwhile, you can get hacking; it won't be too hard. Thank you!
</p>
<p>
Simon
</p>
TicketreinerpSat, 06 Nov 2010 05:54:36 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:4
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:4
<p>
I have written up a design spec at <a class="wiki" href="http://ghc.haskell.org/trac/ghc/wiki/UnresolvedInfixExpressions">UnresolvedInfixExpressions</a>. I'm unsure about some parts of the design; in particular, see the last two sections.
</p>
TicketreinerpSat, 06 Nov 2010 05:55:55 GMTowner set
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:5
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:5
<ul>
<li><strong>owner</strong>
set to <em>reinerp</em>
</li>
</ul>
TicketsimonpjTue, 09 Nov 2010 13:04:50 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:6
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:6
<p>
It all seems rather complicated to me. What's wrong with
</p>
<pre class="wiki">data Exp = ...
| UnresolvedInfixE
Exp -- Left operand
Exp -- Operator
Exp -- Right operand
</pre><p>
with the understanding that any tree of adjacent <tt>UnresolvedInfixE</tt> constructors is re-associated into correctly-associated uses of <tt>InfixE</tt>? The data type is simpler. The specification is simpler. The implementation is simpler.
</p>
<p>
Simon
</p>
TicketreinerpTue, 09 Nov 2010 14:15:45 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:7
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:7
<p>
That alone would not be sufficient. We would also need to add a <tt>Paren</tt> data-constructor, in order to distinguish the following two expressions:
</p>
<pre class="wiki">[parseHaskell| (a * b + c) - d / e |]
[parseHaskell| a * b + c - d / e |]
</pre><p>
(In the first, we want to reassociate within the brackets, and outside the brackets, but not across the brackets.)
</p>
<p>
For completeness, we would also like to support unresolved <em>sections</em>, such as
</p>
<pre class="wiki">[parseHaskell| (a * b *) |]
[parseHaskell| (* a * b) |]
</pre><p>
If <tt>*</tt> is left infix, then only the first of these two is valid; if it is right infix, then only the second of the two is valid.
</p>
<p>
So we might end up with
</p>
<pre class="wiki">data Exp = ...
| UnresolvedInfixE
(Maybe Exp) -- Left
Exp -- Operator
(Maybe Exp) -- Right
| Paren Exp
</pre><p>
with the understanding that any tree of adjacent <tt>UnresolvedInfixE</tt> constructors is reassociated (but of course we don't reassociate across parentheses). I could do that.
</p>
<p>
(My initial objection to this is simply that we're adding the <tt>Paren</tt> constructor, whose only purpose is to signal "don't reassociate <tt>UnresolvedInfixE</tt> trees across me" -- and then this leads me to try and shove the entire tree-to-be-reassociated into one constructor -- say, something like
</p>
<pre class="wiki">data Exp = ...
| UnresolvedInfixE
[Exp] -- operators, in order
[Maybe Exp] -- operands, in order
</pre><p>
I don't have a strong preference either way.)
</p>
TicketsimonpjWed, 10 Nov 2010 08:16:53 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:8
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:8
<p>
Good point. I've thought about this a bit. I suggest we use the
</p>
<pre class="wiki">data Exp = ...
| UnresolvedInfixE
Exp -- Left
Exp -- Operator
Exp -- Right
| Parens Exp
...
</pre><p>
Reasoning
</p>
<ul><li>It's simple, direct, and understandable.
</li><li>It is what GHC itself already does
</li><li>It is easy on the chap <em>constructing</em> the code. A parser is only going to see one infix operator at a time, so building that list of operands (the alternative approach) would be awkward.
</li><li><tt>UnresolvedInfixE</tt> doesn't need to support sections, because <tt>InfixE</tt> can do the job. If you write
<pre class="wiki"> InfixE
Nothing
op1
(UnresolvedInfixE e1 op2 e2)
</pre></li></ul><p>
then there should be an error if the fixity of <tt>op1</tt> and <tt>op2</tt> don't relate correctly. To avoid this check, use <tt>Parens</tt>, or <tt>InfixE</tt> for the inner constructor. (Of course the docs should state this.)
</p>
<p>
I think that leads to a simple, understandable design.
</p>
TicketreinerpThu, 11 Nov 2010 10:13:56 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:9
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:9
<p>
That seems fair enough. Unless anything else comes up, I'll go ahead and do it this way.
</p>
<p>
One remaining point, which is probably easiest just to ignore for now, is: once the <tt>UnresolvedInfixE</tt> and <tt>Parens</tt> constructors are supported, the <tt>InfixE</tt> constructor is not strictly necessary (ignoring sections): any application
</p>
<pre class="wiki"> InfixE left op right
</pre><p>
can be rewritten as
</p>
<pre class="wiki"> Parens (UnresolvedInfixE
(Parens left)
op
(Parens right)
)
</pre><p>
It's just a bit odd.
</p>
TicketsimonpjThu, 11 Nov 2010 17:47:33 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:10
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:10
<p>
Yes, that is true, but we can only resolve it by removing <tt>InfixE</tt> which would be more disruptive. In retrospect the new story might have been easier.
</p>
TicketiglooSat, 13 Nov 2010 21:06:42 GMTmilestone set
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:11
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:11
<ul>
<li><strong>milestone</strong>
set to <em>7.2.1</em>
</li>
</ul>
TicketreinerpTue, 19 Jul 2011 14:14:26 GMTstatus changed
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:12
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:12
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>patch</em>
</li>
</ul>
<p>
I've attached some patches which implement <tt>UnresolvedInfixE</tt> and <tt>UnresolvedInfixP</tt> (exactly the same discussion applies to infix patterns as did to infix expressions, except we don't have to worry about sections). The approach is following the most recent plan discussed above.
</p>
<p>
The attached patch changes the behaviour of <tt>InfixP</tt>, as the previous behaviour seems almost certainly a bug. In particular, GHC would reassociate left-biased <tt>InfixP</tt> splices, (unlike <tt>InfixE</tt> splices, which it would leave alone). This actually broke round-tripping of pattern splices: for example, the pattern match in module B below currently succeeds:
</p>
<pre class="wiki">{-# LANGUAGE TemplateHaskell #-}
module A where
import Language.Haskell.TH
import Language.Haskell.TH.Quote
data Tree = N
| Tree :+ Tree
infixr :+
p1 = QuasiQuoter undefined (const [p| (N :+ N) :+ N |]) undefined undefined
{-# LANGUAGE QuasiQuotes #-}
module B where
import A
f = case N :+ (N :+ N) of
[p1|unused|] -> True
</pre><p>
The attached patch changes this so that <tt>InfixP</tt> trees are never reassociated, which is consistent with the behaviour of <tt>InfixE</tt>.
</p>
<p>
For completeness, I also considered adding unresolved infix constructors for types, but decided against it: there isn't currently an <tt>InfixT</tt> constructor after all, and <tt>TypeOperators</tt> is a language extension.
</p>
<p>
My patches add an unexpected failure (TH_pragma), the failure being the addition of some unnecessary parentheses in <tt>-ddump-splices</tt>.
</p>
<p>
Cheers,
Reiner
</p>
TicketreinerpTue, 19 Jul 2011 14:15:09 GMTattachment set
http://ghc.haskell.org/trac/ghc/ticket/4430
http://ghc.haskell.org/trac/ghc/ticket/4430
<ul>
<li><strong>attachment</strong>
set to <em>compiler.patch</em>
</li>
</ul>
TicketreinerpTue, 19 Jul 2011 14:15:16 GMTattachment set
http://ghc.haskell.org/trac/ghc/ticket/4430
http://ghc.haskell.org/trac/ghc/ticket/4430
<ul>
<li><strong>attachment</strong>
set to <em>template-haskell.patch</em>
</li>
</ul>
TicketreinerpTue, 19 Jul 2011 14:15:24 GMTattachment set
http://ghc.haskell.org/trac/ghc/ticket/4430
http://ghc.haskell.org/trac/ghc/ticket/4430
<ul>
<li><strong>attachment</strong>
set to <em>testsuite.patch</em>
</li>
</ul>
TicketreinerpTue, 19 Jul 2011 14:18:17 GMTowner changed
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:13
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:13
<ul>
<li><strong>owner</strong>
changed from <em>reinerp</em> to <em>igloo</em>
</li>
</ul>
TicketsimonpjTue, 19 Jul 2011 20:36:30 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:14
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:14
<p>
Looks good to me.
</p>
<ul><li>I think the name "<tt>UnresolvedInfixE</tt>" is rather long and clunky. I'd be inclined to say "<tt>UInfixE</tt>" and explain the terminology in the comment. What do you think?
</li></ul><ul><li>I like the long comment in <tt>TH.hs</tt>, but I think it'd be better in <tt>TH/Syntax.hs</tt> where the constructors are declared. Also could you use the <tt>Note [blah]</tt> format described here <a class="ext-link" href="http://hackage.haskell.org/trac/ghc/wiki/Commentary/CodingStyle"><span class="icon"></span>http://hackage.haskell.org/trac/ghc/wiki/Commentary/CodingStyle</a>?
</li></ul><ul><li>In that comment you say "This special handling for sections is the exception to the previous point". I don't understand this comment. The use of <tt>InfixE</tt> in sections is not reassociated, just like any other use of <tt>InfixE</tt>. Seems fine to me!
</li></ul><ul><li>In <tt>Convert.cvtOpAppP</tt> could you add a comment explaining <em>why</em> the chain must be re-associated. It's because GHC's renamer expects to see operator applications in a particular form; better say what that is. Are you sere that <tt>cvtOpAppP</tt> puts an arbitrary tree into a list form? I have not looked very carefully but I'm not sure it does.
</li></ul><p>
Thanks
</p>
<p>
Simon
</p>
TicketreinerpWed, 20 Jul 2011 04:17:44 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:15
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:15
<p>
Thanks for reviewing.
</p>
<ul><li>Sure, I'll change the name.
</li></ul><ul><li>I'll move the comment to TH/Syntax.hs. I'll adopt the style you linked to, but I'll leave the <tt>$infix #infix#</tt> in, because that inserts the note into Haddock and allows for hyperlinks as appropriate.
</li></ul><ul><li>I agree it's not reassociated, but there is some form of special handling. Here's the problem. In Haskell, we have:
</li></ul><pre class="wiki">infixl +
(1 + 2 +) -- valid, and parses as ((1 + 2) +)
(+ 1 + 2) -- invalid
(+ (1 + 2)) -- valid
</pre><blockquote>
<p>
We need some way in Template Haskell to distinguish between the second and third cases. With my patch, we get the following:
</p>
</blockquote>
<pre class="wiki">InfixE Nothing plus (UInfixE 1 plus 2) -----> (+ 1 + 2)
InfixE Nothing plus (Parens $ UInfixE 1 plus 2) -----> (+ (1 + 2))
</pre><blockquote>
<p>
So the <tt>InfixE</tt> is not reassociated, but whether it causes an error depends on the <tt>UInfixE</tt> inside it (and in <tt>Convert.cvtl</tt> we see fully-applied <tt>InfixE</tt> constructors put parentheses around their arguments, whereas <tt>InfixE</tt> sections don't). So it's somewhat against the spirit of "GHC won't play any tricks when you have an <tt>InfixE</tt>", and should be documented somehow, even if it's not an exception.
</p>
</blockquote>
<ul><li>Yes, I was wondering about how much I should document. I could add a brief comment explaining how <tt>cvtOpAppP</tt> works, along the following lines.
</li></ul><pre class="wiki">We can see that @cvtOpAppP@ is correct as follows. The inductive hypothesis
is that @cvtOpAppP x op y@ is left-biased, provided @x@ is. It is clear that
this holds for both branches (of @cvtOpAppP@), provided we assume it holds for
the recursive calls to @cvtOpAppP@.
</pre><blockquote>
<p>
I originally had a simpler (more obviously correct) implementation, but in the worst case (starting with a completely right-biased tree) it had quadratic complexity in the size of the input tree, whereas the current implementation is linear.
</p>
</blockquote>
<p>
Cheers,
Reiner
</p>
TicketsimonpjWed, 20 Jul 2011 07:40:34 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:16
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:16
<p>
Re bullet 3, I see now. (Please put that in the big comment, or whatever we agree.)
</p>
<p>
Perhaps we should just say that
</p>
<pre class="wiki">InfixE Nothing plus (UInfixE 1 plus 2) -----> (+ (1 + 2))
</pre><p>
There is only one (legal) meaning for what the data structre represents, so let's just pick that. That's what I thought the idea was, and it's why I was confused. Why generate a possibly bogus term if there's an obvious legal meaning?
</p>
<p>
After all we have that
</p>
<pre class="wiki">InfixE 0 * (UInfixE 1 + 2) -----> (0 * (1 + 2))
</pre><p>
(that is, we parenthesise the arguments of <tt>InfixE</tt> regardless of their form) so it seems entirely consistent for sections too.
</p>
<p>
Re last bullet:
</p>
<ul><li>I think it would be worth documenting the left-biased form that GHC's renamer expects. There is a bit in <tt>RnTypes.lhs</tt> line 280 or so, but it lacks a <tt>Note [blah]</tt> heading -- you could add that and expand the note.
</li><li>I was thinking of a comment to say "this is why <tt>cvtOpAppP</tt> has to reassociate the tree. Documenting the implementation (to convince oneself that it does indeed do so) is a separate matter. Your comment on that looks fine; except we need to know that the return value of <tt>cvtl</tt> satisfies the invariant, since that supplies the initial arg to <tt>cvtOpAppP</tt>.
</li></ul>
TicketreinerpWed, 20 Jul 2011 08:49:54 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:17
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:17
<p>
Ok, I agree that
</p>
<pre class="wiki">InfixE Nothing plus (UInfixE 1 plus 2) -----> (+ (1 + 2))
</pre><p>
is the right thing to do. But we need a representation for <tt>(+ 1 + 2)</tt>. I think we should use
</p>
<pre class="wiki">data Exp =
...
UInfixE (Maybe Exp) Exp (Maybe
Exp)
...
</pre><p>
Using <tt>InfixE</tt> just doesn't work.
</p>
TicketsimonpjWed, 20 Jul 2011 09:01:28 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:18
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:18
<p>
Sorry, don't understand. Why do we need a rep for an illegal syntax tree?
</p>
TicketreinerpWed, 20 Jul 2011 09:22:26 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:19
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:19
<p>
Because our hypothetical quasiquoter doesn't know that it's illegal (you need to know the operator fixities for that). And even though there is at most one legal way to resolve the expression <tt>(1 + 2 +)</tt>, namely to <tt>((1 + 2) +)</tt>, it would not be acceptable for our quasiquoter to do this conversion:
</p>
<ul><li>firstly, this turns some illegal haskell expressions into legal ones, which in itself is not a huge problem
</li><li>more importantly, some (illegal, but plausible) expressions will be given surprising meanings. For instance, consider <tt>(* 2 + 1)</tt>. It is illegal in haskell, but one might think it should mean <tt>(\x -> x * 2 + 1)</tt>. However, the above conversion would turn it into <tt>(* (2 + 1))</tt>, which is completely different.
</li></ul>
TicketsimonpjWed, 20 Jul 2011 10:41:17 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:20
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:20
<p>
Actually, now I look at it, if you check <tt>HsExpr.rnSection</tt> you'll see that (in <tt>HsSyn</tt>),
</p>
<pre class="wiki">SectionR + (OpApp a + b)
</pre><p>
is rejected (by <tt>checkSectionPrec</tt>). So, the representation for (+ 1 + 2) can simply generate the above, as it will, and then GHC will reject it, as it should.
</p>
<p>
In short, no need for <tt>UInfixE</tt>.
</p>
TicketreinerpThu, 21 Jul 2011 01:29:15 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:21
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:21
<p>
I'm confused. My patch implements the following:
</p>
<pre class="wiki">InfixE Nothing + (UInfixE a + b) ---> SectionR + (OpApp a + b)
InfixE Nothing + (ParensE $ UInfixE a + b) ---> SectionR + (HsPar $ OpApp a + b)
</pre><p>
This means that we can use the first to represent <tt>(+ a + b)</tt> and the second to represent <tt>(+ (a + b))</tt>. All good, we can represent the things we need to. The problem is that <tt>InfixE</tt> behaves quite differently for sections than it does for non-sections (the special handling I discussed in commment 15).
</p>
<p>
It seems there are two ways to proceed:
</p>
<ol><li>Leave the situation with sections as implemented in my patch, so <tt>UInfixE</tt> doesn't need to worry about sections, at the cost of some special cases in <tt>InfixE</tt>
</li><li>Remove the special cases of <tt>InfixE</tt> by making the <tt>UInfixE</tt> constructor handle sections:
<pre class="wiki">data Exp =
...
UInfixE (Maybe Exp) Exp (Maybe Exp)
...
</pre></li></ol><p>
Which are you suggesting?
</p>
TicketreinerpThu, 21 Jul 2011 07:59:35 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:22
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:22
<p>
By the way, I now think the second way is the right way to proceed. It doesn't have special cases or surprising behaviour.
</p>
TicketsimonpjThu, 21 Jul 2011 16:17:06 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:23
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:23
<p>
I'm confused too! I see no special cases for <tt>InfixE</tt> in your patch in <tt>Convert</tt> (which converted TH syn to Hs syn. Can you point me to where it is (file/line number)?
</p>
<p>
My only concern was your <em>comment</em>, where you mention a special case that I cannot see.
</p>
<p>
The actual code looks fine to me! So I currently espouse option 1 on the grounds that it's simpler, and does everything we want.
</p>
<p>
Sorry to be dense
</p>
<p>
Simon
</p>
TicketreinerpFri, 22 Jul 2011 08:25:36 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:24
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:24
<p>
Sorry, I guess I didn't explain my thoughts clearly. I'll give it another shot.
</p>
<h2 id="Whatisthespecialcase">What is the special case</h2>
<p>
The special case is not explicit, but it's there. In Convert.cvtl, compare the cases for <tt>InfixE (Just x) s (Just y)</tt> and <tt>InfixE Nothing s (Just y)</tt> (this is around line 488). We produce the following output:
</p>
<pre class="wiki">InfixE (Just x) s (Just y) ---> HsPar (OpApp (HsPar x) s (HsPar y))
InfixE Nothing s (Just y) ---> HsPar (SectionR s y)
</pre><p>
Note that the former puts parentheses around the <tt>y</tt>, whereas the latter doesn't. This means that when the renamer runs, the <tt>OpApp</tt>s coming from <tt>y</tt> will meet the <tt>SectionR</tt> in the second case, but they won't meet the <tt>OpApp</tt> in the first case. That is:
</p>
<pre class="wiki">InfixE (Just x) s (Just (UInfix y op z))
---> HsPar (OpApp
(HsPar x)
s
(HsPar (OpApp y op z)))
(the OpApps don't meet, so they aren't reassociated)
InfixE Nothing s (Just (UInfix y op z))
---> HsPar (SectionR
s
(OpApp y op z))
(the OpApp meets the SectionR, so the renamer will throw an error if @op@ is left-infix)
</pre><p>
Of course, the same applies the the <tt>SectionL</tt> case.
</p>
<h2 id="Option1asummary">Option 1, a summary</h2>
<p>
For comparison with the next section, here is what Option 1 does:
</p>
<pre class="wiki">data Exp =
...
| InfixE (Maybe Exp) Exp (Maybe Exp)
| UInfixE Exp Exp Exp
...
</pre><p>
and <tt>Convert.cvtl</tt> does this:
</p>
<pre class="wiki">InfixE (Just x) s (Just y) ---> HsPar (OpApp (HsPar x) s (HsPar y))
InfixE Nothing s (Just y) ---> HsPar (SectionR s y )
InfixE (Just x) s Nothing ---> HsPar (SectionL x s )
UInfixE x s y ---> (OpApp x s y )
</pre><h2 id="WhatOption2wouldlooklike">What Option 2 would look like</h2>
<p>
In Option 2, we would have
</p>
<pre class="wiki">data Exp =
...
| InfixE (Maybe Exp) Exp (Maybe Exp)
| UInfixE (Maybe Exp) Exp (Maybe Exp)
...
</pre><p>
and the relevant parts of <tt>Convert.cvtl</tt> would be
</p>
<pre class="wiki">InfixE (Just x) s (Just y) ---> HsPar (OpApp (HsPar x) s (HsPar y))
InfixE Nothing s (Just y) ---> HsPar (SectionR s (HsPar y))
InfixE (Just x) s Nothing ---> HsPar (SectionL (HsPar x) s )
UInfixE (Just x) s (Just y) ---> (OpApp x s y )
UInfixE Nothing s (Just y) ---> (SectionR s y )
UInfixE (Just x) s Nothing ---> (SectionL x s )
</pre><h2 id="Whyoption1issurprising">Why option 1 is surprising</h2>
<p>
Perhaps the comment I wrote was too strongly worded in saying that there was a special case. But Option 1 is still surprising.
</p>
<p>
Consider:
</p>
<pre class="wiki">-- with option 1
InfixE (Just x) s (Just y) -- doesn't care if x and y are UInfixE's, i.e. no fixity errors, no reassociating
InfixE Nothing s (Just y) -- if y is a UInfixE with a left-infix operator, the renamer will throw a fixity error
InfixE Nothing s (Just (Parens y)) -- doesn't care if y is a UInfixE
-- with option 2
InfixE (Just x) s (Just y) -- doesn't care if x and y are UInfixE's
InfixE Nothing s (Just y) -- doesn't care if y is a UInfixE
UInfixE Nothing s (Just y) -- if y is a UInfixE with a left-infix operator, the renamer will throw a fixity error
</pre><p>
I find option 1 surprising, because I don't expect <tt>InfixE</tt> to "look inside" its argument. And for non-section uses of <tt>InfixE</tt>, it doesn't, but for sections it does! (I can't find a better description for what happens in the above examples than "doesn't care" versus "looks inside", which is unfortunate, and perhaps a source of the confusion. I hope it's clear what I mean.)
</p>
<p>
Sorry for dragging things out so long.
</p>
<p>
Reiner
</p>
TicketsimonpjFri, 22 Jul 2011 08:44:44 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:25
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:25
<p>
OK now I understand.
</p>
<ul><li>TH could have had three constructors (for left section, right section, and infix) like GHC, but it didn't. But I <em>think</em> of the three cases as three separate constructors.
</li><li>Once you think like that, there is no more "looking inside".
</li></ul><p>
Bottom line: I'd prefer to stick with Option 1. Could you do the other things we agree and send final patches? Thanks!
</p>
TicketreinerpSat, 23 Jul 2011 14:45:50 GMTattachment set
http://ghc.haskell.org/trac/ghc/ticket/4430
http://ghc.haskell.org/trac/ghc/ticket/4430
<ul>
<li><strong>attachment</strong>
set to <em>compiler-0001-Add-support-for-unresolved-infix-expressions-and-pat.patch</em>
</li>
</ul>
TicketreinerpSat, 23 Jul 2011 14:46:03 GMTattachment set
http://ghc.haskell.org/trac/ghc/ticket/4430
http://ghc.haskell.org/trac/ghc/ticket/4430
<ul>
<li><strong>attachment</strong>
set to <em>testsuite-0001-Test-unresolved-infix-expressions-and-patterns.patch</em>
</li>
</ul>
TicketreinerpSat, 23 Jul 2011 14:46:12 GMTattachment set
http://ghc.haskell.org/trac/ghc/ticket/4430
http://ghc.haskell.org/trac/ghc/ticket/4430
<ul>
<li><strong>attachment</strong>
set to <em>testsuite-0002-Accept-redundant-parens-in-TH_pragma.patch</em>
</li>
</ul>
TicketreinerpSat, 23 Jul 2011 14:46:27 GMTattachment set
http://ghc.haskell.org/trac/ghc/ticket/4430
http://ghc.haskell.org/trac/ghc/ticket/4430
<ul>
<li><strong>attachment</strong>
set to <em>template-haskell-0001-Add-support-for-unresolved-infix-expressions-and-pat.patch</em>
</li>
</ul>
TicketreinerpSat, 23 Jul 2011 14:47:52 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:26
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:26
<p>
Ok, I've attached new patches. I noticed a bug in the old patches, which I've fixed and documented under <tt>Note [Dropping constructors]</tt>.
</p>
<p>
Reiner
</p>
TicketsimonpjMon, 25 Jul 2011 13:22:53 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:27
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:27
<p>
Great. Can you say what the point of testsuite-0002-Accept-redundant-parens-in-TH_pragma.patch is? It seems to modify an existing test. Does the test not work without the mod? Or what?
</p>
TicketreinerpMon, 25 Jul 2011 13:27:58 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:28
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:28
<p>
It's as you say: the test fails without this patch.
</p>
<p>
The only changes I make are the addition of some (redundant) parentheses in the output of <tt>-ddump-splices</tt>. The cause for these extra parentheses is clear: in the <tt>InfixE</tt> case of <tt>Convert.cvtl</tt>, I added some extra <tt>HsPar</tt> constructors. In most cases, these extra parentheses are redundant, but I found it easiest simply to always add them.
</p>
Ticketsimonpj@…Wed, 27 Jul 2011 05:59:13 GMT
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:29
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:29
<p>
commit <a class="changeset" href="http://ghc.haskell.org/trac/ghc/changeset/921b1b3286d95fccb03ec6c31e8abd02fde1bab9/ghc" title="A bit of refactoring on handling HsPar and friends
This relates to Trac ...">921b1b3286d95fccb03ec6c31e8abd02fde1bab9</a>
</p>
<pre class="wiki">Author: Simon Peyton Jones <simonpj@microsoft.com>
Date: Wed Jul 27 06:21:43 2011 +0100
A bit of refactoring on handling HsPar and friends
This relates to Trac #4430 (infix expressions in TH),.
Mainly comments but a bit of code wibbling.
compiler/hsSyn/Convert.lhs | 85 +++++++++++++++++++++++++++-----------------
compiler/hsSyn/HsExpr.lhs | 77 ++++++++++++++++++++++++++-------------
compiler/hsSyn/HsPat.lhs | 39 ++++++++------------
compiler/hsSyn/HsTypes.lhs | 6 +---
compiler/hsSyn/HsUtils.lhs | 60 +++++++++++++++++++------------
5 files changed, 157 insertions(+), 110 deletions(-)
</pre>
TicketsimonpjWed, 27 Jul 2011 06:05:26 GMTstatus changed; resolution set
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:30
http://ghc.haskell.org/trac/ghc/ticket/4430#comment:30
<ul>
<li><strong>status</strong>
changed from <em>patch</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
</ul>
<p>
Plus
</p>
<pre class="wiki">commit 54d7c6beb2d2c6ec6c7b46f5f60935c162045d93
Author: Reiner Pope <reiner.pope@gmail.com>
Date: Sat Jul 23 16:15:41 2011 +1000
Add support for unresolved infix expressions and patterns
compiler/hsSyn/Convert.lhs | 106 +++++++++++++++++++++++++++++++++++++++++---
1 files changed, 99 insertions(+), 7 deletions(-)
</pre><p>
and in the template-haskell library
</p>
<pre class="wiki">commit 2539bc6b3a5b0e4c6ce12746bfefe709026cac2d
Author: Reiner Pope <reiner.pope@gmail.com>
Date: Sat Jul 23 16:13:17 2011 +1000
Add support for unresolved infix expressions and patterns.
Language/Haskell/TH.hs | 6 ++-
Language/Haskell/TH/Lib.hs | 15 ++++++++
Language/Haskell/TH/Ppr.hs | 17 ++++++++--
Language/Haskell/TH/Syntax.hs | 75 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 108 insertions(+), 5 deletions(-)
</pre><p>
All done, I think. Thanks reinerp!
</p>
<p>
Simon
</p>
Ticket