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