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.

Sign up for GitHub

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

Jump to bottom

case class no longer deprecates itself #10071

Merged
merged 1 commit into from
Jul 26, 2022
Merged

case class no longer deprecates itself #10071

merged 1 commit into from
Jul 26, 2022

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jul 11, 2022 •
edited
Loading

Reverts the ancient fix for scala/bug#2799 at 95d7ef4

Surprised to see nothing broke. It would be nice to know why and apply that fairydust to Dotty. (Edit: actually, I mentioned scala.io.Position on the dotty ticket.)

Instead of juggling annotations, the deprecation loophole is expanded to look more like the tax code.

@scala-jenkins scala-jenkins added this to the 2.13.10 milestone Jul 11, 2022
@som-snytt
Copy link
Contributor Author

This is the case (or case class) that wasn't fixed, but the user said on their ticket is exactly their use case.

scala> case class D @deprecated("ctor D is depr", since="now") (i: Int)
                  ^
       warning: constructor D in class D is deprecated (since now): ctor D is depr
class D

That was jroper on scala/bug#8685

@som-snytt som-snytt force-pushed the tweak/2799-case-class-deprecates-itself branch 2 times, most recently from a1a638e to f35539c Compare July 11, 2022 17:58
@som-snytt som-snytt mentioned this pull request Jul 11, 2022
Closed
@som-snytt som-snytt marked this pull request as ready for review July 13, 2022 07:41
@SethTisue SethTisue modified the milestones: 2.13.10, 2.13.9 Jul 19, 2022
@som-snytt
Copy link
Contributor Author

SLS 11.2.3 isn't quite current, but specifies the exclusion for deprecation. The revision would add "...or has a deprecated companion class or module."

Deprecated warnings are suppressed in code that belongs itself to a definition that is labeled deprecated.

som-snytt
som-snytt commented Jul 19, 2022
View reviewed changes
src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
// or an element that is synthetic or has a deprecated companion.
if (sym.isDeprecated &&
(!currentOwner.isSynthetic || currentOwner.isAnonymousFunction || currentOwner.isParameter) &&
!currentOwner.ownersIterator.exists(s => s.isDeprecated || s.companionClass.isDeprecated))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The condition for isSynthetic is unfortunately tweaky. Is there a better way to ask, Did the user introduce this symbol? I might have guessed isArtifact.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of using isSynthetic and then trying to get back warnings that should show up, could we do something more specific for case classes?

I'm pushing an attempt, let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is narrowly focused to address that issue. There is no reason to over-generalize.

@SethTisue
Copy link
Member

although I hit approve, probably @lrytz should have a look too

@SethTisue SethTisue requested a review from lrytz July 19, 2022 17:35
som-snytt
som-snytt commented Jul 26, 2022
View reviewed changes
Copy link
Contributor Author

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

Thanks, I was going to nowarn that code as Seth requested, sorry to make work for you.

Also tweak the spec. I'll do that in a separate PR so this can be merged with due speed.

Oh, I can't "approve", only "comment" that I approve the additional commits.

"I'm the PR author, and I approve these commits."

@som-snytt @lrytz
Handle case class deprecation more directly
e0dcca4 
Revert adding deprecation to companion.
Instead, check companions for deprecation in refchecks.
@lrytz lrytz force-pushed the tweak/2799-case-class-deprecates-itself branch from 36e5912 to e0dcca4 Compare July 26, 2022 13:37
@lrytz
Copy link
Member

lrytz commented Jul 26, 2022

Thanks!

@lrytz lrytz merged commit 9dba58b into scala:2.13.x Jul 26, 2022
@som-snytt som-snytt deleted the tweak/2799-case-class-deprecates-itself branch July 26, 2022 16:30
@som-snytt som-snytt mentioned this pull request Feb 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SethTisue SethTisue SethTisue approved these changes

@lrytz lrytz Awaiting requested review from lrytz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.13.9
Development

Successfully merging this pull request may close these issues.
4 participants
@som-snytt @SethTisue @lrytz @scala-jenkins