API Annotations tweaks.
ClosedPublic

Authored by alanz on Nov 30 2014, 2:28 PM.

Details

Summary

HsTyLit now has SourceText

Update documentation of HsSyn to reflect which annotations are attached to which element.

Ensure that the parser always keeps HsSCC and HsTickPragma values, to
be ignored in the desugar phase if not needed

Bringing in SourceText for pragmas

Add Location in NPlusKPat

Add Location in FunDep

Make RecCon payload Located

Explicitly add AnnVal to RdrName where it is compound

Add Location in IPBind

Add Location to name in IEThingAbs

Add Maybe (Located id,Bool) to Match to track fun_id,infix

This includes converting Match into a record and adding a note about why
the fun_id needs to be replicated in the Match.

Add Location in KindedTyVar

Sort out semi-colons for parsing

  • import statements
  • stmts
  • decls
  • decls_cls
  • decls_inst
Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
alanz updated this revision to Diff 2039.Dec 29 2014, 6:38 AM

rebase ghc-7.10

alanz updated this revision to Diff 2040.Dec 29 2014, 8:01 AM

Trying to represent patch wrt ghc-7.10

alanz updated this revision to Diff 2041.Dec 29 2014, 10:31 AM

Make sure bang annotations work

alanz updated this revision to Diff 2048.Dec 30 2014, 9:21 AM

Preserve parens annotations through checkTyClHdr for class/data decls

alanz updated this revision to Diff 2049.Dec 30 2014, 3:43 PM

Bring in specific annotations for '(',')','[',']','{' and '}'

alanz updated this revision to Diff 2051.Dec 31 2014, 11:06 AM

Add annotations for new 'static' keyword

alanz updated this revision to Diff 2052.Dec 31 2014, 12:01 PM

Tweak tycl_hdr locations so annotations are available for the context

alanz updated this revision to Diff 2053.Jan 1 2015, 2:53 PM

Distinguish between a comma as a separator and one in a tuple

alanz updated this revision to Diff 2054.Jan 2 2015, 11:46 AM

Add Location to the HsOverLit in NPat

alanz added a comment.Jan 6 2015, 8:15 AM

Please review this diff, so that it can go into 7.10 RC2.

It has been rebased against ghc-7.10, and represents the deficiencies found when actually using the API Annotations in ghc-exactprint, which is now in a state to successfully round trip source code.

The diff itself is quite large, as it contains firstly a lot of updates to the haddock documentation to clarify exactly which annotation is attached where, and secondly a suprisingly large number of individually small tweaks that were required to sort out the real-life usage.

You will notice that 'Match' now has

m_fun_id_infix :: (Maybe (Located id,Bool)),
  -- fun_id and fun_infix for functions with multiple equations
  -- only present for a RdrName. See note [fun_id in Match]

which captures the individual locations of the function name in a set of 'FunBind's for a specific declaration.

SPJ has aked whether this could be used to replace the fun_id and fun_infix from FunBind, and I believe this could be the case, but that specific change is against the spirit of this diff which deliberately does not change any behaviour or processing, so should be benign for the RC from a risk perspective.

This work can bbe done in a later diff.

A final note, the problems addressed in this diff are severe enough that without it the Api Annotations will not be able to round tript source code.

Regards
Alan

alanz updated this revision to Diff 2073.Jan 7 2015, 1:41 PM

Add SrcSpan to ResTyGADT as a hook for the API Annotations attached to
the original sigtype.

Problem found via HaRe roundtrip command

alanz updated this revision to Diff 2074.Jan 7 2015, 3:38 PM

Make sure the annotations for nested where clauses in a case come through.

alanz updated this revision to Diff 2085.Jan 8 2015, 8:46 AM

Fix comma in multiple signature declaration

alanz updated this revision to Diff 2087.Jan 8 2015, 2:59 PM

Rebase against current master

alanz updated this revision to Diff 2099.Jan 10 2015, 8:43 AM

Rebase and sort out semis in alts

alanz updated this revision to Diff 2100.Jan 11 2015, 3:38 AM

Fixing semicolons

alanz updated this revision to Diff 2101.Jan 11 2015, 5:43 AM

Fix silly error

alanz updated this revision to Diff 2102.Jan 11 2015, 7:59 AM

Add getAndRemoveAnnotation to ApiAnnotations API

goldfire edited edge metadata.Jan 11 2015, 8:03 AM

Skimmed up to, but not including, HsDecls. Hopefully more later.

compiler/basicTypes/BasicTypes.hs
455

What does this comment mean? I'm sure I'd understand it if I knew about API Annotations, but I don't.

Also, it seems that @ is part of Haddock markup, no? That makes this quite hard to read in source. Maybe separate out the @? Or use a code block?

compiler/basicTypes/DataCon.hs
456

Would this be better like

data UnpackPragma = PackPragma SourceText | UnpackPragma SourceText

data HsBang
  = HsNoBang
  | HsSrcBang (Maybe UnpackPragma) Bool

?

compiler/basicTypes/RdrName.hs
90

I find this comment confusing -- I was expecting an ApiAnnotation field somewhere in RdrName.

95

I don't understand this.

compiler/basicTypes/SrcLoc.hs
527

These are derivable only for l = SrcSpan? If so, why? If not, why not generalize this instance?

compiler/deSugar/DsExpr.hs
303–314

Why this change?

compiler/hsSyn/Convert.hs
529–530

Putting an empty string here seems wrong. I believe there is an effort to make TH produce proper Haskell code that can be parsed. But, maybe the empty string here accomplishes that. If so, please comment.

Of course, this comment applies to other empty strings in this file.

compiler/parser/Lexer.x
707

