Correct location annotation for parenthesized expressions

[expipiplus1]

This change is 

[Anton-Latukha]

I would crack more about the nature of the fixpoints in recursivity and so unFix. Because, lets say, Hackage docs are not very helpful (not existent) on it, I would learn and contribute a litle addition to the unFix and related docs and come back into PR, maybe I would cover that in this weekend, maybe in a couple of days,

[Anton-Latukha]

Oh, wait, unFix is a record of the Fix, which already explains a lot.

[expipiplus1]

Yeah, it feels a little weird to have to strip the annotation and add it again, but seemed like the least intrusive change available. Could perhaps make it a little neater with a replaceAnnotation function.

[expipiplus1]

Would it be possible to get this (and perhaps other recent PRs) merged if there are no problems?

A release would be super too, I'd like to sort out the update-nix-fetchgit tool in nixpkgs and it does depend on this cutting edge hnix.

[Anton-Latukha]

It is currenly very late night. I promise, would sort this out during tomorrow.

Thanks for the notification of urgency.

[expipiplus1]

Thanks! Urgent is a strong word! As long as it doesn't take months then I'm happy :)

…

On Wed, Oct 28, 2020, 9:02 AM Anton Latukha ***@***.***> wrote: It is currenly very late night. I promise, would sort this out during tomorrow. Thanks for the notification of urgency. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRJXEGH7QW6T2RUHKFQATSM5USNANCNFSM4STGNI3A> .

[Anton-Latukha]

John probably allowed me to tackle this.

Sorry, outside it looks very silly that I take so much time with this.

I essentially needed to do a bunch of things to be a better HNix maintainer, so on first request, I started a lazy evaluation of them

Since we do not understand completely the theory, and one needs to understand at least roughly why the cases with code Fix/unFix and procedures are such and what is happening in the algorythm structures - I went on looking some lectures on the recursion schemes.

Started to understand the gist of it. So far I got a ground basic theoretical understanding. For ex. I know what those *F types are open recursive type definitions that by applying scheme operation with algebra transform into a particular result. And you landed on stripAnn . unFix <$> because what is accomplished (probably) a catamorphism from recursively annotated expressions into unannotated form. But so far I did not learned solidly the schemes themselves.

As I observed it is a pattern that recursion schemes question is a rock people stumble on maintenance/contribution to HNix, so it needs at least some basic quick walkthrough explanation for people and links for good materials on it.

Since it is the case that Haskell development on NixOS, may I say, - is more a Nix development exercise case sphere. Haskell dev can not really live comfortably on NixOS at current state of affairs.
I also in this time I fully migrated the setup I have from NixOS onto Ubuntu.

I also once was one who wrote the first article on how to integrate Emacs/HIE/Nix for other people, but in the winter that was no more, and I got bothered that for everyone HLS easilly works already, and for me - all my tries got nope, nope nope. VSCode worked from the box and Emacs - nope. Fixed it yesterday - it was a mix of some upstream bugs, one obscure side-effect bug of a feature that I took as a whole Spacemacs state save feature 1.5 years ago. And as a proper Linux citizen I make daemons a systmed services, and so Emacs daemon did not imported the user env, since systemd services run in the clear env by default - Emacs had no ghcup path loaded, but message retunred by lsp-haskell is really obscure, so I probably should uspream a fix there.

So that what I did in these 1.5 weeks. Sorry.

Now I am more capable of doing support.

[Anton-Latukha]

tl;rd -> section "Most probably"

Honestly, it is an end API function, which currently not documented itself and around it also not much docs.
So, please provide info: what is expected from your view from it, what is the problem with the current implementation, and how PR solve this.

I indeed can not understand why code strips annotation and then put it back on - and that becomes a working implementation of it.

As I understand - nixParens annotates the location of the particular parens, but inside it receives already fully recursively annotated expression, so why we need to strip all internal annotations and only annotations on the external parens.

Since what is inside () in Nix - is a fully proper expression, so nixToplevelForm seems appropriate.

---

If annotateLocation1 is a proper solution for the case - what it requests by type is:

Parser (NExprF NExprLoc)

And what (parens nixToplevelForm <?> "parens") is:

Fix NExprF

And

type NExpr = Fix NExprF

So how we need NExprLoc <-> NExpr.

For me, it seems we are close to finding the proper solution, somewhere indeed should be Fix, or the mk.. from the Nix/Expr/Shorthands.hs.

It is if annotateLocation1 is being a part of a proper solution.

Maybe it is a replacement of nixToplevelForm (which should evaluate at to what it is inside () - nixExpr), but that throws a weird compile-time error of non-exhaustive pattern.

---

Most probably:

What arrives is already annotated expression and annotateLocation1 requires not annotated expression, so since it annotates it fully and the definition is:

annotateLocation1 = fmap annToAnnF . annotateLocation

and that does what is needed.

-- it is also logical to take that definition and do only the last part of it, fmap annToAnnF on annotated part (_ :: Parser (Ann SrcSpan a)).

And the annToAnnF seems to do what is needed:

annToAnnF :: Ann ann (f (Fix (AnnF ann f))) -> Fix (AnnF ann f)
annToAnnF (Ann ann a) = AnnE ann a
--- ... (where)
pattern AnnE ann a = Fix (Compose (Ann ann a))

