> 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.
The key is that there is no linkage from the constructor. Instead of putting ((srcspan,"LetIn"), AnnLetIn ... :: Ann) into the map we put (srcspan, AnnLetIn ... :: AnnLetIn) in the map. The constructor is implicitly encoded by the type of the annotation. > This is a very good reason to break it into separate types. And then the > reason > for the Dynamic becomes clear. The whole TypeRep/Dynamic thing is just a nice way to encode Map with multiple value types that don't tread on each other, it's not a detail the user of the API would ever see. > > >> >> 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
