Add complete support for Java annotations in SemanticDB

[Strum355]

This PR expands the SemanticDB spec to allow for representing Annotation AST's (incl. parameters) by introducing two new types to the Tree family: AnnotationTree and AssignTree.

As AnnotationTree is binary compatible with the Annotation type, this proposal deprecates Annotation in favour of AnnotationTree, replacing its usages in SymbolInformation and AnnotatedType. Alternatively, I would also be open to adding Annotation to the Tree oneof instead, if source-level backwards compatibility is also strongly desired.

The specification doc is updated to include some more examples, given the new capabilities to also model the parameters of annotations.

Working example of rendering annotations from the structures built via the changes in this PR can be found at sourcegraph/scip-java#148

[olafurpg]

Thank you for this contribution. This is a fantastic milestone! Lack of comprehensive support for annotations has always been a weak spot in SemanticDB and it's really exciting to see that we now have a complete implementation for Java annotations.

I have a few minor comments, otherwise this looks good to me 👍

[olafurpg]

Suggested change

	repeated AnnotationTree annotations = 13;
	repeated AnnotationTree annotations = 13; // NOTE: it's possible to manually change the type to `Annotation` for source compatibility with older SemanticDB versions.

[olafurpg]

@tgodzik @gabro I'm curious to hear your thoughts about this Protobuf binary-compatible but Scala binary+source-incompatible change. It's possible to keep the old Annotation signature to preserve source compatibility but the new AnnotationTree is more consistently named with other Tree subclasses.

A few sidenotes:

• The ScalaPB-generated classes have intentionally never been marked as public APIs exactly so that we can make Protobuf binary-compatible changes like this.
• SemanticDB annotations are not really used by any meaningful Scalameta project from what I can tell because they are incomplete for Scala (no parameters until this PR) and they were officially unsupported for Java.
• Scalafix will be able to incorporate these changes into its public API without breaking changes.

WDYT?

[kitbellew]

still relevant?

[Strum355]

still relevant?

We've been using this in https://github.com/sourcegraph/scip-java for the past ~2 years, along with #2292 which also has not been merged here yet

[tgodzik]

I think the issue was the Scala binary+source-incompatible change and we never really settled if it's ok to break it. I think the biggest issue would be with Scalafix, @bjaglin do you think the breaking change for semanticdb in this PR might cause issues for Scalafix?

Maybe we could keep Annotation at least as an existing class and just AnnotationTree as a completely new thing? This might break some logic, but not break binary compat.

[bjaglin]

That change looks non-breaking from a Scalafix perspective as ScalaPB classes are not leaking to the public Scalafix API (they are all converted to case classes manually-defined in scalafix.v1).

When parameters are actually filled by semanticdb-scalac (since they don't seem in this PR?), we'd have to see if it is possible to expose scalafix.v1.AnnotationTree through scalafix.v1.AnnotatedType in a smooth way, but that's another story.

[tgodzik]

So I think we should be good to merge this, what do you think @kitbellew ?

[kitbellew]

@tgodzik looks like it.

i am confused about one thing, however: do we want to add repeated Tree parameters = 2 to the existing Annotation, instead of adding option deprecated = true to it and introducing a new AnnotationTree? we discussed this but I am not sure what has been decided.

[tgodzik]

I think we want to drop the old Annotation by making it deprecated and create the new class. Not sure though how things are protobug compatible, but since java-semanticdb has been using it for a while it seems it didn't break anything in Metals so the change here is safe.

[Strum355]

Not sure though how things are protobug compatible

AnnotationTree, at the protobuf byte-level, is essentially just an extension of Annotation: the latter had a field at index 1 of type Type, the former also has a field at index 1 of type Type and an additional (repeated) field at index 2 of type Tree. So long we're not changing the type of any existing fields of Annotation in AnnotationTree to something that is recursively of a different type (as type names don't matter here, only their byte representation), then itll deserialize successfully. Hope that answers the question 🙂

[kitbellew]

Not sure though how things are protobug compatible

AnnotationTree, at the protobuf byte-level, is essentially just an extension of Annotation: the latter had a field at index 1 of type Type, the former also has a field at index 1 of type Type and an additional (repeated) field at index 2 of type Tree. So long we're not changing the type of any existing fields of Annotation in AnnotationTree to something that is recursively of a different type (as type names don't matter here, only their byte representation), then itll deserialize successfully. Hope that answers the question 🙂

does this mean we don't need to create a new type, and can stick with the old one? protobuf is backwards-compatible when it comes to adding new fields, is our usage of it as well?

[Strum355]

Not sure though how things are protobug compatible

AnnotationTree, at the protobuf byte-level, is essentially just an extension of Annotation: the latter had a field at index 1 of type Type, the former also has a field at index 1 of type Type and an additional (repeated) field at index 2 of type Tree. So long we're not changing the type of any existing fields of Annotation in AnnotationTree to something that is recursively of a different type (as type names don't matter here, only their byte representation), then itll deserialize successfully. Hope that answers the question 🙂

does this mean we don't need to create a new type, and can stick with the old one? protobuf is backwards-compatible when it comes to adding new fields, is our usage of it as well?

I guess technically? I dont remember if there was a reason beyond having the naming more aligned with the rest of the *Tree types, its been a while since this was opened.

This change is byte compatible, yes, but not source compatible. Scala code would most likely complain as theyre different types in the generated code, but programs using either the old or the new schema can communicate with each other just fine