From 224ee248b1cb706a8aaf58ea3fa173835fcea8b7 Mon Sep 17 00:00:00 2001
From: Henrique Nakashima <hnakashima@chromium.org>
Date: Fri, 21 Mar 2025 11:35:02 -0700
Subject: [PATCH] Reland "Add _CheckAndroidNullAwayAnnotatedClasses PRESUBMIT
check"
Doing a simpler check, not trying to figure out if the annotation is in the class declaration or on a method declaration. It would be
very rare for a method to be annotation and not the class.
This reverts commit ae8498e7516099f7e07b7477ecc6af65da6ade99.
Reason for revert: Fixed version
Original change's description:
> Revert "Add _CheckAndroidNullAwayAnnotatedClasses PRESUBMIT check"
>
> This reverts commit cfe861e365f1c00b7dae00cae8cdcf69cf4123e1.
>
> Reason for revert: Doesn't ignore comments
>
> Original change's description:
> > Add _CheckAndroidNullAwayAnnotatedClasses PRESUBMIT check
> >
> > Warn when a .java file is missing @NullMarked and @NullUnmarked.
> >
> > Temporarily disabled in //android_webview and //chrome while migrating.
> >
> > Bug: 404884589
> > Change-Id: I38e331bcec6f3a3f2147c0999fd4dca8fef145d5
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6373892
> > Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>
> > Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1435461}
>
> Bug: 404884589
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Change-Id: I669658f3f21cab9365f0c50b4e0191a1e6a608a0
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6377728
> Auto-Submit: Henrique Nakashima <hnakashima@chromium.org>
> Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1435593}
Bug: 404884589
Change-Id: I11d0ed28bf1941e72df5dcc457fb9d7e62dc89ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6381838
Auto-Submit: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1436182}
---
PRESUBMIT.py | 43 ++++++++++++++++++++++++++
PRESUBMIT_test.py | 78 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 121 insertions(+)
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index b6bbf9ecaf2e1..24a5f29893330 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -5957,6 +5957,7 @@ def ChecksAndroidSpecificOnUpload(input_api, output_api):
results.extend(_CheckNewImagesWarning(input_api, output_api))
results.extend(_CheckAndroidNoBannedImports(input_api, output_api))
results.extend(_CheckAndroidInfoBarDeprecation(input_api, output_api))
+ results.extend(_CheckAndroidNullAwayAnnotatedClasses(input_api, output_api))
return results
@@ -7578,6 +7579,48 @@ a subclass of it), or use "@Rule BaseRobolectricTestRule".
return results
+def _CheckAndroidNullAwayAnnotatedClasses(input_api, output_api):
+ """Checks that Java classes/interfaces/annotations are null-annotated."""
+
+ nullmarked_annotation = input_api.re.compile(r'^\s*@(NullMarked|NullUnmarked)')
+
+ missing_annotation_errors = []
+
+ def _FilterFile(affected_file):
+ return input_api.FilterSourceFile(
+ affected_file,
+ files_to_skip=(_EXCLUDED_PATHS + _TEST_CODE_EXCLUDED_PATHS + input_api.
+ DEFAULT_FILES_TO_SKIP + (
+ r'.*Test.*\.java',
+ r'^android_webview/.*', # Temporary, crbug.com/389129271
+ r'^build/.*',
+ r'^chrome/.*', # Temporary, crbug.com/389129271
+ r'^chromecast/.*',
+ r'^components/cronet/.*',
+ r'^tools/.*',
+ )),
+ files_to_check=[r'.*\.java$'])
+
+ for f in input_api.AffectedSourceFiles(_FilterFile):
+ for line in f.NewContents():
+ if nullmarked_annotation.search(line):
+ break
+ else:
+ missing_annotation_errors.append(str(f.LocalPath()))
+
+ results = []
+
+ if missing_annotation_errors:
+ results.append(
+ output_api.PresubmitPromptWarning(
+ """
+Please add @NullMarked and fix the NullAway warnings in the following files
+(see https://chromium.googlesource.com/chromium/src/+/main/styleguide/java/nullaway.md):
+""", missing_annotation_errors))
+
+ return results
+
+
def CheckNoJsInIos(input_api, output_api):
"""Checks to make sure that JavaScript files are not used on iOS."""
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
index 13552626245ed..1739c3e115deb 100755
--- a/PRESUBMIT_test.py
+++ b/PRESUBMIT_test.py
@@ -5138,6 +5138,84 @@ class CheckAndroidTestAnnotations(unittest.TestCase):
self.assertIn('OneTest.java', errors[0].items[0])
+class CheckAndroidNullAwayAnnotatedClasses(unittest.TestCase):
+ """Test the _CheckAndroidNullAwayAnnotatedClasses presubmit check."""
+
+ def testDetectsInClasses(self):
+ """Tests that missing @NullMarked or @NullUnmarked are correctly flagged in classes."""
+ mock_input = MockInputApi()
+ mock_input.files = [
+ MockFile('path/OneMissing.java', ['public class OneMissing']),
+ MockFile('path/TwoMarked.java', [
+ '@NullMarked',
+ 'public class TwoMarked {',
+ ]),
+ MockFile('path/ThreeMarked.java', [
+ '@NullUnmarked',
+ 'public class ThreeMarked {',
+ ]),
+ MockFile('path/FourMissing.java', ['class FourMissing']),
+ ]
+ results = PRESUBMIT._CheckAndroidNullAwayAnnotatedClasses(mock_input, MockOutputApi())
+ self.assertEqual(1, len(results))
+ self.assertEqual('warning', results[0].type)
+ self.assertEqual(2, len(results[0].items))
+ self.assertIn('OneMissing.java', results[0].items[0])
+ self.assertIn('FourMissing.java', results[0].items[1])
+
+ def testDetectsInAnnotations(self):
+ """Tests that missing @NullMarked or @NullUnmarked are correctly flagged in annotations."""
+ mock_input = MockInputApi()
+ mock_input.files = [
+ MockFile('path/OneMissing.java', ['@interface OneMissing']),
+ MockFile('path/TwoMarked.java', [
+ '@NullMarked',
+ '@interface TwoMarked {',
+ ]),
+ ]
+ results = PRESUBMIT._CheckAndroidNullAwayAnnotatedClasses(mock_input, MockOutputApi())
+ self.assertEqual(1, len(results))
+ self.assertEqual('warning', results[0].type)
+ self.assertEqual(1, len(results[0].items))
+ self.assertIn('OneMissing.java', results[0].items[0])
+
+ def testDetectsInInterfaces(self):
+ """Tests that missing @NullMarked or @NullUnmarked are correctly flagged in interfaces."""
+ mock_input = MockInputApi()
+ mock_input.files = [
+ MockFile('path/OneMissing.java', ['interface OneMissing']),
+ MockFile('path/TwoMarked.java', [
+ '@NullMarked',
+ 'interface TwoMarked {',
+ ]),
+ ]
+ results = PRESUBMIT._CheckAndroidNullAwayAnnotatedClasses(mock_input, MockOutputApi())
+ self.assertEqual(1, len(results))
+ self.assertEqual('warning', results[0].type)
+ self.assertEqual(1, len(results[0].items))
+ self.assertIn('OneMissing.java', results[0].items[0])
+
+ def testExcludesTests(self):
+ """Tests that missing @NullMarked or @NullUnmarked are not flagged in tests."""
+ mock_input = MockInputApi()
+ mock_input.files = [
+ MockFile('path/OneTest.java', ['public class OneTest']),
+ ]
+ results = PRESUBMIT._CheckAndroidNullAwayAnnotatedClasses(mock_input, MockOutputApi())
+ self.assertEqual(0, len(results))
+
+ def testExcludesTestSupport(self):
+ """Tests that missing @NullMarked or @NullUnmarked are not flagged in test support classes."""
+ mock_input = MockInputApi()
+ mock_input.files = [
+ MockFile('path/test/Two.java', [
+ 'public class Two'
+ ]),
+ ]
+ results = PRESUBMIT._CheckAndroidNullAwayAnnotatedClasses(mock_input, MockOutputApi())
+ self.assertEqual(0, len(results))
+
+
class AssertNoJsInIosTest(unittest.TestCase):
def testErrorJs(self):
input_api = MockInputApi()