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

Add SystemOut checker #1534

Closed
wants to merge 1 commit into from
Closed

Add SystemOut checker #1534

wants to merge 1 commit into from

Conversation

tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Mar 5, 2020

This checker checks for uses of System.out and System.err as
possible logs used for debugging purpose.

Inspired by Policeman's Forbidden API Checker's jdk-system-out checks.

See also #1211

@cushon cushon self-assigned this May 16, 2020
@cushon
Copy link
Collaborator

cushon commented May 16, 2020

Is the idea for this to be disabled by default, but make it available so projects where using System.{err,out} is never desirable (servers, libraries, etc.) can opt in to it? That seems like a good approach. I've seen related bugs in code that's deliberately using System.out (e.g. CLIs) where print statements from debugging accidentally get committed to the production version. But that seems like a harder problem to find good heuristics for.

@tbroyer
Copy link
Contributor Author

tbroyer commented May 16, 2020

I would personally always enable it, and @SuppressWarnings("SystemOut") where it makes sense.
One heuristic that would be useful (to me) would be similar to SystemExitOutsideMain: allowing System.{out,err} in classes where a main() method is present. That wouldn't work for most CLIs though.

The reason I put it in the disabled-by-default list was mostly that SystemExitOutsideMain is there too.

@JakeWharton
Copy link
Contributor

I DI System.out and System.err from main but otherwise pass around Print* classes or use a proper logging framework.

@cushon
Copy link
Collaborator

cushon commented May 16, 2020

Thanks for the reminder about SystemExitOutsideMain, I'm not sure if something similar had been discussed for System.out or if I was just half-remembering that check. The heuristic for System.out seems trickier.

I think that requiring @SuppressWarnings("SystemOut") or DI'ing System.{out,err} are both sensible policies for a codebase to adopt, but might be too opinionated to have in the default EP configuration. We've generally been trying to make the defaults reflect things that are probably bugs, not just things that are required e.g. by Google best practices. (We don't always achieve that, but help staying honest is appreciated.)

I think this makes sense to make available as an opt-in / disabled-by-default check.

This checker checks for uses of System.out and System.err as
possible logs used for debugging purpose.
@tbroyer
Copy link
Contributor Author

tbroyer commented Jan 20, 2021

I rebased the branch, in case it can unblock copybara.

@tbroyer tbroyer deleted the system-out branch March 24, 2021 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants