andyglow / scala-jsonschema

Scala JSON Schema
Other
122 stars 38 forks source link

Basic usages triggers compiler warnings when using -Xlint #320

Open eejbyfeldt opened 1 year ago

eejbyfeldt commented 1 year ago

Describe the bug Usage of the library triggers compiler warnings when setting -Xlint.

To Reproduce Compiling code like (based on readme examples)

package test

import json._

object Test {
  case class UserId(value: String) extends AnyVal
  implicit val userIdSchema: json.Schema[UserId] = Json.schema[UserId].toDefinition("userId")
}

with -Xlint and `-Wconf:any:warning-verbose" results in warnings like

[warn] /home/eejbyfeldt/Downloads/hYlrLUvZTGO09M01f2EN7A/src/main/scala/main.scala:7:63: Implicit resolves to enclosing value userIdSchema
[warn] Applicable -Wconf / @nowarn filters for this warning: msg=<part of the message>, cat=w-flag-self-implicit, site=test.Test.userIdSchema
[warn]   implicit val userIdSchema: json.Schema[UserId] = Json.schema[UserId].toDefinition("userId")
[warn]                                                               ^
[warn] one warning found

Expected behavior Basic usage of library should compile without warnings.

Actual results Compiler warnings.

Versions:

Additional context Add any other context about the problem here.

andyglow commented 12 months ago

I'm not really sure that it's a library bug. Please google for it

eejbyfeldt commented 12 months ago

I do not understand the relevancy of the links the first one is just saying that the flag is good for catching potentially problematic code. The second link seems to be about some other bug that is fixed later versions of scalac so it seems likely we are not hitting that bug?

So I had a look at the code to see what triggers it and found this: https://github.com/andyglow/scala-jsonschema/blob/6ebc7a20596da334211d89112885d97827927c8c/macros/src/main/scala/com/github/andyglow/jsonschema/UImplicits.scala#L17-L48 which is guess is the hack that tries to handle the the macro will resolve an implicit pointing to it self. So I guess as long as that code correct it guess the warning would be a false positive, the only problem is that code will not correctly identify self references under lots of circumstances.

Lets take for example this code:

package test

import json._

object TestNullAtRuntime {
  case class UserId(value: String) extends AnyVal
  implicit val userIdSchema: json.Schema[ // Some nice comment
   UserId
  ] = Json.schema[UserId].toDefinition("userId")

  def main(args: Array[String]): Unit = {
    println(userIdSchema)
  }
}

Trying to run this code will crash with:

Caused by: java.lang.NullPointerException
    at json.Schema$def.deepCopy$1(Schema.scala:553)
    at json.Schema$def.toDefinition(Schema.scala:556)
    at test.TestNullAtRuntime$.<clinit>(main.scala:9)
    ... 18 more

so it failed to detect the self implicit and the compiler warning is not a false positive and actually found the bug.

Code like this

package test

import json._

object TestDoesNotCompile {
  case class UserId(value: String) extends AnyVal

  def main(args: Array[String]): Unit = {
    implicit val userIdSchema: json.Schema[UserId] = Json.schema[UserId].toDefinition("userId")
    println(userIdSchema)
  }
}

will fail compilation with

./src/main/scala/main_does_not_compile.scala:9:65: forward reference to value userIdSchema defined on line 9 extends over definition of value userIdSchema
     implicit val userIdSchema: json.Schema[UserId] = Json.schema[UserId].toDefinition("userId")

But my favorite one is code like this:

package test

import json._

object TestCLRF {
  case class UserId(value: String) extends AnyVal
  implicit val userIdSchema: json.Schema[UserId] =
    Json.schema[UserId].toDefinition("userId")
  def main(args: Array[String]): Unit = {
    println(userIdSchema)
  }
}

which compiles and does the correct thing if the file as LF line endings but change it to have CRLF and it fails at runtime with

Caused by: java.lang.NullPointerException
    at json.Schema$def.deepCopy$1(Schema.scala:553)
    at json.Schema$def.toDefinition(Schema.scala:556)
    at test.TestCLRF$.<clinit>(main_clrf.scala:8)
    ... 18 more