[various] Enable warnings as errors, and all warnings, for Android lint

[stuartmorgan-g]

For all Android plugins:

• Enable all warnings for lint
• Treat all warnings as errors for lint

This significantly increases the scope of issues that we'll catch in CI. To allow enabling this without having to make tons of fixes first, so that we get the incremental benefit immediately, this adds new baselines for all plugins. We can incrementally clean those baselines up over time. (In practice we haven't prioritized that, but it would be good to start paying down that technical debt incrementally at some point.)

See flutter/flutter#88011

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[stuartmorgan-g]

Open question for the review: are there any warnings we should add to the ignore list as part of this? We can decide incrementally later, but if there's anything we want to do in every package (like we've done with things like InvalidPackage) this would be be a good moment.

I checked the baseline files to see what errors are in them and how often, and got:

 942         id="UnknownNullness"
 265         id="SyntheticAccessor"
  24         id="StringFormatTrivial"
  11         id="UnusedResources"
  11         id="KotlinPropertyAccess"
   7         id="LogConditional"
   7         id="CommitPrefEdits"
   5         id="SelectableText"
   4         id="UnusedIds"
   4         id="LambdaLast"
   3         id="ViewConstructor"
   3         id="DefaultLocale"
   2         id="SwitchIntDef"
   2         id="NewApi"
   1         id="VisibleForTests"
   1         id="UseCompoundDrawables"
   1         id="OldTargetApi"
   1         id="ObsoleteSdkInt"
   1         id="InlinedApi"
   1         id="GradleOverrides"
   1         id="ContentDescription"

Any thoughts on the common ones in particular?

• I expect we probably do want UnknownNullness since it makes it easier to reason about code, and will help with future Kotlin adoption, but I'm not sure if there are downsides to requiring it.
• I haven't looked at why SyntheticAccessor is so common. It may or may not be useful, but the docs suggest it's probably good for libraries that care about code size, so seems relevant.

[stuartmorgan-g]

Overriding changelog and version since my understanding is that lintOptions will only apply to lint, and are thus dev-only.

[Hixie]

test-exempt: is testing

[stuartmorgan-g]

In case anyone else is curious about the per-plugin breakdown:

camera_android
 209         UnknownNullness
  51         SyntheticAccessor
  11         KotlinPropertyAccess
   3         LogConditional
   1         SwitchIntDef
   1         ObsoleteSdkInt
   1         InlinedApi
camera_android_camerax
 104         UnknownNullness
  25         SyntheticAccessor
   1         SwitchIntDef
espresso
 151         UnknownNullness
  34         SyntheticAccessor
  24         StringFormatTrivial
   4         LogConditional
   3         LambdaLast
   3         DefaultLocale
   1         VisibleForTests
flutter_plugin_android_lifecycle
   1         UnknownNullness
google_maps_flutter_android
  10         SyntheticAccessor
   8         UnknownNullness
google_sign_in_android
  55         UnknownNullness
  11         SyntheticAccessor
image_picker_android
  21         UnknownNullness
  15         SyntheticAccessor
in_app_purchase_android
   5         UnknownNullness
   2         SyntheticAccessor
local_auth_android
  15         SyntheticAccessor
  11         UnusedResources
  10         UnknownNullness
   5         SelectableText
   4         UnusedIds
   2         NewApi
   1         UseCompoundDrawables
   1         OldTargetApi
   1         GradleOverrides
   1         ContentDescription
path_provider_android
   6         SyntheticAccessor
   4         UnknownNullness
quick_actions_android
   6         UnknownNullness
   1         SyntheticAccessor
shared_preferences_android
   7         CommitPrefEdits
   3         UnknownNullness
   1         SyntheticAccessor
url_launcher_android
   9         UnknownNullness
   4         SyntheticAccessor
video_player_android
  34         UnknownNullness
  25         SyntheticAccessor
webview_flutter_android
 322         UnknownNullness
  65         SyntheticAccessor
   3         ViewConstructor
   1         LambdaLast

(It looks like about a third of the webview issues are Pigeon-generated, so we'll need a few Pigeon generation fixes [Edit to add: I just landed a Pigeon change that should make it lint-clean for Java, so as we update Pigeon in packages we'll fix a bunch of issues.])

[bparrishMines]

LambdaLast might be skippable. The warning is from overriding onShowFileChooser. Which is a method that we don't control. Or maybe I should just use a @SuppressWarnings for this scenario.

[bparrishMines]

LGTM

[stuartmorgan-g]

Or maybe I should just use a @SuppressWarnings for this scenario.

That seems like the better option; having it flag any new APIs we create seems worthwhile.

[stuartmorgan-g]

I'll update this PR tomorrow to change the repo tooling to check that these two flags are set for all packages so we don't accidentally forget in the future (e.g., when adding a new Android plugin or implementation).

[stuartmorgan-g]

I'm going to land this without waiting for the iOS flaky hang to resolve, since this PR can't affect iOS, and it's very prone to bitrot.

[reidbaker]

Consider writing this match to ignore lines that are commented out.

[stuartmorgan-g]

I can make that change if you want; there are potential downsides though. The current approach is to enforce that we haven't just forgotten to set this up when adding a new package. Allowing commenting it out allows for someone to explicitly say "I know that this is supposed to be set up, but I'm turning it off for now for a specific reason". I actually did exactly that with the javac part of this file previously, where for packages that weren't ready yet I added the line it was looking for commented out, with a TODO referencing an issue.

That may not be a useful thing to support for this though, where we can use the baseline file as an escape hatch.

[reidbaker]

If we want to support disabling a check that makes sense. I guess I think of commented out lines passing a check that is expected as a foot gun. I would expect something like # ignore checkAllWarnings to be how it was disabled.

I don't think it is important enough to go back and rework unless you happen to be working on the code already.