deprecated case class warns at its definition

[bishabosha]

Minimized code

@deprecated("no CaseClass", "0.1") case class CaseClass(rgb: Int)

Output

-- Deprecation Warning: CaseClass.scala:1:0 ----
21 |@deprecated("no CaseClass", "0.1") case class CaseClass(rgb: Int)
   |^
   |class CaseClass is deprecated since 0.1: no CaseClass
-- Deprecation Warning: CaseClass.scala:1:46 ---
21 |@deprecated("no CaseClass", "0.1") case class CaseClass(rgb: Int)
   |                                              ^
   |                   class CaseClass is deprecated since 0.1: no CaseClass

Expectation

no warning as in Scala 2.13.4.

[gregbeech]

This is also an issue for deprecated implicit classes. I assume it's the same basic issue so I'll just add this as a comment rather than opening a new issue.

Minimized Code

object repro {
  @deprecated("no ImplicitClass", "0.1") implicit class ImplicitClass(val a: Any)
}

Output

[warn] 2 |  @deprecated("no ImplicitClass", "0.1") implicit class ImplicitClass(val a: Any)
[warn]   |  ^
[warn]   |class ImplicitClass in object repro is deprecated since 0.1: no ImplicitClass

Expectation

No warning, as in Scala 2.13.6

[SethTisue]

would this be a good issue-spree ticket?

[som-snytt]

I was going to address it after scala/scala#10071 which ... how did I put it ...

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

[He-Pin]

Any update about this @som-snytt ,thanks.

[som-snytt]

@He-Pin by coincidence, yesterday I deleted the old branch from a year ago, but got distracted by warning on deprecated constant-folded ops. I'll return to this right after that. The interesting lesson from that other fix is that because of structural differences in the pipeline, fixes don't necessarily forward-port directly.

[jan-pieter]

When adding a companion object with a deprecated annotation, the warning disappears:

@deprecated("no CaseClass", "0.1")
case class CaseClass(rgb: Int)

@deprecated("no CaseClass", "0.1")
object CaseClass

[som-snytt]

The draft PR is the Scala 2 fix, which specifies suppression by deprecated companion of enclosing element, but the fix is less fussy than Scala 2, at least as a first attempt.

Previously excluded was the linked issue #12706

[mbovel]

As there now is a work-in-progress pull request (#16918) fixing this issue, I remove the “Spree” label so that we don't work on it in parallel.

[som-snytt]

I haven't had a chance to return to this issue (both Scala 2 and 3) to evaluate whether my proposal is a bad idea. (That must happen before 2.13.11.) Thanks for the bump.

[scala-center-bot]

This issue was picked for the Issue Spree No. 28 of 28 March 2023 which takes place in a week from now. @SethTisue, @nmcb, @jan-pieter will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

[som-snytt]

Um, I had an idea but maybe don't go down that alley. Thanks, scala-center-bot!

Edit: to summarize, Scala 2 only excludes the case ctor call. The rejected idea was that companion of a deprecated class should not see warnings for usages; instead of that implicit exclusion, the companion or its member must explicitly choose deprecated or nowarn, which is easy to do and should be used sparingly.

Sample changes: object higherKinds extends higherKinds should be deprecated. object Position extends Position was actually concealing deprecated usages of 42 + "str", so targeted use of nowarn is appropriate.

[SethTisue]

As there now is a work-in-progress pull request (#12706) fixing this issue

I guess you mean #16918

[mbovel]

I guess you mean #16918

Indeed! I fixed it in my original comment.

[SethTisue]

The Scala 2 PR for this, scala/scala#10071, was merged a while back.

@som-snytt thanks for your helpful remarks above, last week.

The approach taken in the Scala 2 PR (namely "only exclude the case ctor call") seems satisfactory to me, but I guess we should begin our session by seeing whether we agree on that.

[SethTisue]

If we do agree on that, then we don't need to look at #16918, since it takes a different approach, and it doesn't have any test cases that aren't also in scala/scala#10071 .

[SethTisue]

(It actually was helpful to look at #16918, not to copy the whole approach, but to steal little bits and pieces of implementation. Thanks @som-snytt.)

Here's where things stand: we feel like we're close to having it right, and we just ran out of time. The part we got stuck on is checking whether it's specifically the constructor that we're calling. We want to call isConstructor but we're at the case class symbol rather than at the method symbol and we don't understand why we're at the wrong symbol.

Anyway, the next step is that I'll ask @dwijnand to have a look, though it's possible that when I prepare to talk to him I'll spot the problem myself. @nmcb and @jan-pieter, feel free to take your own stab at it in parallel. We shouldn't wait six weeks or we'll forget where we were :-)

@jan-pieter will open a draft PR as a reference point.

[SethTisue]

@nmcb @jan-pieter wdyt about this as the possible culprit? CrossVersionChecks.scala:141

  override def transformNew(tree: New)(using Context): New = {
    checkUndesiredProperties(tree.tpe.typeSymbol, tree.srcPos)
    tree
  }

New is the tree node for a constructor call, but note that the symbol passed is tree.tpe.typeSymbol — which symbol is that? doesn't look offhand like it would be the constructor symbol, could be the case class's symbol

whereas we were looking for a Apply(Select(...)) which is how normal method calls are represented

I think I would have spotted this if we'd had literally 3 more minutes 😅

I'm familiar with New from my Fortify work, so I don't know why I didn't think of this sooner

[jan-pieter]

Both the transformNew and transformTypeTree entrypoints were triggering the warning. Do you think we need to only apply the exemption defined here when coming from those entrypoints?