Skip to content

Conversation

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Dec 14, 2018

Uplifted from akka.annotation.

By having these in the standard library we can make use of them throughout the ecosystem and do things like build tools (like MiMa) upon their usage.

Note StaticAnnotation states:

Annotation classes defined in Scala are not stored in classfiles in a Java-compatible manner

and ClassfileAnnotation is (freshly) deprecated

Annotation classes need to be written in Java in order to be stored in classfiles in a Java-compatible manner

so I'm concerned that these might not be seen in MiMa (trivially).

Also, I retained the original name, with their upper-case, for Java users.

Finally I put these in the annotation package, as it seemed sensible (and mirrored Akka's original decision).

TODO:

  • Verify these would work with MiMa
  • Finalise the name casing choice
  • Finalise the package choice

Review (and help wanted) by @lrytz or @adriaanm, kindly.

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Dec 14, 2018
@dwijnand dwijnand added the release-notes worth highlighting in next release notes label Dec 15, 2018
@adriaanm
Copy link
Contributor

Thanks, I'm happy to have @ApiMayChange. However, not convinced about the other two.

On a related note, it'd be interesting to consider our whole story on communicating deprecation / migration / api stability / accessibility, and see about more radical improvements for the 3 series.

@dwijnand
Copy link
Member Author

  • @InternalApi -- we should improve our compilation scheme for access, or MiMa's understanding of it, instead (I know this has been a longstanding issue, so I'm willing to concede that maybe we need an annotation)

The use cases for @InternalApi include (for example) marking a method that must be public so that it may be accessed from a (non-subclass) class in another package. There's nothing in the bytecode to say that, for cross Scala/Java communications, so an annotation is the best bet. In a similar vein to @ApiMayChange I'm proposing putting it in the standard library to make it ubiquitous in the ecosystem.

  • @DoNotInherit -- we already have sealed for that. If the JVM ever adds support for sealing classes (openjdk.java.net/jeps/181 is promising), we'd use that to signal the intent to javac.

The use cases for @DoNotInherit require it not to be sealed: the annotated class is not to be extended by user-code.

On a related note, it'd be interesting to consider our whole story on communicating deprecation / migration / api stability / accessibility, and see about more radical improvements for the 3 series.

While I welcome such exploration and research, I'd love if we could start with something basic like this, which if need be we can move away from (how meta).

@adriaanm
Copy link
Contributor

adriaanm commented Dec 20, 2018 via email

@dwijnand
Copy link
Member Author

being conservative with what we add to the stdlib, especially at the root of the namespace. That stuff sticks around for 10 years.

I'm adding it in the scala.annotation package, because it needn't be in the root IMO.

Sealed classes can be extended within their compilation units. Should be enough for library use, no?

I assume you mean file. But no, particularly when you end up having multiple packages (javadsl, scaladsl, internal) you end up with multiple files. And sometimes you split those packages into independent jars, so not even the same compilation unit.

I wish I had to setup to extract the stats for you on this, from the usage in Akka.

@adriaanm
Copy link
Contributor

adriaanm commented Dec 20, 2018

I'm adding it in the scala.annotation package, because it needn't be in the root IMO.

right, root + 1 -- so, conservative - 1, but still a high-ish amount of conservatism :-)

I assume you mean file. But no,

Yeah, "compilation unit" is specese for file. Fair enough, and this is where I think we get into exploring the design space -- I think it's really interesting to consider separate accessibility for callers and subclassers.

@dwijnand dwijnand mentioned this pull request Dec 21, 2018
3 tasks
@dwijnand
Copy link
Member Author

Notes from meeting with Adriaan about this.

@ApiMayChange and @InternalApi have similar "do not touch" semantics, except that @ApiMayChange APIs might change less aggressively and are intended to stabalise at some point. So can they be merged into one?

@DoNotInherit is along the lines of a scoped sealed (sealed[akka]) and has similarities to https://openjdk.java.net/jeps/181.

My initial intents were to use MiMa to post-process the bytecode to verify and warn or error on incorrect usage, and therefore was only requesting to add these to the standard library. But Adriaan mentioned the compiler could potentially enforce these at compile time, just like sealed.

But it might be still good to have these annotations present in (what I'm calling) the "java space" of the bytecode, i.e not in the pickler, so that general java bytecode analysing tools could see them.

Adriaan, of course, is concerned about additions to the standard library, knowing what it means to remove them afterwards.

We also touched on how these annotations are not too dissimilar to @deprecated (and its lesser-known friends), and would it be possible to unify them (@unstable?).

Also, as with all changes, as soon as it's accepted you want to be able to use it, but that's not possible in a cross-compiled project (including Scala 2.11).

So on balance, I think it's better to rally users around an external library and tooling around this, and later reconsider uplifting one, the other or both into Scala proper.

@dwijnand dwijnand closed this Dec 21, 2018
@dwijnand dwijnand deleted the akka-annotations branch December 21, 2018 11:50
@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Jan 9, 2019
@SethTisue SethTisue removed the release-notes worth highlighting in next release notes label Jan 9, 2019
@dwijnand dwijnand restored the akka-annotations branch March 18, 2019 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants