Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review Simulacrum Scalafix annotation practices #3496

Merged

Conversation

travisbrown
Copy link
Contributor

@travisbrown travisbrown commented Jun 24, 2020

This is a follow-up to @djspiewak's question here about whether the Simulacrum Scalafix annotations dependency needs to be in the runtime classpath.

With Simulacrum 1 we used the following approach:

  • Simulacrum's annotations extended StaticAnnotation.
  • The annotations were themselves annotated with @compileTimeOnly.
  • The Simulacrum macro removed the annotations.
  • Cats's Simulacrum dependency had Provided scope.
  • Cats used POM postprocessing to remove the dependency entirely from the published artifact configuration.

With Simulacrum Scalafix we currently do the following:

  • The annotations only extend scala.annotation.Annotation.
  • Cats uses the same POM postprocessing to remove the dependency entirely.

This PR changes that to reinstate the Provided scope configuration. I'm not sure this actually does anything given the POM postprocessing already in our build, but I also don't think it hurts anything.

Some additional details

In the new model I made the annotations only extend Annotation because there's no step where we could remove them, and extending Annotation directly seems to be the recommended way to define non-macro annotations that shouldn't exist at runtime. From the Scaladoc:

Annotations extending this class directly are not preserved in the classfile. To enable storing annotations in the classfile's Scala signature and make it available to Scala reflection and other tools, the annotation needs to inherit from scala.annotation.StaticAnnotation.

I hadn't used @compileTimeOnly in Simulacrum Scalafix because it didn't seem necessary, since unlike StaticAnnotation, the Annotation-extending annotations are never preserved in the classfile. I don't think it would hurt to add the annotation, though, and will propose that in a Simulacrum Scalafix PR [update: adding the annotation fails for some reason—I guess it's only intended for use with StaticAnnotation?].

We can confirm that the Simulacrum Scalafix annotations aren't available to either Scala or Java reflection:

scala> cats.Monad.getClass.getAnnotations
res0: Array[java.lang.annotation.Annotation] = Array()

scala> reflect.runtime.universe.weakTypeOf[cats.Monad[Option]].typeSymbol.annotations
res1: List[reflect.runtime.universe.Annotation] = List(scala.annotation.implicitNotFound("Could not find an instance of Monad for ${F}"))

(This is the same both with and without Provided.)

@codecov-commenter
Copy link

Codecov Report

Merging #3496 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3496   +/-   ##
=======================================
  Coverage   91.77%   91.77%           
=======================================
  Files         383      383           
  Lines        8400     8400           
  Branches      208      218   +10     
=======================================
  Hits         7709     7709           
  Misses        691      691           

@travisbrown travisbrown changed the title Review Simulacrum Simulacrum annotation practices Review Simulacrum Scalafix annotation practices Jun 24, 2020
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. Thanks!

Copy link
Contributor

@barambani barambani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extensive explanation.

@travisbrown travisbrown merged commit c968812 into typelevel:master Jul 3, 2020
@travisbrown travisbrown added this to the 2.2.0-RC1 milestone Jul 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants