<div dir="ltr"><div><div><div>Neil<br><br></div>I looked into your proposed change in more detail, and I think it is flawed because it is trying to map the annotation back to itself.<br><br></div>To start with we have a SrcSpan and the concrete AST value. We need to map the concrete constructor to the relevant annotation, which is of a different type.<br><br></div>One straightforward way of doing it is the following<br><div><br> data ApiAnnKey = AK SrcSpan String<br> deriving (Eq,Ord,Show)<br><br> getAnnotation :: Data a => Map.Map ApiAnnKey ApiAnn -> Located a -> Maybe ApiAnn<br> getAnnotation anns a = Map.lookup (mkApiAnnKey a) anns<br><br> mkApiAnnKey :: (Data e) => (Located e) -> ApiAnnKey<br> mkApiAnnKey (L l e) = AK l (gconname e)<br><br> gconname :: Data a => a -> String<br> gconname = (\t -> showConstr . toConstr $ t)<br><br></div><div>Note that showConstr is just an alias to the record selector for the Data.Data.Constr, so it is fast and returns a constant string.<br><br></div><div>In this scenario I am not sure that there is a benefit to splitting the ApiAnn type into multiple separate ones.<br><br></div><div>Also, it only relies on the AST being an instance of Data, which already holds.<br></div><div><br><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 1, 2014 at 6:37 PM, Neil Mitchell <span dir="ltr"><<a href="mailto:ndmitchell@gmail.com" target="_blank">ndmitchell@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I was getting a bit lost between the idea and the implementation. Let<br>
me try rephrasing the idea in my own words.<br>
<br>
The goal: Capture inner source spans in AST syntax nodes. At the<br>
moment if ... then ... else ... captures the spans [if [...] then<br>
[...] else [...]]. We want to capture the spans for each keyword as<br>
well, so: [{if} [...] {then} [...] {else} [...]].<br>
<br>
The proposal: Rather than add anything to the AST, have a separate<br>
mapping (SrcSpan,AstCtor) to [SrcSpan]. So you give in the SrcSpan<br>
from the IfThenElse node, and some token for the IfThenElse<br>
constructor, and get back a list of IfThenElse for the particular<br>
keyword.<br>
<br>
I like the proposal because it adds nothing inside the AST, and<br>
requires no fresh invariants of the AST. I dislike it because the<br>
contents of that separate mapping are highly tied up with the AST, and<br>
easy to get out of sync. I think it's the right choice for three<br>
reasons, 1) it is easier to try out and doesn't break the AST, so we<br>
have more scope for changing our minds later; 2) the same technique is<br>
able to represent things other than SrcSpan without introducing a<br>
polymorphic src span; 3) the people who pay the complexity are the<br>
people who use it, which is relatively few people.<br>
<br>
That said, as a tweak to the API, rather than a single data type for<br>
all annotations, you could have:<br>
<br>
data AnnIfThenElse = AnnIfThenElse {posIf, posThen, posElse :: SrcSpan}<br>
data AnnDo = AnnDo {posDo :: SrcSpan}<br>
<br>
Then you could just have an opaque Map (SrcSpan, TypeRep) Dynamic,<br>
with the invariant that the TypeRep in the key matches the Dynamic.<br>
Then you can have: getAnnotation :: Typeable a => Annotations -><br>
SrcSpan -> Maybe a. I think it simplifies some of the TypeRep trickery<br>
you are engaging in with mkAnnKey.<br>
<br>
Thanks, Neil<br>
<br>
On Wed, Oct 1, 2014 at 5:06 PM, Simon Peyton Jones<br>
<div class="HOEnZb"><div class="h5"><<a href="mailto:simonpj@microsoft.com">simonpj@microsoft.com</a>> wrote:<br>
> Let me urge you, once more, to consult some actual heavy-duty users of these<br>
> proposed facilities. I am very keen to avoid investing design and<br>
> implementation effort in facilities that may not meet the need.<br>
><br>
><br>
><br>
> If they end up acclaiming the node-key idea, then we should surely simply<br>
> make the key an abstract type, simply an instance of Hashable, Ord, etc.<br>
><br>
><br>
><br>
> Simon<br>
><br>
><br>
><br>
> From: Alan & Kim Zimmerman [mailto:<a href="mailto:alan.zimm@gmail.com">alan.zimm@gmail.com</a>]<br>
> Sent: 30 September 2014 19:48<br>
> To: Simon Peyton Jones<br>
> Cc: Richard Eisenberg; Edward Z. Yang; <a href="mailto:ghc-devs@haskell.org">ghc-devs@haskell.org</a><br>
><br>
><br>
> Subject: Re: Feedback request for #9628 AST Annotations<br>
><br>
><br>
><br>
> On further reflection of the goals for the annotation, I would like to put<br>
> forward the following proposal for comment<br>
><br>
><br>
> Instead of physically placing a "node-key" in each AST Node, a virtual<br>
> node key can be generated from any `GenLocated SrcSpan e' comprising a<br>
> combination of the `SrcSpan` value and a unique identifier from the<br>
> constructor for `e`, perhaps using its `TypeRep`, since the entire AST<br>
> derives Typeable.<br>
><br>
> To further reduce the intrusiveness, a base Annotation type can be<br>
> defined that captures the location of noise tokens for each AST<br>
> constructor. This can then be emitted from the parser, if the<br>
> appropriate flag is set to enable it.<br>
><br>
> So<br>
><br>
> data ApiAnnKey = AK SrcSpan TypeRep<br>
><br>
> mkApiAnnKey :: (Located e) -> ApiAnnKey<br>
> mkApiAnnKey = ...<br>
><br>
> data Ann =<br>
> ....<br>
> | AnnHsLet SrcSpan -- of the word "let"<br>
> SrcSpan -- of the word "in"<br>
><br>
> | AnnHsDo SrcSpan -- of the word "do"<br>
><br>
> And then in the parser<br>
><br>
> | 'let' binds 'in' exp { mkAnnHsLet $1 $3 (LL $ HsLet (unLoc $2)<br>
> $4) }<br>
><br>
> The helper is<br>
><br>
> mkAnnHsLet :: Located a -> Located b -> LHsExpr RdrName -> P (LHsExpr<br>
> RdrName)<br>
> mkAnnHsLet (L l_let _) (L l_in _) e = do<br>
> addAnnotation (mkAnnKey e) (AnnHsLet l_let l_in)<br>
> return e;<br>
><br>
> The Parse Monad would have to accumulate the annotations to be<br>
> returned at the end, if called with the appropriate flag.<br>
><br>
> There will be some boilerplate in getting the annotations and helper<br>
> functions defined, but it will not pollute the rest.<br>
><br>
> This technique can also potentially be backported to support older GHC<br>
> versions via a modification to ghc-parser.<br>
><br>
> <a href="https://hackage.haskell.org/package/ghc-parser" target="_blank">https://hackage.haskell.org/package/ghc-parser</a><br>
><br>
> Regards<br>
><br>
> Alan<br>
><br>
><br>
><br>
> On Tue, Sep 30, 2014 at 2:04 PM, Alan & Kim Zimmerman <<a href="mailto:alan.zimm@gmail.com">alan.zimm@gmail.com</a>><br>
> wrote:<br>
><br>
> I tend to agree that this change is much too intrusive for what it attempts<br>
> to do.<br>
><br>
> I think the concept of a node key could be workable, and ties in to the<br>
> approach I am taking in ghc-exactprint [1], which uses a SrcSpan together<br>
> with node type as the annotation key.<br>
><br>
> [1] <a href="https://github.com/alanz/ghc-exactprint" target="_blank">https://github.com/alanz/ghc-exactprint</a><br>
><br>
><br>
><br>
> On Tue, Sep 30, 2014 at 11:19 AM, Simon Peyton Jones <<a href="mailto:simonpj@microsoft.com">simonpj@microsoft.com</a>><br>
> wrote:<br>
><br>
> I'm anxious about it being too big a change too.<br>
><br>
> I'd be up for it if we had several "customers" all saying "yes, this is<br>
> precisely what we need to make our usage of the GHC API far far easier".<br>
> With enough detail so we can understand their use-case.<br>
><br>
> Otherwise I worry that we might go to a lot of effort to solve the wrong<br>
> problem; or to build a solution that does not, in the end, work for the<br>
> actual use-case.<br>
><br>
> Another way to tackle this would be to ensure that syntax tree nodes have a<br>
> "node-key" (a bit like their source location) that clients could use in a<br>
> finite map, to map node-key to values of their choice.<br>
><br>
> I have not reviewed your patch in detail, but it's uncomfortable that the<br>
> 'l' parameter gets into IfGblEnv and DsM. That doesn't smell right.<br>
><br>
> Ditto DynFlags/HscEnv, though I think here that you are right that the<br>
> "hooks" interface is very crucial. After all, the WHOLE POINT is too make<br>
> the client interface more flexible. I would consult Luite and Edsko, who<br>
> were instrumental in designing the new hooks interface<br>
> <a href="https://ghc.haskell.org/trac/ghc/wiki/Ghc/Hooks" target="_blank">https://ghc.haskell.org/trac/ghc/wiki/Ghc/Hooks</a><br>
> (I'm not sure if that page is up to date, but I hope so)<br>
><br>
> A good way to proceed might be to identify some of the big users of the GHC<br>
> API (I'm sure I don't know them all), discuss with them what would help<br>
> them, and share the results on a wiki page.<br>
><br>
> Simon<br>
><br>
><br>
> | -----Original Message-----<br>
> | From: ghc-devs [mailto:<a href="mailto:ghc-devs-bounces@haskell.org">ghc-devs-bounces@haskell.org</a>] On Behalf Of<br>
> | Richard Eisenberg<br>
> | Sent: 30 September 2014 03:04<br>
> | To: Edward Z. Yang<br>
> | Cc: <a href="mailto:ghc-devs@haskell.org">ghc-devs@haskell.org</a><br>
> | Subject: Re: Feedback request for #9628 AST Annotations<br>
> |<br>
> | I'm only speaking up because Alan is specifically requesting feedback:<br>
> | I'm really ambivalent about this. I agree with Edward that this is a<br>
> | big change and adds permanent noise in a lot of places. But, I also<br>
> | really respect the goal here -- better tool support. Is it worthwhile<br>
> | to do this using a dynamically typed bit (using Typeable and such),<br>
> | which would avoid the noise? Maybe.<br>
> |<br>
> | What do other languages do? Do we know what, say, Agda does to get<br>
> | such tight coupling with an editor? Does, say, Eclipse have such a<br>
> | chummy relationship with a Java compiler to do its refactoring, or is<br>
> | that separately implemented? Haskell/GHC is not the first project to<br>
> | have this problem, and there's plenty of solutions out there. And,<br>
> | unlike most other times, I don't think Haskell is exceptional in this<br>
> | regard (there's nothing very special about Haskell's AST, maybe beyond<br>
> | indentation-awareness), so we can probably adopt other solutions<br>
> | nicely.<br>
> |<br>
> | Richard<br>
> |<br>
> | On Sep 29, 2014, at 8:58 PM, "Edward Z. Yang" <<a href="mailto:ezyang@mit.edu">ezyang@mit.edu</a>> wrote:<br>
> |<br>
> | > Excerpts from Alan & Kim Zimmerman's message of 2014-09-29 13:38:45<br>
> | -0700:<br>
> | >> 1. Is this change too big, should I scale it back to just update<br>
> | the<br>
> | >> HsSyn structures and then lock it down to Located SrcSpan for all<br>
> | >> the rest?<br>
> | ><br>
> | > I don't claim to speak for the rest of the GHC developers, but I<br>
> | think<br>
> | > this change is too big. I am almost tempted to say that we<br>
> | shouldn't<br>
> | > add the type parameter at all, and do something else (maybe Backpack<br>
> | > can let us extend SrcSpan in a modular way, or even use a<br>
> | dynamically<br>
> | > typed map for annotations.)<br>
> | ><br>
> | > Edward<br>
> | > _______________________________________________<br>
> | > ghc-devs mailing list<br>
> | > <a href="mailto:ghc-devs@haskell.org">ghc-devs@haskell.org</a><br>
> | > <a href="http://www.haskell.org/mailman/listinfo/ghc-devs" target="_blank">http://www.haskell.org/mailman/listinfo/ghc-devs</a><br>
> |<br>
> | _______________________________________________<br>
> | ghc-devs mailing list<br>
> | <a href="mailto:ghc-devs@haskell.org">ghc-devs@haskell.org</a><br>
> | <a href="http://www.haskell.org/mailman/listinfo/ghc-devs" target="_blank">http://www.haskell.org/mailman/listinfo/ghc-devs</a><br>
> _______________________________________________<br>
> ghc-devs mailing list<br>
> <a href="mailto:ghc-devs@haskell.org">ghc-devs@haskell.org</a><br>
> <a href="http://www.haskell.org/mailman/listinfo/ghc-devs" target="_blank">http://www.haskell.org/mailman/listinfo/ghc-devs</a><br>
><br>
><br>
><br>
><br>
><br>
><br>
> _______________________________________________<br>
> ghc-devs mailing list<br>
> <a href="mailto:ghc-devs@haskell.org">ghc-devs@haskell.org</a><br>
> <a href="http://www.haskell.org/mailman/listinfo/ghc-devs" target="_blank">http://www.haskell.org/mailman/listinfo/ghc-devs</a><br>
><br>
</div></div></blockquote></div><br></div>