PicnicSupermarket / error-prone-support

Error Prone extensions: extra bug checkers and a large battery of Refaster rules.
https://error-prone.picnic.tech
MIT License
201 stars 39 forks source link

Expand `StringRules` to cover `CharSequence` as well #1394

Open timtebeek opened 3 weeks ago

timtebeek commented 3 weeks ago

Problem

Right now StringRules is able to do replacements for Strings, but skips StringBuilder for instance, as identified here:

There's a note at the top of this recipe that goes into somewhat more detail on what's needed to cover any CharSequence. https://github.com/PicnicSupermarket/error-prone-support/blob/4b458e01bd4d6af3fad23fec9fb6e795fa78062a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java#L31-L46

Description of the proposed new feature

Implement whatever's needed to rewrite any CharSequence, especially since this project now targets Java 17+.

Considerations

If we're to support Java 8-15 still (please do 😇) then any recipe targeting [Java 15+ CharSequence.isEmpty()](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/CharSequence.html#isEmpty()) might need to be in addition to the recipe for String.isEmpty(), which is still available on older versions of Java, and matches as such.

Participation

Would this need anything more than just an additional CharSequenceRule ? Or what approach should be taken here?

Stephan202 commented 3 weeks ago

Thanks for filing an issue for this @timtebeek! Off the top of my head there are (at least) ~two ways forward:

  1. Solve the problem using multiple Maven modules. E.g. by having multiple refaster-rules-java-X modules, plus an aggregate refaster-rules module with a runtime dependency on the others. There may be several variations on this, and we'd have to assess whether the refaster-rules-java-X modules should target Java X. That would validate compliance by the @AfterTemplate methods, but would be too restrictive for the @BeforeTemplate methods, likely leading to duplication.
  2. Express the constraint using an annotation (analogous to JUnit 5's @EnabledForJreRange), and have the Refaster check respect said annotation. "For reasons" this may require some reflection that (I think) we should be able to lift from #261. I need to give some thought to how we'd validate that the proposed annotation is applied correctly.

While (2) comes with more unknowns, it feels more maintainable, and the annotation idea matches what I wrote here (that is: we may add custom annotation support for Refaster anyway). Something to sleep on.

(Managing expectations: also "for reasons", it may be quite a while before I find time to dig into this more deeply.)