I wholly share this concern, which is why I commented on the Phab diff: > Does this rely on the caller to call dL on the pattern? Very fragile, let's > not do that.
In addition, I'm worried about illegal states where we end up with multiple nested levels of `NewPat`, and calling `dL` once is not sufficient. As to the better solution, I think we should just go with Solution B from the Wiki page. Yes, it's somewhat more boilerplate, but it guarantees to have locations in the right places for all nodes. The main argument against it was that we'd have to define `type instance XThing (GhcPass p) = SrcSpan` for many a `Thing`, but I don't see it as a downside at all. We should do so anyway, to get rid of parsing API annotations and put them in the AST proper. All the best, Vladislav On Sat, Feb 9, 2019 at 7:19 PM Richard Eisenberg <r...@cs.brynmawr.edu> wrote: > > Hi devs, > > I just came across [TTG: Handling Source Locations], as I was poking around > in RdrHsSyn and found wondrous things like (dL->L wiz waz) all over the place. > > General outline: > https://ghc.haskell.org/trac/ghc/wiki/ImplementingTreesThatGrow/HandlingSourceLocations > Phab diff: https://phabricator.haskell.org/D5036 > Trac ticket: https://ghc.haskell.org/trac/ghc/ticket/15495 > Commit: > https://gitlab.haskell.org/ghc/ghc/commit/509d5be69c7507ba5d0a5f39ffd1613a59e73eea > > I see why this change is wanted and how the new version works. > > It seems to me, though, that this move makes us *less typed*. That is, it > would be very easy (and disastrous) to forget to match on a location node. > For example, I can now do this: > > > foo :: LPat p -> ... > > foo (VarPat ...) = ... > > Note that I have declared that foo takes a located pat, but then I forgot to > extract the location with dL. This would type-check, but it would fail. > Previously, the type checker would ensure that I didn't forget to match on > the L constructor. This error would get caught after some poking about, > because foo just wouldn't work. > > However, worse, we might forget to *add* a location when downstream functions > expect one. This would be harder to detect, for two reasons: > 1. The problem is caught at deconstruction, and figuring out where an object > was constructed can be quite hard. > 2. The problem might silently cause trouble, because dL won't actually fail > on a node missing a location -- it just gives noSrcSpan. So the problem would > manifest as a subtle degradation in the quality of an error message, perhaps > not caught until several patches (or years!) later. > > So I'm uncomfortable with this direction of travel. > > Has this aspect of this design been brought up before? I have to say I don't > have a great solution to suggest. Perhaps the best I can think of is to make > Located a type family. It would branch on the type index to HsSyn types, > introducing a Located node for GhcPass but not for other types. This Isn't > really all that extensible (I think) and it gives special status to GHC's > usage of the AST. But it seems to solve the immediate problems without the > downside above. > > Sorry for reopening something that has already been debated, but (unless I'm > missing something) the current state of affairs seems like a potential > wellspring of subtle bugs. > > Thanks, > Richard > _______________________________________________ > ghc-devs mailing list > ghc-devs@haskell.org > http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs _______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
