Re: TTG: Handling Source Locations

Someone could easily call rnPatAndThen when they should call rnLPatAndThen. This would cause a panic.

With Solution A, there shouldn't be two functions `rnLPatAndThen` and `rnPatAndThen` anyways. There should be only `rnPatAndThen` with an extra case for the wrapper node.

There's also the problem that the pattern-match checker can't usefully look through view patterns.

Yes, I have reported it while back. I don't know of the progress in fixing this. On Tue, 12 Feb 2019 at 16:24, Richard Eisenberg wrote:

That's true, but how would it play out in practice? For example, take a look at RnPat. There is a rnLPatAndThen which uses wrapSrcSpanCps to extract the location and then call rnPatAndThen. rnPatAndThen, in turn, just panics if it sees the extension point, because that's an unexpected constructor. Someone could easily call rnPatAndThen when they should call rnLPatAndThen. This would cause a panic.

There's also the problem that the pattern-match checker can't usefully look through view patterns. If there is a nested pattern-match (that is, we see dL->L _ (SomeOtherConstructor), then there is no way to guarantee a complete pattern-match short of a catch-all. So it doesn't seem to me that the pattern-match checker is really helping us achieve what we want here.

Richard

...

On Feb 12, 2019, at 9:30 AM, Shayan Najd wrote:

...

My problem, though, is that this is just a convention -- no one checks it. It would be easy to forget.

I am not sure if I understand: shouldn't the totality checker warn if there is no pattern for the wrapper constructor (hence enforce the convention)?

On Tue, 12 Feb 2019 at 15:19, Richard Eisenberg wrote:

...

...

On Feb 12, 2019, at 5:19 AM, Shayan Najd wrote:

About the new code, the convention is straightforward: anytime you destruct an AST node, assume a wrapper node inside (add an extra case), or use the smart constructors/pattern synonyms.

Aha! This, I did not know. So, you're saying that all the consumers of the GHC AST need to remember to use dL every time they pattern-match. With the new design, using dL when it's unnecessary doesn't hurt, but forgetting it is problematic. So: just use it every time. My problem, though, is that this is just a convention -- no one checks it. It would be easy to forget.

...

On Feb 12, 2019, at 6:00 AM, Simon Peyton Jones via ghc-devs wrote:

One way to think of it is this: we can now put SrcSpans where they make sense, rather than everywhere.

This has some logic to it, but I'm not quite sold. Another way of saying this is that the new design favors flexibility for the producer, at the cost of requiring consumers to be aware of and consistently apply the convention Shayan describes above. The problem is, though, that if the producer is stingy in adding source locations, the consumer won't know which locations are expected to be informative. Is the consumer expected to collect locations from a variety of places and try to combine them somehow? I doubt it. So this means that the flexibility for the producer isn't really there -- the type system will accept arbitrary choices of where to put locations, but consumers won't get the locations where they expect them.

...

We can still say (Located t) in places where we want to guarantee a SrcSpan.

This seems to go against the TTG philosophy. We can do this in, say, the return type of a function, but we can't in the AST proper, because that's shared among a number of clients, some of whom don't want the source locations.

...

Yes, this lets us add more than one; that's redundant but not harmful.

I disagree here. If we add locations to a node twice, then we'll have to use dL twice to find the underlying constructor. This is another case there the type system offers the producer flexibility but hamstrings the consumer.

...

On Feb 12, 2019, at 7:32 AM, Vladislav Zavialov wrote:

I claim an SrcSpan makes sense everywhere, so this is not a useful distinction. Think about it as code provenance, an AST node always comes from somewhere

I agree with this observation. Perhaps SrcSpan is a bad name, and SrcProvenance is better. We could even imagine using the new HasCallStack feature to track where generated code comes from (perhaps only in DEBUG compilers). Do we need to do this today? I'm not sure there's a crying need. But philosophically, we are able to attach a provenance to every slice of AST, so there's really no reason for uninformative locations.

...

My concrete proposal: let's just put SrcSpan in the extension fields of each node

I support this counter-proposal. Perhaps if it required writing loads of extra type instances, I wouldn't be as much in favor. But we already have to write those instances -- they just change from NoExt to SrcSpan. This seems to solve all the problems nicely, at relatively low cost. And, I'm sure it's more efficient at runtime than either the previous ping-pong style or the current scenario, as we can pattern-match on constructors directly, requiring one less pointer-chase or function call.

One downside of this proposal is that it means that more care will have to be taken when setting the extension field of AST nodes after a pass, making sure to preserve the location. (This isn't really all that different from location-shuffling today.) A quick mock-up shows that record-updates can make this easier:

...

data Phase = Parsed | Renamed

data Exp p = Node (XNode p) Int

type family XNode (p :: Phase) type instance XNode p = NodeExt p

data NodeExt p where NodeExt :: { flag :: Bool, fvs :: RenamedOnly p String } -> NodeExt p

type family RenamedOnly p t where RenamedOnly Parsed _ = () RenamedOnly Renamed t = t

example :: Exp Parsed example = Node (NodeExt { flag = True, fvs = () }) 5

rename :: Exp Parsed -> Exp Renamed rename (Node ext n) = Node (ext { fvs = "xyz" }) n

Note that the extension point is a record type that has a field available only after renaming. We can then do a type-changing record update when producing the renamed node, preserving the flag in the code above. What's sad is that, if there were no renamer-only field, we couldn't do a type-changing empty record update as the default case. (Haskell doesn't have empty record updates. Perhaps it should. They would be useful in doing a type-change on a datatype with a phantom index. A clever compiler could even somehow ensure that such a record update is completely compiled away.) In any case, this little example is essentially orthogonal to my points above, and the choice of whether to use records or other structures are completely local to the extension point. I just thought it might make for a nice style.

Thanks, Richard