EPA: Document and/or restructure NameAnn
In reviewing some code, I decided to educate myself a bit on the new EPA structure, and I stumbled.
Very specifically: the EpaAnnNotUsed constructor says that it's used in generated code, such as TH or deriving. Yet, this is not true in practice: the sL1n function is used much in the parser of user-written code, and it calls noAnnSrcSpan, which uses EpAnnNotUsed. So, at a minimum, the comment on EpaAnnNotUsed should be updated.
But I think there is some restructuring that we could do here.
-
I like the idea of
EpaAnnNotUsed, as currently documented (but not as used): it says when there is no useful EPA to give. But shouldn't it always be paired withUnhelpfulSpan? If I'm right -- thatEpaAnnNotUsedandUnhelpfulSpango together -- that suggests that theEpAnn/SrcSpanAnn'structure is mis-aligned. That is, maybeSrcSpanAnn'(what is that prime doing there?!) should have two constructors, one for real spans and one for unhelpful ones. This would be instead of givingEpAnntwo constructors. -
I was quite surprised reading
NameAnn: I kept looking for a constructor that would apply for normal names. I had to poke around to find out that the normal case is covered byEpaAnnNotUsed... which (to me) doesn't communicate "normal name". -
There's also something that feels off about the five difference
SrcSpanAnn?types. In particular, there isNameAnn, useful for names, andAnnListItem, useful for things that can be stored in lists. But what if I have a list of names? This happens in fixity declarations, for example. Looking atdata FixitySig-- which just stores a[LIdP pass](which storesNameAnn) -- I don't see how it's possible to store the commas correctly. That is, how do we tell the difference betweeninfix 5 *, +andinfix 5 * ,+? I really don't know! And it seems impossible to store this with the currentNameAnn/AnnListItemsplit. So something seems wrong here. -
Perhaps related: each constructor of
NameAnnhas anann_trailingfield. Lots of other annotations have a similar field, too. This seems like something that should abstracted into its own definition.
Aha! I just unlocked new understanding. The last two points are intimately related. AnnListItem is, essentially, a least-common-denominator for these annotations, containing just [TrailingAnn] (still not 100% sure what that is). This relates to some wisdom I read somewhere, which said "If you need a Located, use LocatedA now." -- that's because AnnListItem is in some sense the least informative annotation. But because all sorts of other things can also appear in lists, all the other annotation forms also include the [TrailingAnn] -- in particular, NameAnn. So names can appear in a list: they just use the nann_trailing to act like the lann_trailing field of an AnnListItem.
This all strongly suggests that the [TrailingAnn] information is misplaced. Either it should be part of the enclosing list AST node somehow, or, failing that, it should be part of EpAnn, which frequently (always?) wraps the individual annotations (like NameAnn and AnnListItem). With this change, AnnListItem stores no information and can become a type isomorphic to ().
I'm sure there's more to work out here, but at the very very least, let's redocument EpaAnnNotUsed (and maybe rename it?) to say what it's really for. Thanks!