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

Update 2.12's scala.annotation.nowarn once 2.12.13 is out #397

Closed
dwijnand opened this issue Dec 4, 2020 · 9 comments · Fixed by #417
Closed

Update 2.12's scala.annotation.nowarn once 2.12.13 is out #397

dwijnand opened this issue Dec 4, 2020 · 9 comments · Fixed by #417
Assignees

Comments

@dwijnand
Copy link
Member

dwijnand commented Dec 4, 2020

See scala/scala-lang#1184 (comment)

2.12.13 is coming out imminently (today?). Once it's out, probably best when bumping to 2.12.13, it would be good to move that noop to 2.11 only.

But two scenarios to think about:

  1. users that upgrade scalaVersion but not scala-collection-compat (or if we don't implement this change): it's unclear what classfile for @nowarn they'll class load, which could break the configurable warning feature

  2. users that upgrade scala-collection-compat (with this implemented) but not scalaVersion: won't have a @nowarn in they're classpath any more... 😕 🤔 ...

@lrytz
Copy link
Member

lrytz commented Dec 4, 2020

  • users that upgrade scalaVersion but not scala-collection-compat (or if we don't implement this change): it's unclear what classfile for @nowarn they'll class load, which could break the configurable warning feature

are you sure things would break? even if

lazy val NowarnClass                = getClassIfDefined("scala.annotation.nowarn")

picks up the classfile from collection-compat, things could still work.

Ah, the problem is that the one here extends StaticAnnotation, but the one in 2.12.13 extends ClassfileAnnotation, so the 2.12.13 compiler would look for the string parameter in the wrong spot (annotationInfo.assocs instead of annotationInfo.args).

@SethTisue
Copy link
Member

@lrytz want to do one more round of thinking about this now that 2.12.13 is out...? I'm happy to pull publishing levers as needed once we're sure what the right thing is.

@lrytz
Copy link
Member

lrytz commented Jan 13, 2021

Maybe we can find some workaround by making scala.annotation.nowarn a type alias in this library?

Another option is to add this patch to the compiler, it would make the 2.12 nowarn implementation work with the annotation from this library (which extends StaticAnnotation instead of ClassfileAnnotation). The patch is from some other WIP branch of mine. scala/scala@6816ff0. This would only be in 2.12.14 though.

Another option is to change the annotation in this library to extends ClassfileAnnotation, i.e., make it identical to the one in 2.12.13.

@SethTisue
Copy link
Member

Another option is to change the annotation in this library to extends ClassfileAnnotation, i.e., make it identical to the one in 2.12.13.

Is there some downside to that?

@lrytz
Copy link
Member

lrytz commented Jan 13, 2021

I can't think of any issues that change would cause.

@NthPortal
Copy link
Contributor

is that binary compatible?

@SethTisue
Copy link
Member

technically no, but it shouldn't matter since it's an annotation. at least, I cannot contrive a situation in which it matters, and in a similar situation over at scala/scala#9336 (comment) , @lrytz wrote:

Changing the annotation to extends ConstantAnnotation [is] not binary compatible, but since it's an annotation that doesn't matter

@SethTisue SethTisue mentioned this issue Jan 21, 2021
10 tasks
@SethTisue SethTisue self-assigned this Jan 22, 2021
SethTisue added a commit to SethTisue/scala-collection-compat that referenced this issue Jan 26, 2021
@SethTisue
Copy link
Member

#417

@SethTisue SethTisue changed the title Drop 2.12's scala.annotation.nowarn once 2.12.13 is out Update 2.12's scala.annotation.nowarn once 2.12.13 is out Jan 26, 2021
@SethTisue
Copy link
Member

fyi @ghik (not sure if it matters over in your repo)

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 a pull request may close this issue.

4 participants