Feedback request for #9628 AST Annotations
Alan & Kim Zimmerman
alan.zimm at gmail.com
Wed Oct 1 20:16:26 UTC 2014
> 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 <ndmitchell at gmail.com>
> 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
> >> <simonpj at microsoft.com> 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:alan.zimm at gmail.com]
> >> > Sent: 30 September 2014 19:48
> >> > To: Simon Peyton Jones
> >> > Cc: Richard Eisenberg; Edward Z. Yang; ghc-devs at haskell.org
> >> >
> >> >
> >> > 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
> >> > <alan.zimm at gmail.com>
> >> > 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
> >> > <simonpj at microsoft.com>
> >> > 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:ghc-devs-bounces at haskell.org] On Behalf Of
> >> > | Richard Eisenberg
> >> > | Sent: 30 September 2014 03:04
> >> > | To: Edward Z. Yang
> >> > | Cc: ghc-devs at haskell.org
> >> > | 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" <ezyang at mit.edu>
> 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
> >> > | > ghc-devs at haskell.org
> >> > | > http://www.haskell.org/mailman/listinfo/ghc-devs
> >> > |
> >> > | _______________________________________________
> >> > | ghc-devs mailing list
> >> > | ghc-devs at haskell.org
> >> > | http://www.haskell.org/mailman/listinfo/ghc-devs
> >> > _______________________________________________
> >> > ghc-devs mailing list
> >> > ghc-devs at haskell.org
> >> > http://www.haskell.org/mailman/listinfo/ghc-devs
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > _______________________________________________
> >> > ghc-devs mailing list
> >> > ghc-devs at haskell.org
> >> > http://www.haskell.org/mailman/listinfo/ghc-devs
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.haskell.org/pipermail/ghc-devs/attachments/20141001/38d3d6ec/attachment-0001.html>
More information about the ghc-devs
mailing list