Drop unused DiffNotApplicableException

[Stephan202]

This exception type was introduced along with Refaster in
01f7136, but no instance is ever
instantiated.

[Stephan202]

I noticed this while working on #4085. It could of course be that some Google-internal code does reference this type, in which case this PR may not make sense or require significant modification.

[Stephan202]

Which raises the question: was the following perhaps intended?

Suggested change

	} catch (IOException e) {
	} catch (IOException | RuntimeException e) {

(But if after all those years the current code sufficed... 🤷)

[cushon]

I noticed this while working on #4085. It could of course be that some Google-internal code does reference this type, in which case this PR may not make sense or require significant modification.

That is unfortunately the case, but I will take a closer look at whether the single place that's instantiating this exception type still makes sense

[cushon]

I noticed this while working on #4085. It could of course be that some Google-internal code does reference this type, in which case this PR may not make sense or require significant modification.

That is unfortunately the case, but I will take a closer look at whether the single place that's instantiating this exception type still makes sense

We have a another Diff implementation for uninteresting historical reasons, and it tries to handle invalid replacements (e.g. that offsets outside the file) and throw DiffNotApplicableException. The goal is to be able to apply a collection of fixes and skip any invalid ones, but still apply the valid ones.

I don't have strong opinions about this :) I think we could either

• add more uses DiffNotApplicableException, and try to use it consistently for the related error handling in https://github.com/google/error-prone/tree/master/check_api/src/main/java/com/google/errorprone/apply
• drop it, and catch RuntimeException

[Stephan202]

Interesting! I don't immediately have a strong opinion either. Some initial thoughts:

• Going for option (1) would likely be easier to do if the other diff logic is open-source/included, so that the requisite changes can be tested more easily.
• On the other hand, one could argue that in case of invalid replacements, the proper thing to do is to fix the check that emits these invalid replacements. 😅
  • This stance assumes that the suggested fixes aren't made invalid by other changes, but IIUC, as long as all suggested fixes are applied in one go, that shouldn't be an issue.^†
  • (It is true that occasionally emitting the right fix can be tricky, because the AST-source correspondence isn't perfect. But so far I've found that a solution always exists.)

So all-in-all I'm tending to "let's keep it simple" and go for option (2), but that does assume of course that the Google-internal diff is no longer relied upon.

^†: Distantly related here is #4088, but as far as I can tell the general case of chaining dependent bug checkers will always require an intermediate compilation phase, in which case there's no reason to assume that the replacements emitted by each round can't be accurate, relative to the source code produced by the previous round.

[cushon]

Thanks for thinking this through! I am leaning towards (2).

one could argue that in case of invalid replacements, the proper thing to do is to fix the check that emits these invalid replacements.

I think that is an extremely valid argument :) I think the motivation might have been for iterating on new checks, where it's useful to apply the fixes we can even if some of them are bogus. We'd also want to fix the bad findings, but it takes some time to run the check across the depot, and being able to apply a subset of the fixes in that case cuts down on the number of iterations. But (2) is still fine for that.

[cushon]

DiffNotApplicableException has been removed in 737dec0. Thanks for pointing this out!

[jeandersonbc]

but as far as I can tell the general case of chaining dependent bug checkers will always require an intermediate compilation phase

Thanks for the input @Stephan202!