Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refaster support for varargs methods #568

Closed
Stephan202 opened this issue Mar 11, 2017 · 5 comments
Closed

Refaster support for varargs methods #568

Stephan202 opened this issue Mar 11, 2017 · 5 comments

Comments

@Stephan202
Copy link
Contributor

I tried to write a Refaster rule for callers of some internal library method M which has a signature that matches String#format(String, Object...). The idea is to identify callers which explicitly call String.format before invoking M and to rewrite those expressions to defer the formatting to M. But nothing happened.

This appears to be because of the use of varargs. As a test I tried to apply the following dummy Refaster template to our code base, but that didn't do anything either:

final class VarargsRefasterTemplate {
    @BeforeTemplate
    String before(final String format, final Object... args) {
        return String.format(format, args);
    }

    @AfterTemplate
    String after(final String format, final Object... args) {
        return "This is a test";
    }
}

Would it be possible to support varargs?

@lowasser
Copy link
Contributor

It is supported, just not in a super well documented way, I think?

Write @BeforeTemplate String before(String format, @Repeated Object arg).

@Repeated is slightly more general, though; it can be interspersed with normal arguments, put in an array declaration, and generally stand in for any comma repeated expression.

@Stephan202
Copy link
Contributor Author

Awesome, thanks! This works like a charm. I agree it would be good to mention this in the Refaster documentation, but perhaps the RefasterRuleCompiler could also emit an error/warning when a user tries to do what I did above; that would make the feature even more discoverable.

NB: I would have expected the following rule to also match three-or-more element expressions, but it doesn't. (And with just e1 it only matches singletons):

final class FixedImmutableEnumSet<E extends Enum<E>> {
    @BeforeTemplate
    ImmutableSet<E> before(final E e1, @Repeated final E e2) {
        return ImmutableSet.of(e1, e2);
    }

    @AfterTemplate
    @SuppressWarnings("unchecked")
    @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)
    ImmutableSet<E> after(final E e1, @Repeated final E e2) {
        return Sets.immutableEnumSet(e1, e2);
    }
}

Am I "holding it wrong"?

@lowasser
Copy link
Contributor

It's because ImmutableSet.of does the weird thing of not using varargs until n gets really big, but having different overloads outright until n=11 IIRC. If it were really a varargs method it'd work :(

@eaftan
Copy link
Contributor

eaftan commented Mar 13, 2017

@lowasser Are there more internal Refaster docs we should externalize? Can we unify the internal and external docs? This would be a good doc day activity.

bulldozer-bot bot pushed a commit to palantir/safe-logging that referenced this issue Jul 17, 2019
Fix refaster rule for hasArgs -> containsArgs migration.

A different syntax needed to be used when dealing with varargs, see related issue:
google/error-prone#568
@knutwannheden
Copy link

When writing a Refaster template for a method with a varargs parameter, I currently always need to specify at least two templates: one for call sites using varargs and one for call sites using explicit arrays. Could Refaster simplify this use case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants