Exclude macro generated code from WartRemover checks

[baldram]

Hi there! Thanks for working on this amazing library! I have a suggestion for improvement.
It's probably just one line of code added to the macro code.
The link to an example solution in another project is at the very bottom.

Checklist

• I read the documentation at https://chimney.readthedocs.io/ and checked that the functionality exists
• I verified that the behavior for my use case doesn't match the documentation
• I checked the https://github.com/scalalandio/chimney/issues and haven't found the issue reported
  • I checked the https://github.com/scala/bug/issues and haven't found the issue reported there - relevant for Scala 2 compiler crashes, since it might be a bug in the compiler
  • I checked the https://github.com/lampepfl/dotty/issues and haven't found the issue reported there - relevant for Scala 3 compiler crashes, since it might be a bug in the compiler
• I confirmed that the bug is not related to functionality that was deprecated: lifted transformers (TransformerFs) or unsafeOption flags
• I believe that this is actual bug, rather than a question how to use existing features

Describe the bug

The goal would be to be able to use Chimney in projects where strict rules are active.

We've successfully used Chimney for years; however, after migrating from 0.6.2 to 0.8.5, it began causing Wartremover errors, leading to the need for linter exclusions. This introduces extra noise and prevents some code aspects from being linter-verified.
Practical example:

  @SuppressWarnings(
    Array( // `.buildTransformer` from Chimney causes wartremover warning
      "org.wartremover.warts.Equals",
      "org.wartremover.warts.Null",
      "org.wartremover.warts.Var",
      "org.wartremover.warts.SeqApply"
    )
  )
  private[schedule] val contractScheduleRequestTransformer: PartialTransformer[ContractScheduleRequestDto, ContractScheduleRequest] =
    PartialTransformer
      .define[ContractScheduleRequestDto, ContractScheduleRequest]
      .withFieldComputed( ... ) 
      ...
      .buildTransformer

Reproduction

Creating a Scala-CLI snippet is challenging because it doesn't support SBT plugins. However, this applies to any use of Chimney (after migrating to a newer version) where the transformer will be sufficiently complex.
But if you want me to attach an SBT project here, please let me know.

Expected behavior

The goal would be to be able to use Chimney in projects where strict rules are active.
The previous version of Chimney did not cause such problems. There was no need to add linter ignores.

Actual behavior

The problem arises with slightly more complex, yet still relatively simple transformers. It occurred with partials: PartialTransformer.define, Transformer.definePartial, .intoPartial. There's a need of adding linter ignores.

Which Chimney version do you use

It's 0.8.5

Which platform do you use

• JVM
• Scala.js
• Scala Native

If you checked JVM

It's related to all JVM versions. We use currently Amazon Corretto 17.

Additional context

A similar issue was resolved in Jsoniter. There, a @SuppressWarnings(Array("org.wartremover.warts.All")) annotation was added for code generated by macros.

Please find that change here: plokhotnyuk/jsoniter-scala@fa8b53f

[MateuszKubuszok]

Hello, thank you for taking the time to pick all of that information together.

I have some idea how it can be fixed although I am not sure when I'll have the time to sit down on it.

It's slightly more complicated than what you see in Jsoniter since we are not using quasiquotes directly but through Scala 2/Scala 3 compatibility layer, and we have not 1 but multiple entrypoints to the macros.

So no a 5 minute task but 15 minutes + a lot of making sure that nothing was overlooked.

BTW, IMHO, if it's a big then it's more of a bug in Wartremover.

Since the intent of WR is to make programmer avoid writing bugs, it should ignore things that programmer did not wrote (code generated by codegens, macros), so I see it more like macro libraries having to provide workarounds for an arbitrary number of third-party linting tools (I am assuming that if I'd been suppressing Wartremover, I'd have to add Scapegoat, and if yet another tool arise, then I'll receive another bug ticket caused by something that didn't even existed when the particular version was released).

ATM I am thinking about using Java parameter (like MacWire does for debugging) so that users would provide a list of things that should be put into @SuppressWarnings(Array(...)). It doesn't sit right with me as it forces me to do someone else's job, but at least it would avoid any discussion what kind of suppression should go there (All, Var, nothing, something else).

[baldram]

It's true, one could see it that way; it's something Wartremover itself should solve, though I'm not sure if that's feasible from their side :/ The ticket you mentioned will probably remain open indefinitely. :/

multiple entrypoints to the macros

I expected there to be many entry points to the macro, but there's no harm, even if the annotation is used more times than necessary.

So no a 5 minute task but 15 minutes + a lot of making sure that nothing was overlooked.

That could be also made in steps at least, and I might report if something still is happening.

but at least it would avoid any discussion what kind of suppression should go there (All, Var, nothing, something else).

If we want to handle Wartremover, the matter is quite simple. One annotation takes care of everything. By using it, we are guaranteed not to miss anything.

@SuppressWarnings(Array("org.wartremover.warts.All"))

We did it selectively on our end, so as not to disable the linter entirely.

If we're dealing with other tools, perhaps we could create a separate ticket to replace this with a configurable solution in the future? What do you think?

[MateuszKubuszok]

I made #497 bit it would have to be tested, since Chimney tests do not cover external linters.

[baldram]

Thank you! Are there snapshot releases? I would test it then. Otherwise, publishLocal is also an option.

[MateuszKubuszok]

Unfortunately we have snapshots released automatically only on merge to master. But something like

cd /tmp
git clone [email protected]:scalalandio/chimney.git
cd chimney
git checkout linter-warning-suppression
sbt chimneyMacroCommons/publishLocal chimney/publishLocal # assuming 2.13

