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

Fix bug where nested MissingBraces violations' suggested fixes result in broken code #3797

Closed

Conversation

keithl-stripe
Copy link
Contributor

@keithl-stripe keithl-stripe commented Feb 26, 2023

@holtherndon-stripe came across this issue while applying MissingBraces to Stripe's code base of 4 million lines of Java code.

See unit test for sample code.

I'm not sure about a few things:

  • Why does the coalescing code suppress duplicate inserts? It seems a bit odd that this is the default. I wonder which checkers' fixes need this, and why.
  • Is Fix the right level of abstraction? Or should the coalescing policy be per Replacement?

@cpovirk
Copy link
Member

cpovirk commented Feb 27, 2023

I don't know enough to offer a definite way forward here, but I happen to know a couple relevant pieces of information:

Actual core Error Prone team members may have more useful advice.

@cushon
Copy link
Collaborator

cushon commented Sep 12, 2023

I had a quick look at whether we could 'just' preserve duplicate inserts by default. ReturnMissingNullable was one example I ran into, there were a couple of others that similarly emitted the same method-level fix once per occurrence of a problem int he method body, leading to things like duplicate @SuppressWarnings annotations, or duplicate exceptions in throws, etc.

  • I recall that we had a problem very similar to the one you're reporting but with MustBeClosedChecker. We added some code to manually insert n closing braces as a single postFix with call, and then we were able to rip it out for reasons that I'm not actually clear on. I don't know if that's evidence that we should support a proper fix (like you're proposing), evidence that we'd rather hack around it, or evidence that there's actually some superior way of doing it that I'm unaware of.

I think part of what happened with MustBeClosedChecker was that the DuplicateInsertPolicy stuff got added, and that was configured when using MustBeClosedChecker as a refactoring tool.

I suspect we're not done play whack-a-mole with edge cases involving overlapping and adjacent fixes, but I also think allowing MissingBraces to opt-in to this is an improvement over the status quo.

copybara-service bot pushed a commit that referenced this pull request Sep 19, 2023
…lt in broken code

@holtherndon-stripe came across this issue while applying `MissingBraces` to Stripe's code base of 4 million lines of Java code.

See unit test for sample code.

I'm not sure about a few things:
- Why does the coalescing code suppress duplicate inserts? It seems a bit odd that this is the default. I wonder which checkers' fixes need this, and why.
- Is `Fix` the right level of abstraction? Or should the coalescing policy be per `Replacement`?

Fixes #3797

FUTURE_COPYBARA_INTEGRATE_REVIEW=#3797 from keithl-stripe:keithl-missing-braces-patch de82c53
PiperOrigin-RevId: 564840815
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

Successfully merging this pull request may close these issues.

3 participants