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 -- thatEpaAnnNotUsed
andUnhelpfulSpan
go 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 givingEpAnn
two 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
/AnnListItem
split. So something seems wrong here. -
Perhaps related: each constructor of
NameAnn
has anann_trailing
field. 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!