should do the trick

[baldram]

Amazing! All good!
(btw. I will mention that I played earlier with -Ywarn-macros scalac flag without success here).

Now, after using locally built snapshot (in my case it was 1.0.0-M5-1-gb4885f0-SNAPSHOT) everything is clean!
We just have to wait for the next minor release, which might appear even before 1.0.0.

Should I create a ticket for the future regarding a side thread from this discussion to create a configurable solution (inspired by Tapir) that could potentially work for Scapegoat, Scalafix, or other tools? From our perspective, we are happy at this moment, but looking more broadly, you're right about this systemic solution.

[MateuszKubuszok]

Thanks for testing it out!

If this works I think I could merge it and release as a part of 1.0.0-M6 (I need a couple more iterations of breaking changes before things would stabilize for good) - there is a few more relatively small changes that I want to merge before releasing the next milestone, so I guess it's a couple days?

I'll see how much that I would have on my hands (I am hoping to have RC1 before the end of the month and final 1.0.0 by the end of May) and to release 0.8.6 I'd have to release things manually, our current CI setup only releases from master.

I wouldn't mind having a ticket to trace some long term solution. I could leave the current values as they are as defaults, but if someone wanted to customize them/remove that annotation, they should be able to (e.g. in the very WartRemover issue there people arguing against suppressing all errors in the macros, and instead suppressing warts selectively).

[baldram]

to release 0.8.6 I'd have to release things manually, our current CI setup only releases from master.

Of course, it's not feasible. You have a lot of work on 1.0.0. Huge thanks for that and for the support for Scala 3.

Btw. I see you have some kind of magic release setup. I'm not sure if it's sbt-sonatype or where this devilry is ;-) and how you control it to make it M5 now. I need to peek into our internal libraries and future open-source ones I'll want to extract from our internals.

If this works I think I could merge it and release as a part of 1.0.0-M6

Good to know that we need to prepare for more breaking changes. We will be migrating gradually.

I'll see how much that I would have on my hands (I am hoping to have RC1 before the end of the month and final 1.0.0 by the end of May)

Thank you very much!

I wouldn't mind having a ticket to trace some long term solution

👍

[MateuszKubuszok]

Yes, the whole series of milestones is just that - a big batch of breaking changes on byte-code level which try to avoid breaking source compatibility (unless you used some internals by hand or cherry-picked your implicits).

OTOH:

• chimney-java-collections are relatively good - in separation. The moment you start thinking about converting cats.data.NonEmptyList into java.util.List the naive approach stops working. Even more when you start considering .everyItem/.everyMapKey/.everyMapValue (released yesterday) and how could they works with Cats/Java collections or whatever user would like to support
• 0.8.x introduced nested paths - that is withFieldConst(_.foo.bar.baz, ...) but to enable withFieldConst(_.foo.matching[Bar].baz) some (phantom) types had to be changed - again, not something that users should use in their own code but it might be inferred somewhere and eviction could also make things nasty
• then there are Cats instances - what Chimney have in 0.8.x is not always following the laws, and sorting that out requires changes in implicits (and in behavior: now sequential instances for partial.Result: Functor/Applicative/Monad/etc - have fail-fast/sequential semantics, while Parallel have parallel semantics, like it should have since the beginning)
• there were some issues with how to match setter field names, and how to handle ambiguities (more than one source field matched) - now ambiguity is a compilation error and there are several ways to resolve it
• the only visible change would be Deprecate withCoproductInstance name #500 - it's a deprecation, so without fatal warnings code would still compile and a fix would be a simple find+replace, yet I believe it is justified for making the code easier to understand ("coproduct" is an artifact of the initial Shapeless representation, I doubt that people other than library maintainer ever use that nomenclature)

Most of that should be pretty much source compatible - you update the version, recompile code and you are done. If you managed to migrate from pre-0.8.x to 0.8.x (TransformerF removal) you should be good to go.

Still, if you compiled the code against 0.8.x and had an eviction things could get messy with classloader, so it should be considered a breaking change. The idea is, if we have to make a breaking change to fix some issue (because of bytecode) we might as well shove there as much source compatible repairs as possible, so that we would do it once, and then stay happily on 1.0.0 for like several years.

[MateuszKubuszok]

Released in 1.0.0-M6.

[baldram]

Released in 1.0.0-M6.

All good! I bumped the dependency in one of the projects (using 0.8.5), all tests passed, no need to adjust anything.
Thank you!
We'll be on the lookout for RC1 soon and will set it globally in the BOM for all projects.

[MateuszKubuszok]

@baldram I just merged #572 which would allow customizing content of the annotation - it is a paint to test though, so while it seems to work, would you mind testing the snapshot if it hasn't broken anything?

[baldram]

Thank you. Imight try tomorrow. Is it ok?

[MateuszKubuszok]

I'm not in hurry with the next release 🙂

[baldram]

I'm not in hurry with the next release

If there's no rush, I gave myself an extra day. Been having some brutal days lately, packed with meetings from dawn till dusk. It's freakin' exhausting. 🤕

would you mind testing the snapshot if it hasn't broken anything?

Just tested out that artifact 1.3.0-9-g8057162-SNAPSHOT. Looks like it's new these artifacts are hitting the Maven repo now. Last time I think I had to mess with publishLocal to test.

Good news! Everything's working smooth as butter - no backsliding at all.
Switched over to this version and zero warnings. As for the functionality, all tests are passing no sweat.

[MateuszKubuszok]

Thanks!