What are the exact contents of the SourceText? Specifically, does it include the {-# and #-}? I would assume it contains all whitespace between the delimiters, but the delimiters themselves may or may not be there. Please answer in the Note.

alanz added a comment.Jan 11 2015, 8:47 AM

Thanks, great to get some feedback.

Oops, didn't realise I had to do a submit on these.

compiler/basicTypes/BasicTypes.hs
455

This is the tradeoff between documentation that makes sense in the source vs in the generated haddock documentation.

The generated doc looks like this

AnnKeywordId : AnnOpen '{-# OVERLAPPABLE' or '{-# OVERLAPPING' or '{-# OVERLAPS' or '{-# INCOHERENT', AnnClose `#-}`,

Where AnnKeywordId, AnnOpen and AnnClose are links to the documentations for the ApiAnnotation module.

Perhaps I should put a separate source code reference to the ApiAnnotations module, or to https://ghc.haskell.org/trac/ghc/wiki/GhcAstAnnotations

compiler/basicTypes/DataCon.hs
456

I'm not sure.

SourceText has been introduced for the API Annotations, and to me makes it clear that this is only an annotation, as opposed to something that can potentially be processed later.

The rest of the datatype represents the parsing of the pack pragma, so introducing the new type may sow confusion.

I do not feel strongly about this, just a point.

compiler/basicTypes/RdrName.hs
90

Changed to refer to 'Located RdrName'. The API Annotations are attached to the SrcSpan for a Located item.

compiler/basicTypes/SrcLoc.hs
527

Ok

compiler/deSugar/DsExpr.hs
303–314

Prior to this, if SccProfiling was off the parser would replace the HsSCC with HsPar. This cannot happen if we want to round-trip source, so it had to be moved here.

compiler/hsSyn/Convert.hs
529–530

Ok.

I left it out initially because my expectation was that source to source conversions will be generated from the ParsedSource only.

compiler/parser/Lexer.x
707

Ok

alanz updated this revision to Diff 2104.Jan 11 2015, 12:06 PM
alanz edited edge metadata.

Updates based on @goldfire's comments

simonpj accepted this revision.Jan 14 2015, 5:02 PM
simonpj edited edge metadata.

SPJ has aked whether this could be used to replace the fun_id and fun_infix from FunBind, and I believe this could be the case, but that specific change is against the spirit of this diff which deliberately does not change any behaviour or processing, so should be benign for the RC from a risk perspective.

This work can bbe done in a later diff.

Fine; I'm ok with the current patch going in; I think you have been discussing it with Austin quite regularly.

But could you please open a ticket for this job, with a clear statement of what needs to be done? Otherwise we'll just lose it.

(And would you feel able to make a patch for it too?)

Simon

compiler/basicTypes/BasicTypes.hs
494–495

I beg you to *comment* these new fields. Why does Overlappable have a "SourceText" field? If you like, refer to a Note somewhere. But please say.

(Later) Ah, I see that *other* uses of SourceText do have a reference to a Note, just not these ones. Do add it.

859

I'd put your main Note here, not in Lexer.x nor in HsLit. It's important to you, to give it its own little section in BasicTypes, with examples. Is it just pragmas and literals?

compiler/hsSyn/HsExpr.hs
1006

What does "only present for a RdrName" mean? Do you mean that this info is discarded by the renamer? Doesn't that threaten round-tripping? Why not keep it?

compiler/hsSyn/HsPat.hs
163

Why this change to add Located? How does the location on the hsOverLit differ from the location on the NPat?

compiler/hsSyn/HsTypes.hs
255

I have no idea what these "ApiAnnotation.AnnKeywordId" things mean. (Well I do a bit, because of the history, but if I was Joe Programmer I definitely wouldn't.) How would the reader be expected to interpret them? You have gone to a lot of trouble to add them; it would be a pity if your customers did not know what they meant, and hence ignored them.

360–361

where is Note [Literal source text]?

This revision is now accepted and ready to land.Jan 14 2015, 5:02 PM
alanz updated this revision to Diff 2122.Jan 15 2015, 2:38 AM
alanz edited edge metadata.

Updates based on feeback from @simonpj

alanz added a comment.Jan 15 2015, 2:41 AM

Thanks for the feeback. I have updated the diff and replied to the comments.

The future work is captured in Trac #9988, assigned to me.

compiler/basicTypes/BasicTypes.hs
494–495

Ok

859

Moved.

Pragmas, literals, and also used in ForeignImport and CType, but each of those has an individual comment.

compiler/hsSyn/HsExpr.hs
1006

The source-to-source conversions are intended to make use of the ParsedSource, as the renamer does some re-ordering of declarations when it gathers the various declaration types into HsGroup.

In keeping with "don't change existing functionality" for this diff, the field is set to 'Nothing' in 'rnMatch' as I did not want to have to plumb through the requisite information to rename the fun_id at this point.

Renaming this field will be addresses as part of Trac #9988.

compiler/hsSyn/HsPat.hs
163

A negated value in the source looks like

-  n

The NPat discards the '-', but we need to output it again. The location for the NPat gives the start of the '-' and end of the literal, we need the start of the literal within that.

compiler/hsSyn/HsTypes.hs
255

This is an issue @goldfire brought up too.

The problem is that there are two different classes of user for this comment, GHC developers reading the source and GHC API users reading the haddock documentation. These comments are targeted at the latter.

I am not sure what the best solution is, because adding a note to each place they occur would be intrusive.

I have updated the haddock comment for the target of "ApiAnnotation.AnnKeywordId" to summarize what they are used for.

360–361

Fixed

alanz updated this revision to Diff 2123.Jan 15 2015, 4:22 AM

rebase

simonpj added inline comments.Jan 15 2015, 8:13 AM
compiler/basicTypes/BasicTypes.hs
793

Tiny point; I never put a blank line after the underlining of a Note.

819

Another tiny point; I use an initial capital: Note [Literal source text]

827

The point of your motivating examples is that there are multiple strings that all represent the same literal value. So each row of this table should demonstrate that. Currently only a handful do

859

Are you sure you moved the main Note. I don't see it here.

compiler/hsSyn/HsPat.hs
163

Fine. But don't tell *me* that! Instead add a comment that all the other readers will know.

alanz added a comment.Jan 15 2015, 8:39 AM

I have adressed the inline comments, update coming

This revision was automatically updated to reflect the committed changes.