Exclude macro generated code from WartRemover checks about chimney HOT 14 CLOSED

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

from chimney.

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

👍

from chimney.

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 #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.

from chimney.

Released in 1.0.0-M6.

from chimney.

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.

from chimney.

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).

from chimney.

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?

from chimney.

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

from chimney.

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

from chimney.

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.

from chimney.

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).

from chimney.