Skip Refaster matches when @AfterTemplate returns a wider type

[knutwannheden]

Refaster currently does not verify that the @AfterTemplate's return type is compatible with the @BeforeTemplate's return type. This can produce replacements that break compilation.

Example

Given this Refaster rule:

public class WiderTypeTemplate {
  @BeforeTemplate
  String before(Object obj) {
    return String.valueOf(obj);
  }

  @AfterTemplate
  Object after(Object obj) {
    return obj;
  }
}

Refaster would transform this code:

String str = String.valueOf(obj);
int len = String.valueOf(obj).length();

into:

String str = obj;          // error: Object cannot be converted to String
int len = obj.length();    // error: cannot find symbol length() on Object

Fix

Before generating a fix, RefasterScanner now checks whether the @AfterTemplate's return type (erased) is a subtype of the @BeforeTemplate's return type (erased). If not, the match is skipped.

Caveats

While having an @AfterTemplate with a wider return type than the @BeforeTemplate may sound unusual, it can be useful in practice. For example, Error Prone Support's AssertThatThrownByAsInstanceOfThrowable rule replaces ThrowableAssertAlternative<T> with the wider AbstractThrowableAssert<?, T>:

@BeforeTemplate
ThrowableAssertAlternative<T> before(ThrowingCallable tc, Class<T> type) {
  return assertThatExceptionOfType(type).isThrownBy(tc);
}

@AfterTemplate
AbstractThrowableAssert<?, T> after(ThrowingCallable tc, Class<T> type) {
  return assertThatThrownBy(tc).asInstanceOf(throwable(type));
}

The current fix is conservative: it compares the before and after template return types directly, without considering whether the wider type would actually be compatible with the specific match context (e.g. compatible method overloads or assignment targets that accept the wider type). This means some valid replacements may be skipped. A more precise check that inspects the parent AST node for context-specific compatibility could be added in the future.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[Stephan202]

I think adding such functionality is great, but given the mentioned limitations, I would prefer that this were introduced behind a flag. Quite a lot of our rules would stop applying. There's also the question whether as-is this functionality perhaps belongs at the rule compilation phase, rather than the application phase (if I understand the logic correctly).

Something I've been thinking about (but didn't implement yet), is that rules could be annotated as (or inferred to be) "potentially breaking", such that users can choose to run all or only a safe subset of rules.

A more precise check that inspects the parent AST node for context-specific compatibility could be added in the future.

This would be awesome, though likely non-trivial. I can't see myself have time to investigate this in the near to medium term, but am interested to hear whether this was ever looked into by a Google maintainer: do you think it's achievable, and if so, desirable?

[cushon]

I think adding such functionality is great, but given the mentioned limitations, I would prefer that this were introduced behind a flag. Quite a lot of our rules would stop applying.

That makes sense to me as well, I'd need to check how many of Google's existing templates would need updates for this, but I'd guess at least some of them.

A more precise check that inspects the parent AST node for context-specific compatibility could be added in the future.

This would be awesome, though likely non-trivial. I can't see myself have time to investigate this in the near to medium term, but am interested to hear whether this was ever looked into by a Google maintainer: do you think it's achievable, and if so, desirable?

This seems like an interesting idea. I'm not aware of this being investigated (it's possible it was at some point, I'm not certain).