New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore commented out annotations #617

Closed
rpglover64 opened this Issue Feb 21, 2016 · 18 comments

Comments

Projects
None yet
5 participants
@rpglover64
Copy link
Contributor

rpglover64 commented Feb 21, 2016

-- {-@ foo :: () @-}
-- foo = ()

and

{-
{-@ foo :: () @-}
foo = ()
-}

Both cause LH to complain about foo being undefined.

@ranjitjhala

This comment has been minimized.

Copy link
Member

ranjitjhala commented Feb 22, 2016

Thanks! This has bothered me too!
@spinda do you think you can take a quick look as you are working with the parser?

On Feb 20, 2016, at 8:30 PM, Alex R notifications@github.com wrote:

-- {-@ foo :: () @-}
-- foo = ()
and

{-
{-@ foo :: () @-}
foo = ()
-}
Both cause LH to complain about foo being undefined.


Reply to this email directly or view it on GitHub.

@spinda

This comment has been minimized.

Copy link
Member

spinda commented Mar 7, 2016

The code that grabs the {-@ @-} comments is here. It just skips everything until it hits a {-@. To skip commented-out annotations, it could:

  1. Skip everything until {-
  2. If the next character is @, parse a specification. Otherwise, skip the entire comment.
  3. Repeat.
@ranjitjhala

This comment has been minimized.

Copy link
Member

ranjitjhala commented Mar 7, 2016

​That sounds right...​

On Sun, Mar 6, 2016 at 10:12 PM, Michael Smith notifications@github.com
wrote:

The code that grabs the {-@ @-} comments is here
https://github.com/ucsd-progsys/liquidhaskell/blob/master/src/Language/Haskell/Liquid/Parse.hs#L925-L939.
It just skips everything until it hits a {-@. To skip commented-out
annotations, it could:

  1. Skip everything until {-
  2. If the next character is @, parse a specification. Otherwise, skip
    the entire comment.
  3. Repeat.


Reply to this email directly or view it on GitHub
#617 (comment)
.

@rpglover64

This comment has been minimized.

Copy link
Contributor Author

rpglover64 commented Mar 7, 2016

What about inline comments?

@spinda

This comment has been minimized.

Copy link
Member

spinda commented Mar 8, 2016

Ah, true. Also it should deal with things that look like comments within quotes. So:

  1. Skip everything until ", --, or {-.
  2. If ", skip a Haskell string. Go to 1.
  3. If --, skip the rest of the line comment. Go to 1.
  4. If {- and the next character is @, parse a specification. Otherwise, skip the entire comment. Go to 1.
@rpglover64

This comment has been minimized.

Copy link
Contributor Author

rpglover64 commented Mar 8, 2016

GHC-the-library is already a dependency, right? Does it not expose a parser that preserves comments? (I feel like that would be less maintenance burden; other than that, this description looks good to me)

I think quasiquotes are rare enough that they're not a worry here.

@spinda

This comment has been minimized.

Copy link
Member

spinda commented Mar 8, 2016

GHC's parser is implemented in a great big Happy file and very monolithic. I don't think we'd be able to reuse parts of it here.

However, now that you mention it, there is the ApiAnnotations interface, which is designed for extracting special comments like Haddock's from Haskell source. But it doesn't retain the location of each comment, so we'd have to somehow obtain that manually (searching through the source for the comment text...?).

@alanz

This comment has been minimized.

Copy link
Contributor

alanz commented Mar 8, 2016

The API Annotations interface does provide locations for the comments.

@alanz

This comment has been minimized.

Copy link
Contributor

alanz commented Mar 8, 2016

And if you set Opt_KeepRawTokenStream in the dynflags you can access the comments as a field in ParsedModule when invoking GHC.

@spinda

This comment has been minimized.

Copy link
Member

spinda commented Mar 8, 2016

@alanz

It associates certain comments with SrcSpans, but those SrcSpans correspond to the location of a nearby syntax element rather than the location of the comment itself, and top-level comments don't have real SrcSpans attached at all. There's an example of the output on the wiki page. Is there other location info that can be extracted through ApiAnnotations?

ParsedModule has pm_annotations, the ApiAnnotations stuff, and pm_parsed_source, the parsed AST which I don't believe retains any comment information.

@alanz

This comment has been minimized.

Copy link
Contributor

alanz commented Mar 8, 2016

The ApiAnns type is defined as

type ApiAnns = (Map ApiAnnKey [SrcSpan], Map SrcSpan [Located AnnotationComment]) 

For the comments, the map is indexed by the nearest enclosing SrcSpan, but the AnnotationComment has its original location in the source file.

So Map.elems (snd anns) will give you a list of located comments.

@spinda

This comment has been minimized.

Copy link
Member

spinda commented Mar 8, 2016

Ah ha, I should have looked at that closer. Thanks!

Then ApiAnnotations should be perfect for our use case.

@spinda spinda self-assigned this Mar 8, 2016

@ranjitjhala

This comment has been minimized.

Copy link
Member

ranjitjhala commented Mar 8, 2016

Thanks a bunch @alanz !!!

@gridaphobe

This comment has been minimized.

Copy link
Contributor

gridaphobe commented Mar 8, 2016

So is the idea now that instead of parsing the module ourselves and extracting the comments, we just extract these AnnotationComments from GHC itself and discard the ones that don't start with {-@?

@spinda

This comment has been minimized.

Copy link
Member

spinda commented Mar 8, 2016

Exactly. Does that sound good?

@gridaphobe

This comment has been minimized.

Copy link
Contributor

gridaphobe commented Mar 8, 2016

Sounds great!

@ranjitjhala

This comment has been minimized.

Copy link
Member

ranjitjhala commented Mar 8, 2016

+1

On Mar 8, 2016, at 2:55 PM, Eric Seidel notifications@github.com wrote:

Sounds great!


Reply to this email directly or view it on GitHub.

spinda added a commit that referenced this issue Apr 28, 2016

@spinda

This comment has been minimized.

Copy link
Member

spinda commented Apr 28, 2016

The api-annotations branch is pushed and will hopefully be merged soon. It both addresses this issue and reworks the module processing pipeline to allow for more improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment