RefasterRuleCompiler ignores all but one input file

[Stephan202]

First of all: Refaster is awesome. Defining new rules is child's play. Super cool!

While playing around with it I defined a number of rewrite rules. If possible I'd like pass all of these to Error Prone in one go. But, while cursory look at the RefasterRuleCompiler indicates that it outputs a list of CodeTransformers to the file specified using the --out flag, only one of the input .java files actually contributes to the output. Would it be possible to combine multiple before/after templates into a single .refaster file?

(I'm not familiar with javac internals, but if you give me some pointers I'm willing to implement this generalization. Provided that you think this is a sound idea in the first place, of course.)

[nick-someone]

Thanks for the bug file!

It looks like the RefasterRuleAnalyzer appears to output after each compilation unit: https://github.com/google/error-prone/blob/master/refaster/src/main/java/com/google/errorprone/refaster/RefasterRuleCompilerAnalyzer.java#L61

I think we can restructure this to collect over all of the classes then emit at the end.

In the meantime: does this pattern work?

class MyWrapper {
   static class Rule1 { @BeforeTemplate ... }
   static class Rule2 { @BeforeTemplate ... }
}

and compile MyWrapper.java?

[lowasser]

FWIW: internally the refaster rule compiler only accepts one file at a time, but we also have a separate tool that assembles a "suite" (which can include mixed Refaster and Error Prone refactorings). We might be able to externalize that bit?

[Stephan202]

@nglorioso: GMTA, I did try the wrapper trick ;). Unfortunately that yields an empty list of code transformers. (I.e., same as if one would compile an empty class.) I do think there are good use cases for supporting that feature, though: if one has several closely-related rewrite rules (e.g. rewriting toList() and toSet() to toImmutableList() and toImmuntableSet(), respectively), then it makes sense to group those together in the same class.

@lowasser: that sounds exactly like what I'm look for :). What I'd like to do is create a Maven project which documents, in Error-Prone compatible form, a number of company-internal best-practices. The idea would be to use Refaster where possible, and fall back to Error Prone plugins where necessary. Ideally Error Prone would treat them identically (discovery through service loading, automatic patching, etc).

[Stephan202]

For the time being I tweaked our fork to collect Refaster rules from all provided files. The patch is small; see 0ff8c5c (branch bugfix/issue-552, in case I decide to rebase).

Given that you have an alternative approach internally I assume you're not interested in this fix, but if you are, let me know and I'll open a PR.

[jbduncan]

FWIW, I also tried the wrapper trick myself (the trick of combining multiple Refaster classes as sub-classes of a master Refaster class, as demonstrated in #552 (comment)), but last time I tried it, it caused Refaster to crash. I'd be happy to provide a reproducible example if it would help. :)

[PhilippWendler]

I tried the wrapper trick of putting all refaster rules into a single translation unit, and it worked. However, of course this is not really doable in practice.

Did I understand correctly that this is currently the only way of applying more than one refaster rule in a single error-prone run? If yes, any feature that would allow this would be highly appreciated. It could be

• support in the rule compiler for more than one translation unit,
• or a separate utility for merging rule files, or
• support in error-prone for more than one rule files.

Any of them would work for us, but without it refaster seems not really usable in practice. And we really would love to use it, it is such a great tool!

[Stephan202]

@PhilippWendler yes, afaik the wrapper trick is currently the only way that works out of the box.

With respect to the alternatives you suggest:

• The first option is what I did in the comment referenced further up in this thread. That specific change is incompatible with the latest version of Error Prone, but we track a fork of Error Prone and in each release we incorporate an up-to-date variant of that tweak. For the version 2.3.3-compatible variant, see a3033e6. These forked versions are also available on Jitpack. All Java builds within Picnic currently use this fork, so I'd say it's safe to use in your own code :) (But the different Maven groupId and the fact that the artifacts aren't in Maven Central is a bit annoying, of course.)

• I have some (not-yet-open sourced) code that tackles the issue more generally using a combination of your second and third suggestion. The solution consists of two pieces:

  1. A modified version of the RefasterRuleCompiler which does not accept a an --out parameter, but instead creates a /package/path/ClassName.refaster resource file for each compiled /package/path/ClassName.java file with an @AfterTemplate method.
  2. An Error Prone plugin which loads all .refaster resources from the classpath and applies them in one batch.

I'm perpetually "6 to 8 weeks" away from open-sourcing this code, but if you're very interested I might make an effort to properly pull this across the finish line. (Or perhaps just dump the relevant files in a Gist, but I'd rather do it "properly", with a blog post etc. This approach unlocks some nice other features that are worth expanding on.)

[PhilippWendler]

Thanks a lot @Stephan202! I hope a solution can be found that does not need the use of forks.

Given that the implementation of your first approach seems so trivial:
Googlers (@nglorioso, @lowasser), is there hope that this gets integrated soon in error-prone?
It absolutely makes sense to do this, and would make at least refaster usable in many cases until a more general solution like @Stephan202's second approach can be done.