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

Proposal: checker to prevent other checkers from calling javac methods that changed across JDKs #4026

Closed
msridhar opened this issue Jul 29, 2023 · 4 comments

Comments

@msridhar
Copy link
Contributor

Error Prone has run into a couple issues lately where building on JDK 17 caused incompatibility with JDK 11, due to changes in javac APIs:

#3895
#2330

NullAway is running into these exact same issues:

uber/NullAway#779
uber/NullAway#795

It might be nice to have a checker that flags when other checkers are calling the methods whose signatures have changed. Maybe just like the DoNotCallChecker but a separate implementation? This way checkers could share knowledge on what calls to avoid moving forward. (This kind of issue will get caught if tests are configured to run on older JDKs, which NullAway does now, but we still missed uber/NullAway#794 due to a lack of unit test coverage.)

@cushon
Copy link
Collaborator

cushon commented Jul 29, 2023

This SGTM.

I created ASTHelpersSuggestions as a partial solution to this, e.g. for the com.sun.tools.javac.util.Filter issue there's a wrapper API in ASTHelpers, and that check suggests using the wrapper API.

I'd be happy to review PRs to improve coverage in this area. It might make sense to add additional APIs to ASTHelpersSuggestions, and maybe rename the check.

This could also potentially be a separate check if not all of the issues have ASTHelpers workarounds, or if that would be more useful. I saw in uber/NullAway#779 that you used your own wrapper API because you weren't on Error Prone 2.20.0 yet. Would it be good enough to have a check that caught these issues and suggested ASTHelpers, since even if you could apply the fix as-is the finding would still be useful?

@msridhar
Copy link
Contributor Author

msridhar commented Jul 29, 2023

(Edited: previously this comment referred to a bad call that looked like it was getting missed but it was due to a @SuppressWarnings 🤦‍♂️ removed that part)

The ASTHelpersSuggestions check looks like exactly what I was looking for! Thanks for the pointer! In fact we were already running the check :-) Looks like we had some suppressions of that check; we'll be more careful in the future.

Would it be good enough to have a check that caught these issues and suggested ASTHelpers, since even if you could apply the fix as-is the finding would still be useful?

Yes, as long as it flags issues this is still very useful to us, even if we can't apply the auto-fix.

@msridhar
Copy link
Contributor Author

I opened #4027 to improve coverage of ASTHelpersSuggestions a bit. Beyond that, I think the check / error messages could be renamed to better convey the potential consequences of calling the javac APIs directly. I can put up a PR for that too, but didn't have a good idea on what to rename the check to exactly.

copybara-service bot pushed a commit that referenced this issue Jul 31, 2023
See #4026.  `Symbol.getEnclosedElements()` is another method that can cause compatibility issues across JDK versions (#3895).

Fixes #4027

FUTURE_COPYBARA_INTEGRATE_REVIEW=#4027 from msridhar:asthelperssuggestions-getenclosedelements b4c01ba
PiperOrigin-RevId: 552505468
copybara-service bot pushed a commit that referenced this issue Jul 31, 2023
See #4026.  `Symbol.getEnclosedElements()` is another method that can cause compatibility issues across JDK versions (#3895).

Fixes #4027

COPYBARA_INTEGRATE_REVIEW=#4027 from msridhar:asthelperssuggestions-getenclosedelements b4c01ba
PiperOrigin-RevId: 552510760
@cushon
Copy link
Collaborator

cushon commented Sep 7, 2023

I'm going to close this out now that #4029 has landed, thanks!

didn't have a good idea on what to rename the check to exactly

I don't have any brilliant ideas for this either, but proposals are welcome if you think of a name that would be clearer.

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

2 participants