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.

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

-Wunused:all and nested type class derivation #16679

Closed
amumurst opened this issue Jan 13, 2023 · 7 comments · Fixed by #17095
Closed

-Wunused:all and nested type class derivation #16679

amumurst opened this issue Jan 13, 2023 · 7 comments · Fixed by #17095
Assignees
Labels
area:linting Linting warnings enabled with -W or -Xlint itype:bug
Milestone

Comments

@amumurst
Copy link

amumurst commented Jan 13, 2023

I tried adding the new unused flag (from #16157) to my work codebase and was surprised that it warned about unused import when used with the json library circes derivation of json Encoders from case classes. ( case class Foo(i: Int) derives Encoder.AsObject). A minimal reproducer independent of circe is below. I see no error when trying to derive the "top level" typeclass and not a subtype.

Compiler version

With nightly: 3.3.0-RC1-bin-20230112-be10bc6-NIGHTLY

Minimized code

//> using scala "3.3.0-RC1-bin-20230112-be10bc6-NIGHTLY"
//> using option "-Wunused:all"

object myPackage:
   trait CaseClassName[A]:
      def name: String

   object CaseClassName:
      trait CaseClassByStringName[A] extends CaseClassName[A]
      import scala.deriving.Mirror
      object CaseClassByStringName:
         inline final def derived[A](using inline A: Mirror.Of[A]): CaseClassByStringName[A] = 
            new CaseClassByStringName[A]:
               def name: String = A.toString

import myPackage.CaseClassName
case class CoolClass(i: Int) derives CaseClassName.CaseClassByStringName
println(summon[CaseClassName[CoolClass]].name)

Output

[warn] ./unused-typeclass-derivation.sc:16:18: unused import
[warn] import myPackage.CaseClassName
[warn]                  ^^^^^^^^^^^^^

Expectation

The import is needed and used 2 places (at the summon and at the derives), so no warn should be provided.

@amumurst amumurst added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Jan 13, 2023
@szymon-rd szymon-rd self-assigned this Jan 13, 2023
@szymon-rd szymon-rd added area:reporting Error reporting including formatting, implicit suggestions, etc and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jan 13, 2023
@Kordyjan Kordyjan added this to the 3.3.0 backports milestone Jan 13, 2023
@PaulCoral
Copy link
Contributor

Thanks for reporting all this -Wunused issue, @amumurst !

I see no error when trying to derive the "top level" typeclass and not a subtype.

Can you give me a code example of this. It would help me a lot debuging this. I do not really understand what you meant here.

@amumurst
Copy link
Author

amumurst commented Jan 15, 2023

Just that this does not give a warning where there is no subtype.

object myPackage:
   trait CaseClassName[A]:
      def name: String

   object CaseClassName:
      import scala.deriving.Mirror
      inline final def derived[A](using inline A: Mirror.Of[A]): CaseClassName[A] = 
         new CaseClassName[A]:
            def name: String = A.toString

import myPackage.CaseClassName
case class CoolClass(i: Int) derives CaseClassName
println(summon[myPackage.CaseClassName[CoolClass]].name)

But I see now that its not really that relevant. This also fails, so its more about derives X.Y than anything specific to subtyping

More minimized

With this setup

object myPackage:
   object Foo:
      trait CaseClassName[A]:
         def name: String

      object CaseClassName:
         inline final def derived[A](using inline A:  scala.deriving.Mirror.Of[A]): CaseClassName[A] = 
            new CaseClassName[A]:
               def name: String = A.toString

This fails

import myPackage.Foo
case class CoolClass(i: Int) derives Foo.CaseClassName
println(summon[Foo.CaseClassName[CoolClass]].name)

But this works

import myPackage.Foo.CaseClassName
case class CoolClass(i: Int) derives CaseClassName
println(summon[CaseClassName[CoolClass]].name)

@ivan-klass
Copy link

Nested term selection is not required to be after derives to reproduce, if import from an object directly, false-positive is still there:

package example

import io.circe.Encoder.{AsObject => ToJson}

case class Foo(bar: String, baz: Int) derives ToJson
[warn] 3 |import io.circe.Encoder.{AsObject => ToJson}
[warn]   |                         ^^^^^^^^^^^^^^^^^^
[warn]   |                         unused import

@amumurst
Copy link
Author

Aha, my minimised example picked it up as used since it was used in the summon 🤦 @PaulCoral

@jchyb jchyb added area:linting Linting warnings enabled with -W or -Xlint and removed area:reporting Error reporting including formatting, implicit suggestions, etc labels Feb 12, 2023
@ivan-klass
Copy link

ivan-klass commented Feb 25, 2023

Disclaimer: I'm not an expert here but just a curious dotty user impatient to -Wunused flag which now is still unusable because of false-positives. And here is some hints I've found:

I'm not sure if CheckUnused is a right place to explicitly traverse DerivingTemplate to workaround traverseChildren behavior, but I'll try to compose a draft PR

@szymon-rd
Copy link
Contributor

szymon-rd commented Mar 7, 2023

@ivan-klass thank you for your observations, I am looking into it right now.

@szymon-rd
Copy link
Contributor

szymon-rd commented Mar 13, 2023

The reason behind this issue was the way the Idents were generated in the derived code. If you wrote:

object Foo:
   object Bar

Foo.Bar

Then, Idents for Foo and Bar would appear in the AST. However, in the derived code, only Foo$.Bar appeared. What's more, the name had to be treated a bit differently (ignored) to accommodate for the usage of the module class (name with $ sign).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:linting Linting warnings enabled with -W or -Xlint itype:bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants