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

Make more annotations extend ConstantAnnotation #9336

Merged
merged 1 commit into from Mar 17, 2021

Conversation

BalmungSan
Copy link
Contributor

@BalmungSan BalmungSan commented Nov 21, 2020

This makes it clearer that any custom error messages (e.g. on implicitNotFound) must be literal values.

Fixes scala/bug#10424

@scala-jenkins scala-jenkins added this to the 2.13.5 milestone Nov 21, 2020
@SethTisue SethTisue added the welcome hello new contributor! label Nov 21, 2020
@lrytz
Copy link
Member

lrytz commented Dec 15, 2020

I rebased this on top of #9379 and changed both implicitNotFound and implicitAmbiguous to extend ConstantAnnotation.

There are a bunch of workarounds required to get this green because the STARR compiler (2.13.4) doesn't yet have the fixes from #9379 and some additional compiler fixes from this PR (extract the implicitNotFound message from AnnotationInfo.assocs instead of AnnotationInfo.args - the data moves when starting to use ConstantAnnotation). User-code will not need such workarounds, the only change for users will be that the compiler enforces the implicitNotFound / implicitAmbiguous messages to be compile-time constants.

To avoid the internal workarounds, we can first merge only the compiler changes but keep implicitNotFound / implicitAmbiguous as-is until 2.13.5 is out. For me, either way is fine.

There are additional annotations that we should change to extend ConstantAnnotation: @deprecated, @deprecatedName, @deprecatedInheritance, @deprecatedOverriding, @elidable, @migration. Doing that before a restarr might require more temporary workarounds.

summon[@SethTisue.type]

@dwijnand
Copy link
Member

Needs a rebase to not conflict. #9379 is now merged.

dwijnand
dwijnand previously approved these changes Dec 17, 2020
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM

@dwijnand dwijnand dismissed their stale review December 17, 2020 16:45

Nvm, just realised this is based on a previous version of #9379. Needs a review after rebase.

@lrytz lrytz requested a review from SethTisue December 18, 2020 08:09
@SethTisue SethTisue self-assigned this Jan 6, 2021
@SethTisue
Copy link
Member

SethTisue commented Jan 26, 2021

@BalmungSan #9379 is merged now. Could you rebase this and remove all in the insertions of msg =, which iiuc shouldn't be necessary anymore?

also the PR title and description need revision now that the scope of the PR has broadened

@BalmungSan
Copy link
Contributor Author

BalmungSan commented Jan 26, 2021

@SethTisue let me know if I rebased it correctly, or if I screw it up (I have a backup just in case).

About the title, I think it is fine, at the end of the day this PR is just enforcing the use of a literal value for the annotation message and improving the documentation to reflect that.

BTW, should I add the same documentation message to implicitAmbiguous?
Also, I am not sure if I should have also removed the changes from this commit?


Judging from the failed CI, I did screw it up.

@SethTisue
Copy link
Member

SethTisue commented Jan 29, 2021

@BalmungSan It seems the story got a bit more tangled than any of us expected. Your original goal here was simply to improve the implicitNotFound documentation, but that ended up leading us into deeper waters. I see now that I did not read Lukas's early remarks closely enough until now. You didn't screw up, it's me who suggested you take an approach that wouldn't work, or rather, won't work yet.

Let's proceed as follows:

  1. I am closing this PR. Soon, I and/or Lukas will submit a different PR that includes only the Lukas's compiler changes that will make changing the annotations later possible.
  2. After 2.13.5 is released and we re-STARR, I will submit a second PR reopen this PR and include the rest of the changes, including changing the other annotations Lukas suggests. I will keep your commit with your authorship.

@SethTisue SethTisue closed this Jan 29, 2021
@SethTisue SethTisue removed this from the 2.13.5 milestone Jan 29, 2021
@SethTisue
Copy link
Member

@lrytz And before I can do that, I have a question. You wrote:

There are a bunch of workarounds required to get this green because the STARR compiler (2.13.4) doesn't yet have the fixes from #9379 and some additional compiler fixes from this PR (extract the implicitNotFound message from AnnotationInfo.assocs instead of AnnotationInfo.args - the data moves when starting to use ConstantAnnotation). User-code will not need such workarounds, the only change for users will be that the compiler enforces the implicitNotFound / implicitAmbiguous messages to be compile-time constants.

If I understand correctly, that means that some of the changes to AnnotationInfos.scala in this PR should be made now for 2.13.5, and some others are "workarounds" that won't be needed now that we're doing this in stages. Can you help me distinguish which is which? (Or perhaps it would be easier for you to just submit the 2.13.5 PR yourself than to verbally characterize it to me; I don't know.)

@lrytz
Copy link
Member

lrytz commented Jan 29, 2021

Yes, that's right. I'll split up the PR so we can do the changes after the next restarr.

@lrytz
Copy link
Member

lrytz commented Jan 29, 2021

#9463

@SethTisue
Copy link
Member

SethTisue commented Mar 16, 2021

I was unable to change @deprecatedName, since it has auxiliary constructors. Those constructors are deprecated, so we'll be able to make the change for Scala 3.x. I'll add a comment about it.

(Or, we could just remove the auxiliary constructors now, on the grounds that bincompat doesn't matter here since it's compile-time stuff? I've chosen to play it safe, but I don't have a strong opinion about it.)

@SethTisue SethTisue changed the title Improve clarity of implicitNotFound documentation Make more annotations extend ConstantAnnotation Mar 16, 2021
@SethTisue SethTisue added the library:base Changes to the base library, i.e. Function Tuples Try label Mar 16, 2021
@lrytz
Copy link
Member

lrytz commented Mar 17, 2021

This needs some MiMa excludes and a check file update.

I agree to leave @deprecatedName

(now that it's possible to do so, after scala#9463)

In this case of e.g. `implicitNotFound`, this makes it clearer that
the custom error message must be a literal value.

Fixes scala#10424

Co-authored-by: Seth Tisue <seth@tisue.net>
@SethTisue
Copy link
Member

welcome, new Scala contributor!

@BalmungSan BalmungSan deleted the patch-1 branch March 17, 2021 18:35
@SethTisue
Copy link
Member

SethTisue commented Mar 30, 2021

it came up in the community build that something like this no longer compiles:

  @deprecated(
    """|Empty parts are not allowed by the multipart spec, see: https://tools.ietf.org/html/rfc7578#section-4.2          
       |Moreover, it allows the creation of potentially incorrect multipart bodies                                       
       |""".stripMargin,
    "0.18.12"
  )

because using .stripMargin makes it not a constant. I guess this is acceptable though

SethTisue added a commit to scalacommunitybuild/http4s that referenced this pull request Mar 30, 2021
@BalmungSan
Copy link
Contributor Author

@SethTisue note that the annotation was ignored since it wasn't a constant anyways.

Thus I would say that a compilation error is good.

@lrytz
Copy link
Member

lrytz commented Mar 31, 2021

Yes, getting this error is exactly the the goal of this PR

@lrytz lrytz added the release-notes worth highlighting in next release notes label May 17, 2021
mkurz added a commit to mkurz/play-slick that referenced this pull request Jan 5, 2022
Otherwise scaladoc will complain in Scala 2.13.6
See scala/scala#9336
mergify bot pushed a commit to playframework/play-slick that referenced this pull request Jan 5, 2022
Otherwise scaladoc will complain in Scala 2.13.6
See scala/scala#9336

(cherry picked from commit 27a7f83)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library:base Changes to the base library, i.e. Function Tuples Try release-notes worth highlighting in next release notes welcome hello new contributor!
Projects
None yet
5 participants