Android Lint warnings/errors

[Peter Wen]

Do we need to treat all lint warnings as errors? I'd like to start a discussion about this.

I'm working on https://crbug.com/793007 and it's very painful to upgrade even from 25.3.2 to 26.0.0. The vast majority of the issues are suppressed one way or another and very few are actually useful.

Some warnings are low false-positive enough and actually useful like UnusedResources, but most others are not, in fact, we completely ignore a vast number of them in suppressions.xml.

What does everyone think of only surfacing errors in android lint and explicitly elevating a warning as an error once it is found to be useful and has a very low false positive rate?

There's also the issue of errorprone's verbose warning output. We should also have a discussion on turning those off too.

Opinions/comments welcome.

Peter

[Andrew Grieve]

My opinion - we should turn off all warnings that have non-trivial false-positives, or that don't provide obvious value. I don't think there is any value in having build warnings that do not fail the build.

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To post to this group, send email to ja...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAKNWQC5p8HRA0ywSp4%2B8%2Bw30zEp-CDhj8GP%2BZhfgyS1%2BXLduog%40mail.gmail.com.

[Tommy Nyquist]

I agree with agrieve@. If the warning provides no value, we should disable it.

It's also problematic with warnings that have high false-positive rates, so we should consider disabling those too, unless they have some extraordinary property of being amazingly useful in some particular cases. I'd err on the side of disabling probably though.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CABiQX1WX1bip%2BVZLSLQmueaqMkYgb5iXSA2pW4RnJh%2BTNg1kRA%40mail.gmail.com.

[Peter Wen]

Great! I think that for starters, all the existing errorprone warnings should be disabled. They make the build extremely verbose while not adding anything particularly actionable.

If someone wants to actually work on fixing them then they can easily re-enable them.

Thoughts?

All lint warnings are treated as errors right now, so we can slowly get rid of the ones with high false positive rates while not preventing meaningful regressions.

[Ted Choc]

+1 to agrieve and nyquist@

For example, I've spent time looking into this warning:

warning: [DefaultCharset] Implicit use of the platform default charset, which can result in e.g. non-ASCII characters being silently replaced with '?' in many environments 

Only then to look at Android's documentation and see that ("Android note: The Android platform default is always UTF-8."):

https://developer.android.com/reference/java/nio/charset/Charset.html#defaultCharset()

Since the platform always uses a sane default encoding, do we really care?  Seems like I can go through and replace all instances of getBytes() with getBytes("UTF-8") and be pretty confident that it won't break.  With that said, is it worth it?  Is it worth the code churn to silence a warning that likely doesn't apply on Android?  Does it make us feel better to be more specific?

- Ted

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAA6XDjOa8FM2ijWcDmEaC5gy%2Bx%2Bnf3MJ8iWUZ1BAV7b9kSTo7Q%40mail.gmail.com.

[Tommy Nyquist]

On Tue, Jan 9, 2018 at 10:33 AM Ted Choc <ted...@chromium.org> wrote:

+1 to agrieve and nyquist@

For example, I've spent time looking into this warning:

warning: [DefaultCharset] Implicit use of the platform default charset, which can result in e.g. non-ASCII characters being silently replaced with '?' in many environments 

Only then to look at Android's documentation and see that ("Android note: The Android platform default is always UTF-8."):

https://developer.android.com/reference/java/nio/charset/Charset.html#defaultCharset()

Since the platform always uses a sane default encoding, do we really care?  Seems like I can go through and replace all instances of getBytes() with getBytes("UTF-8") and be pretty confident that it won't break.  With that said, is it worth it?  Is it worth the code churn to silence a warning that likely doesn't apply on Android?  Does it make us feel better to be more specific?

Just for this particular one I think I'm initially in the camp of: Yes, it makes me feel better. It low-key freaks me out to see .getBytes() without a charset specified, and I'm guessing other people coming from a Java world might too. I've heard from reputable sources that freaking out is not great, so maybe we should just go through the code base to fix those particular warnings?

That being said; it sure looks like a lot of technically unnecessary churn.

So: Either fix all of the .getBytes() call sites very soon, or turn off the warning.

[Peter Wen]

Sounds like we all agree. I'll start adding in some initial code to suppress and track errorprone warnings so our build output isn't so verbose anymore.

[Tommy Nyquist]

Could we file bugs for error prone output that we want to follow up on later, once our build output has reached 0 lines again and we have systems in place to keep it that way?

[Peter Wen]

I'm currently filing bugs as I go. Would you prefer I file them later? Perhaps I'm misunderstanding.

I'm looking to target 0 lines at least for errorprone output. This CL (https://crrev.com/c/862323) already reduces multiple tens of screens of output for chrome_java to *only* ~10 screens.

Peter

[Tommy Nyquist]

No, sorry for being unclear. What you've been doing is great!

[wn...@google.com]

This should help greatly in reducing our build output: https://crrev.com/c/887001

With this I think building a single apk should now have around a single screen's worth of build output!

Peter