One way to think of it is this: we can now put SrcSpans where they make sense, rather than everywhere. We can still say (Located t) in places where we want to guarantee a SrcSpan. Yes, this lets us add more than one; that's redundant but not harmful. Simon | -----Original Message----- | From: ghc-devs On Behalf Of Matthew | Pickering | Sent: 12 February 2019 09:08 | To: Vladislav Zavialov | Cc: GHC | Subject: Re: TTG: Handling Source Locations | | I just did this now, it was quite disconcerting that my code continued to | compile after applying `cL loc` to the return value of one of my | functions. | | On Sat, Feb 9, 2019 at 5:40 PM Vladislav Zavialov | wrote: | > | > 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 | 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgh | > > c.haskell.org%2Ftrac%2Fghc%2Fwiki%2FImplementingTreesThatGrow%2FHand | > > lingSourceLocations&data=02%7C01%7Csimonpj%40microsoft.com%7C915 | > > 2cd5c5b624a9fac5c08d690c9a908%7C72f988bf86f141af91ab2d7cd011db47%7C1 | > > %7C0%7C636855593134767677&sdata=I6kltUVNtcMItCao1dPvnM86%2FlE8ky | > > CwshV81dD6mbY%3D&reserved=0 Phab diff: | > > https://phabricator.haskell.org/D5036 | > > Trac ticket: | > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgh | > > c.haskell.org%2Ftrac%2Fghc%2Fticket%2F15495&data=02%7C01%7Csimon | > > pj%40microsoft.com%7C9152cd5c5b624a9fac5c08d690c9a908%7C72f988bf86f1 | > > 41af91ab2d7cd011db47%7C1%7C0%7C636855593134767677&sdata=VeRbLhJD | > > ZQv%2FCZ39lMpwo2SRhmcyIsHRgwXNYDN28cA%3D&reserved=0 | > > Commit: | > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi | > > tlab.haskell.org%2Fghc%2Fghc%2Fcommit%2F509d5be69c7507ba5d0a5f39ffd1 | > > 613a59e73eea&data=02%7C01%7Csimonpj%40microsoft.com%7C9152cd5c5b | > > 624a9fac5c08d690c9a908%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C | > > 636855593134767677&sdata=nv9GjvSvGweBPmsHEVD1jBB7yz0Br0hDHtZ5Exv | > > uDqU%3D&reserved=0 | > > | > > 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 | > > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmai | > > l.haskell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-devs&data=02% | > > 7C01%7Csimonpj%40microsoft.com%7C9152cd5c5b624a9fac5c08d690c9a908%7C | > > 72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636855593134767677&sd | > > ata=EAHftJhgmjTvm8f3de99dOFSoddfwu1KPoVHMwP1KtA%3D&reserved=0 | > _______________________________________________ | > ghc-devs mailing list | > ghc-devs@haskell.org | > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail. | > haskell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-devs&data=02%7C01 | > %7Csimonpj%40microsoft.com%7C9152cd5c5b624a9fac5c08d690c9a908%7C72f988 | > bf86f141af91ab2d7cd011db47%7C1%7C0%7C636855593134767677&sdata=EAHf | > tJhgmjTvm8f3de99dOFSoddfwu1KPoVHMwP1KtA%3D&reserved=0 | _______________________________________________ | ghc-devs mailing list | ghc-devs@haskell.org | https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.has | kell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc- | devs&data=02%7C01%7Csimonpj%40microsoft.com%7C9152cd5c5b624a9fac5c08d | 690c9a908%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636855593134767677 | &sdata=EAHftJhgmjTvm8f3de99dOFSoddfwu1KPoVHMwP1KtA%3D&reserved=0

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

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

While we're on the topic, is there any plan to get rid of all those panics? AFAICS they are entirely unnecessary: we should just use an empty datatype for unused constructor extension points, then we can eliminate it to get whatever we like. See #15247. Adam On 12/02/2019 15:40, Shayan Najd wrote:

...

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

-- Adam Gundry, Haskell Consultant Well-Typed LLP, https://www.well-typed.com/