Introduce NewStringFromCharArray{,SubSequence} Refaster rules

[BLasan]

fixes: #1001

[BLasan]

@rickie @Stephan202 Please review. I have added the rule to copy string characters with an offset and a count. I tried the below way to support both copying all characters and copying characters with an offset[1]. But it gives the following error[2]

[1]

@AfterTemplate
    String after(char[] data, int offset, int count) {
      return Refaster.anyOf(new String(data, offset, count), new String(data));
    }

[2] Caused by: java.lang.IllegalArgumentException: @AfterTemplate of tech.picnic.errorprone.refasterrules.StringRules.StringCopyValueOf defines arguments that are not present in all @BeforeTemplate methods: [offset, count]

Is there a specific way to overcome this? Or do we need another class to be initiated?

Thanks,
Benura

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[Stephan202]

Hey @BLasan! Thanks; I'll have a look this weekend.

To answer your question: Refaster has some constraints. Relevant to your example are:

1. Refaster.anyOf can only be used in @BeforeTemplate methods.
2. Any parameter to an @AfterTemplate method must also be specified (and bound) by all @BeforeTemplate methods.

The new String(data) case would require a separate rule that rewrites (off the top of my head) String.copyValueOf(data, 0, data.length). I think that makes sense to do!

[Stephan202]

I added a commit with some suggestions, including an extra rule.

Suggested commit message:

Introduce `NewStringFromCharArray{,SubSequence}` Refaster rules (#1012)

Resolves #1001.

@rickie will also review the PR, but as he's off, that may take a week or so.

Thanks for your contribution @BLasan!

[Stephan202]

This sentence contains a copy-paste error 👀

[Stephan202]

I see that we can also rewrite a String.valueOf variant; will propose something.

[Stephan202]

We generally order the test methods the same as the rules; I'll move these test methods up :)

[Stephan202]

We generally name the rule after the @AfterTemplate code. I this case it's a bit tricky, but I'll propose something.

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[BLasan]

Hi @Stephan202 ,

Thank you for the review. Are there any resources to learn the procedure of adding new rules? Also, any other issues that I could work on :) ?

Thanks,
Benura

[Stephan202]

Are there any resources to learn the procedure of adding new rules?

That's a good question. I think that right now there's no documentation for this. What we instead try to do, is add Error Prone checks that validate and (when possible) attempt to auto-fix rules to match our guidelines. PR #1002 moves some of those rules around, but there are (vague) plans to add more such rules.

For now the best I can offer is to employ careful pattern matching by looking at existing rules 😄

Also, any other issues that I could work on :) ?

Certainly our work isn't done 😉. The list of currently open issues could serve as inspiration, but note that most/all of those will require writing an Error Prone BugChecker, rather than a Refaster rule. That's quite a bit more involved, but if you're feeling adventurous: go for it 😄.

But let me offer another idea: are there aspects of your company's code base that you'd like to see different, such as things that you (perhaps repeatedly) point out during code review? If any of those are generally applicable and solvable using Refaster, we're certainly open to considering rules for that. (This suggestion does come with a caveat: we're kind of opinionated on some topics, such as preferring immutablility. But when in doubt: just ask!)

[BLasan]

Hi @Stephan202 ,

Sure, will take a look at some other issues opened. I asked specifically for an issue since someone internally (from your team) might have started working on such issues :) I need to recall some of the ideas (which are solvable using Refaster) pointed out in a code review as well as during the implementations. I'll just revisit some of the feature implementations and open issues if found any :)

Thanks,
Benura

[Venorcis]

LGTM 👍🏻

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[rickie]

Thanks for the Refaster rules @BLasan! 🚀

Also, any other issues that I could work on :) ?

@Stephan202 and I discussed something that could be a nice thing to work on. In this PR #770 that we are about to merge, we want to make clear for type migrations which methods are not (yet) migrated. We of course want to minimize the number of methods that are not migrated, see an example of the migration from JUnit Assertions to AssertJ here.

If working on Refaster rules increase the coverage of an JUnit Assertions -> AssertJ assertions migration sound interesting to you, you can take a few of those list. Add a few of the Refaster rules, create a PR and we can review them. If we do a few rules per PR the PR will not become too big. We can start with a few simple ones to go through this process the first time, we make sure to review it in an elaborate way to help you better understand Refaster 😄. That will make it easier for you to add new things and on our side easier to reviewing following rules.

For example the following methods could be a nice start to migrate from org.junit.jupiter.api.Assertions to their AssertJ equivalent:

"assertFalse(BooleanSupplier)",
"assertFalse(BooleanSupplier, String)",
"assertFalse(BooleanSupplier, Supplier<String>)",
"assertTrue(BooleanSupplier)",
"assertTrue(BooleanSupplier, String)",
"assertTrue(BooleanSupplier, Supplier<String>)",

Something that might not be clear is that in the JUnitToAssertJRules.java is that the Refaster rules are sorted by the order in which the methods we migrate away from are in the org.junit.jupiter.api.Assertions class.

But, don't worry we are of course here to help and guide you if you think this sounds like a nice thing! Don't hesitate to open a discussion, draft PR or an answer on this PR 😄.

Let us know what you think :).

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.