Skip to content

case class no longer deprecates itself#10071

Merged
lrytz merged 1 commit intoscala:2.13.xfrom
som-snytt:tweak/2799-case-class-deprecates-itself
Jul 26, 2022
Merged

case class no longer deprecates itself#10071
lrytz merged 1 commit intoscala:2.13.xfrom
som-snytt:tweak/2799-case-class-deprecates-itself

Conversation

@som-snytt
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

som-snytt commented Jul 11, 2022

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
SethTisue
SethTisue approved these changes Jul 19, 2022
View reviewed changes
@som-snytt
Copy link
Copy Markdown
Contributor Author

som-snytt commented Jul 19, 2022

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
Copy Markdown
Contributor Author

@som-snytt som-snytt Jul 19, 2022

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
Copy Markdown
Member

@lrytz lrytz Jul 26, 2022

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
Copy Markdown
Contributor Author

@som-snytt som-snytt Jul 26, 2022

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
Copy Markdown
Member

SethTisue commented Jul 19, 2022

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
Copy Markdown
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
Copy Markdown
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