scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

"method is deprecated" warning is not produced for `kotlin.Deprecated` #12714

Closed unkarjedy closed 1 year ago

unkarjedy commented 1 year ago

Reproduction steps

Scala 2.13.10

  1. Use this example project. It contains usages of Java, Scala and Kotlin deprecated methods.
    
    ThisBuild / scalacOptions ++= Seq("-deprecation")

lazy val root = (project in file(".")) .settings( name := "scala-java-kotlin-deprecated-warnings", scalaVersion := "2.13.10", libraryDependencies ++= Seq( "org.jetbrains.kotlin" % "kotlin-stdlib" % "1.7.22" ) )


```scala
object MainScala {
  def main(args: Array[String]): Unit = {
    //Scala deprecated
    scala.io.Source.fromRawBytes(Array())

    //Java deprecated
    (null: java.rmi.server.LoaderHandler).loadClass(null, null)

    //Kotlin deprecated fun
    (null: kotlin.DeepRecursiveScope[_, _]).invoke(null, null)

    //Kotlin deprecated class
    abstract class MyClass extends kotlin.UseExperimental//(null) needed for Scala 3 only
  }
}
  1. Compile

Actual result

Observe deprecation warnings (1 for Scala, 2 for Java):

method fromRawBytes in object Source is deprecated (since 2.13.9): Use `fromBytes` and specify an encoding
method loadClass in trait LoaderHandler is deprecated
trait LoaderHandler in package server is deprecated

There are no deprecation warnings for Kotlin members (marked with kotlin.Deprecated annotation)

Expected result

Deprecation warnings are generated for Kotlin members (marked with kotlin.Deprecated annotation)

Note 1

Scala 3 compiler can generate deprecation warnings for Kotlin members

Note 2

In IntelliJ Scala Plugin we are strongly bound to IntelliJ platform jars. A huge part of it is written in Kotlin. We use "fatal warnings" not to miss important deprecations from the platform in new updates. Usually, there are a lot of deprecations and we want to detect them. Today I noticed that our build doesn't fail if the IntelliJ platform deprecates some API written in Kotlin. The fix seems to be relatively easy, so would be grateful to see it in minor scala releases.

image

image

unkarjedy commented 1 year ago

In Scala 3 it works because when class files are parsed, it detects Deprecated Attribute and adds synthetic scala.deprecated annotation to the class definition. The code is located here. Members marked with java.lang.Deprecated and kotlin.Deprecated annotations have Deprecated Attribute in the classfiles.

In Scala 2 compiler there is similar logic, but seems it only handles classes.

som-snytt commented 1 year ago

This issue stopped working after 2.13.2 due to tweaks in this area for Java deprecations. I'm taking a look...

To be clear, it's not warning for either the method or extending UseExperimental.

SethTisue commented 1 year ago

@unkarjedy nice catch — thanks for reporting. looks like Som's PR will resolve it

unkarjedy commented 1 year ago

@SethTisue Do you know if it's intentional that Java & Kotlin deprecation annotations on Scala definitions are ignored? I understand that this code is not the most popular. But maybe it would be a simple change. (BTW it's the same in Scala 3)

object MainScala {
  def main(args: Array[String]): Unit = {
    println(new MyClassDeprecatedScala)
    println(new MyClassDeprecatedJava)
    println(new MyClassDeprecatedKotlin)

    println(MyObjectDeprecatedScala)
    println(MyObjectDeprecatedJava)
    println(MyObjectDeprecatedKotlin)

    println(Wrapper.myDeprecactedScala)
    println(Wrapper.myDeprecactedJava)
    println(Wrapper.myDeprecatedKotlin)
  }
}

@scala.deprecated class MyClassDeprecatedScala
@java.lang.Deprecated class MyClassDeprecatedJava
@kotlin.Deprecated("") class MyClassDeprecatedKotlin

@scala.deprecated object MyObjectDeprecatedScala
@java.lang.Deprecated object MyObjectDeprecatedJava
@kotlin.Deprecated("") object MyObjectDeprecatedKotlin

object Wrapper {
  @scala.deprecated def myDeprecatedScala: String = ???
  @java.lang.Deprecated def myDeprecactedJava: String = ???
  @kotlin.Deprecated("") def myDeprecatedKotlin: String = ???
}
image
som-snytt commented 1 year ago

For some definition of intention.

def isDeprecated           = hasAnnotation(DeprecatedAttr) || (isJava && hasAnnotation(JavaDeprecatedAttr))

In case it is not obvious, annotating a Scala element with the Java Deprecated does not make it deprecated in the class file (with respect to the deprecated attribute). However, it seems javac will honor the (runtime visible) annotation and report the deprecation.

While working on the related issue, I did wonder why Scala doesn't have a uniform notion of deprecation, perhaps with arbitrary metadata (to accommodate forRemoval, for example). But the deprecated attribute is the gold standard; the extra work in the recent change was to preserve the extra metadata if the Java annotation is also present in the class file (in particular, to preserve since).

Other questions: why hasn't this come up before? is it up to arbitrary tools (such as an IDE) to use arbitrary annotations which mean deprecation? also why is it up to the compiler to warn about everything.

unkarjedy commented 1 year ago

why hasn't this come up before?

I noticed this accidentally and for the record, it doesn't hurt us. Due to some internal tooling, I had to use Java @Deprecated annotation and use it in our code. We use -Xfatal-warnings so I expected the code to not compile. I was surprised to see that it doesn't

is it up to arbitrary tools (such as an IDE) to use arbitrary annotations which mean deprecation?

I wouldn't call java.lang.Deprecated an "arbitrary" annotation taking that java.lang.* is one of the default Scala imports. And I mentioned kotlin.Deprecated mostly for because I thought of checking it as well, just out of curiosity. It might indeed be less actual compared to java.lang.Deprecated

I will also mention that explicitly, just in case. In the screenshot above, I use IntelliJ IDEA with compiler-based highlighting, so the warnings are from the compiler. If I use IntelliJ IDEA native highlighting, it can highlight that code as deprecated (though detecting kotlin.Deprecated didn't work and I already fixed it locally)

image

also why is it up to the compiler to warn about everything

As I mentioned, at least IntelliJ IDEA users won't suffer from the absence of the warning. But having an IDE warning is a complementary thing which will help you in the editor but won't save from potential issues in the whole project, unless you run IDEA inspections for the whole project. Having it in the compiler + using -Xfatal-warnings can save you from potential issues.

There are also those who love writing code in code editors :)

And clearly Scala compiler chose a direction to introduce more diagnostics inside the compiler itself, at least in Scala 3.

unkarjedy commented 1 year ago

@som-snytt Recognising at least @java.lang.Deprecated could be helpful. It could reveal potential bugs in existing projects. I am sure that over the years there were people who made a typo and wrote @Deprecated instead of @deprecated (Think about how many Scala developers come from Java.. 🙂)

som-snytt commented 1 year ago

Native highlighting FTW! Thanks for the screenshot.

Although I'm guilty of introducing warnings, I prefer that the compiler did not warn, and that it magically exposed enough instrumentation for external tools; but under public pressure to produce warnings, inevitably the compiler does its own linting.

Ha, I was also about to ask did no one mistype Deprecated after all these years of mixed compilation? That would be an obvious lint (unless it gets supported).