Annotation System

[CAG2Mark]

Adds an annotation system and basic type checker support (TODO).

The parser supports adding arbitrary annotations to any expressions/statements. For example:

@tailrec
fun sum(n) = if n == 0 then 0 else @tailrec sum(n - 1)

This is in preparation for implementing the @tailrec annotation.

TODO:

• Currently it uses a mutable style Annotated trait similar to Located. Change it to use an Ann syntax that contains other terms and contains a list of annotations.
• Implement pretty-printing for the changes.
• Basic type checking to make sure all annotation names are in scope.

[CAG2Mark]

Added an Ann case class that extends Term and modified the parser to parse to this.

TODO:

• Figure out the best way to store annotations in non-Term statements that we still want to annotate such as NuFunDef. Possible solutions:
  • Just add an annotations field to each of these.
  • Create an AnnStatement class, though this is quite messy and also forces us to also consider the case when AnnStatement contains a Term, which feels unclean.
• (need help) Missing case for Ann in ClassLifter class liftTerm. Still need to figure out what this function does.
• (need help) In compiler/Helpers:32, need to figure out whether this is a conversion we actually want, i.e. whether we should preserve annotations when converting to Expr. If I'm understanding correctly, Expr represents nodes in a control flow graph, so we probably don't need to preserve annotations (?)
• Update the type checker.

[LPTK]

Figure out the best way to store annotations in non-Term statements that we still want to annotate such as NuFunDef. Possible solutions:

• Just add an annotations field to each of these.

Yes, this is the way. In practice that means you only have to add the field to NuFunDef and NuTypeDef. Terms already support annotations through Ann, and other statements are either deprecated or won't support annotations.

• (need help) Missing case for Ann in ClassLifter class liftTerm. Still need to figure out what this function does.

Just add it to the catch-all case of unsupported stuff, for now. This is experimental code that's already being changed anyway.

• (need help) In compiler/Helpers:32, need to figure out whether this is a conversion we actually want, i.e. whether we should preserve annotations when converting to Expr. If I'm understanding correctly, Expr represents nodes in a control flow graph, so we probably don't need to preserve annotations (?)

Sam remark. This code is already obsolete.

• Update the type checker.

Easy. Just defer the type checking of Ann(Var(nme) :: anns, receiver) to that of Var(nme) and Ann(anns, receiver).

BTW I'm not convinced we need multiple annotations in the same Ann term. This has the downside of allowing the dummy Ann(Nil, t) and it's also kind of unnecessary. Class Term could be given a helper method peelAnnotations that would return a (List[Var], Term), if that's even needed.

[CAG2Mark]

BTW I'm not convinced we need multiple annotations in the same Ann term. This has the downside of allowing the dummy Ann(Nil, t) and it's also kind of unnecessary.

True, we can just store it as Ann(annotation1, Ann(annotation2, ...)).

[CAG2Mark]

It seems that adding an annotations field to NuTypeDef or NuFunDef creates errors in 70+ other places. Is there a better way of doing this?

[LPTK]

For such secondary fields, we normally add them to the second parameter list, which creates fewer errors as it does not affect the pattern syntax. Just add another val in there. You'll still need to initialize and propagate a bunch of them, but these are not very hard changes.

[CAG2Mark]

TODO: Fix codegen -- probably ok to just discard annotations for now. Maybe throw an error if @tailrec is seen, since it is not yet implemented.

[CAG2Mark]

TODO: Fix codegen -- probably ok to just discard annotations for now. Maybe throw an error if @tailrec is seen, since it is not yet implemented.

I see you've approved the changes, but I suppose this should be done before merging into master?

[LPTK]

Right. Forgot about that one, but it should be trivial.

I any case, I'll do the merge when it's ready.

[CAG2Mark]

My bad, probably should have rebased instead of merging. Anyway, it should be ready to merge now.

[LPTK]

My bad, probably should have rebased instead of merging. Anyway, it should be ready to merge now.

No, I know people rebase branches like this, but it's an anti-pattern. It is likely to make the commits in the middle broken, causes unnecessary conflicts, and is annoying for people (like me) who've already checked out your branch. I'm going to squash the PR on merge anyway, as its history isn't interesting/useful.

[CAG2Mark]

My bad, probably should have rebased instead of merging. Anyway, it should be ready to merge now.

No, I know people rebase branches like this, but it's an anti-pattern. It is likely to make the commits in the middle broken, causes unnecessary conflicts, and is annoying for people (like me) who've already checked out your branch. I'm going to squash the PR on merge anyway, as its history isn't interesting/useful.

Got it. What's the usual procedure for merging into the main branch here?

[LPTK]

It really depends. For short-lived PRs like this with many almost meaningless commits (like applying review suggestions), I prefer to squash. But if the branch has a lot of history by several authors or/and has been used as a base of other branches, I use a merge commit, to retain history details and reproducible commits (ie which compile and whose tests are usually green).