-- it takes an annotated expression and composes new annotation onto it. Which seems what parens are semantically about (invisible abstraction around something).

So:

nixParens = fmap annToAnnF ... parens ... nixToplevelForm <?> "parens" -- ... profit!

In play before this idea - had yielded something like (or literally) Parser (Ann SrcSpan a) which is what fmap annToAnnF requires.

I think it is worth playing with this and figuring it out a little more, so one of us finds the idea.

[expipiplus1]

Sorry, outside it looks very silly that I take so much time with this.

Not at all.

And you landed on stripAnn . unFix <$> because what is accomplished (probably) a catamorphism from recursively annotated expressions into unannotated form. But so far I did not learned solidly the schemes themselves.

I think you might have confused stripAnn and stripAnnotation; the latter
removes annotations over a whole tree, whereas the former strips just one layer
of annotations.

As I observed it is a pattern that recursion schemes question is a rock people stumble on maintenance/contribution to HNix, so it needs at least some basic quick walkthrough explanation for people and links for good materials on it.

Just for some context, I wrote the original code adding location annotations:
#35

Honestly, it is an end API function, which currently not documented itself and around it also not much docs.
So, please provide info: what is expected from your view from it, what is the problem with the current implementation, and how PR solve this.

Sorry, I should have been clearer in the commit message.

At the moment parentheses are not included in the location annotation for nix
expressions. This changes the code to include any parentheses in the SrcSpan
of the underlying expression. Information which is difficult to recover
otherwise.

As I understand - nixParens annotates the location of the particular parens, but inside it receives already fully recursively annotated expression, so why we need to strip all internal annotations and only annotations on the external parens.

Because the top level annotation is incorrect, as it doesn't include the parentheses.

Since what is inside () in Nix - is a fully proper expression, so nixToplevelForm seems appropriate.

If annotateLocation1 is a proper solution for the case - what it requests by type is:

Parser (NExprF NExprLoc)

Correct, note that NExprF f is a top level unannotated expression full of
fs, in this case annotated expressions. i.e. annotateLocation1 takes an
expression where only the top level is unannotated.

And what (parens nixToplevelForm <?> "parens") is:

Fix NExprF

This is not correct.

• parens :: Parser a -> Parser a
• (<?>) :: Parser a -> String -> Parser a
• nixToplevelForm :: Parser NExprLoc

hence

• parens nixToplevelForm <?> parens :: Parser NExprLoc

Most probably:

What arrives is already annotated expression and annotateLocation1 requires not annotated expression, so since it annotates it fully and the definition is:

annotateLocation1 = fmap annToAnnF . annotateLocation

and that does what is needed.

Exactly, the stripAnn . unFix <$> is going from an annotated expression to an
unannotated one, suitable for passing to annotateLocation1

And the annToAnnF seems to do what is needed:

annToAnnF :: Ann ann (f (Fix (AnnF ann f))) -> Fix (AnnF ann f)
annToAnnF (Ann ann a) = AnnE ann a
--- ... (where)
pattern AnnE ann a = Fix (Compose (Ann ann a))

-- it takes an annotated expression and composes new annotation onto it. Which seems what parens are semantically about (invisible abstraction around something).

It doesn't add any new annotations (indeed there are no new annotations to add
in the type signature), just changes the type. It may as well be coerce.

So:

nixParens = fmap annToAnnF ... parens ... nixToplevelForm <?> "parens" -- ... profit!

In play before this idea - had yielded something like (or literally) Parser (Ann SrcSpan a) which is what fmap annToAnnF requires.

I think it is worth playing with this and figuring it out a little more, so one of us finds the idea.

That's pretty much what's going on in this change.

• The first ... between annToAnnF is the remainder of annotateLocation1
• As parens doesn't change the type the expression in the second or third
  ... doesn't care where it's placed. I chose to put it after the parens,
  however it would work equally well before the parens.

Thank you for making me think about this a bit more, upon reviewing the code
writing this answer I came to the conclusion that with source annotations in
play, it's not generally appropriate to have higher-order parsers operate on
annotated locations as they are liable to perform changes to the parser which
are not captured in the annotation. See
#744 for another example of this
problem.

This PR is an improvement on the status quo, but future work should make sure
that Parser NExprLoc doesn't appear in a negative position in parser combinators which perform additional parsing.

[expipiplus1]

I have improved the comment on nixParens and added more restrictive types to parens and brackets as mentioned above.

[Anton-Latukha]

Such in-depth response, very informative, thank you!

Indeed, I thought that stripAnn does what it says, and with stripAnnotation I see the difference, stripAnn is open version and stripAnnotation is a fixed point version.

Since it is one level of annotation action, on the first read I see that the case is solved here.

I would parse your respone a couple of times more further down the line.

And I see that I going to go into a phase of making adjustments to the project code, mainly docs and renaming some functions where rename is appropriate to be descriptive of the action, or since John has a particular naming style - naming convention should be cleared-out, so the code would be more readable from the function names and so fewer docs/explanations would be needed for working with it.

The main point I currently see for me, is to work on making the project more approachable for contributors, so additional thanks for adding some comments.

I have great experience working with you, thank you.

[expipiplus1]

Thanks!