Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy #31291

zanderso · 2022-02-06T22:36:07Z

The CREATE_NATIVE_ENTRY() pattern used in unit tests results in a lot of clang-analyzer-core.StackAddressEscape warnings due to the closures it gets passed retaining references to local variables. One way to resolve the warnings would be to declare all of those locals as static, however since the tests that use CREATE_NATIVE_ENTRY() are constructed to avoid the possibility of the callback getting called after the lifetime of the locals, that doesn't really seem like an improvement. Instead, this PR turns off checking for clang-analyzer-core.StackAddressEscape in the files where CREATE_NATIVE_ENTRY() and similar patterns are used.

This PR also ignores some warnings flagged due to rapidjson. We should find another json parsing solution, as suggested in flutter/flutter#97680.

Our Objective-C code has a lot of inconsistencies around nullability. @jmagman or @gaaclarke do you know if we already have an issue filed around that?

Related flutter/flutter#93576

bdero

LGTM. The blinkenlights on #31098 will be extreme. 😅

jmagman

With this config file, no issues found with ./ci/lint.sh --lint-all --variant ios_debug. However with --variant host_debug I see

/Users/m/Projects/engine/src/flutter/fml/synchronization/waitable_event_unittest.cc:34:57: error: Function 'rand' is obsolete because it implements a poor random number generator.  Use 'arc4random' instead [clang-analyzer-security.insecureAPI.rand,-warnings-as-errors]
      TimeDelta::FromMilliseconds(static_cast<unsigned>(rand()) % 20u);
                                                        ^~~~
/Users/m/Projects/engine/src/flutter/fml/synchronization/waitable_event_unittest.cc:34:57: note: Function 'rand' is obsolete because it implements a poor random number generator.  Use 'arc4random' instead
      TimeDelta::FromMilliseconds(static_cast<unsigned>(rand()) % 20u);
                                                        ^~~~
/Users/m/Projects/engine/src/flutter/fml/synchronization/waitable_event_unittest.cc:76:13: error: Function 'rand' is obsolete because it implements a poor random number generator.  Use 'arc4random' instead [clang-analyzer-security.insecureAPI.rand,-warnings-as-errors]
        if (rand() % 2 == 0) {
            ^~~~
/Users/m/Projects/engine/src/flutter/fml/synchronization/waitable_event_unittest.cc:76:13: note: Function 'rand' is obsolete because it implements a poor random number generator.  Use 'arc4random' instead
        if (rand() % 2 == 0) {
            ^~~~
/Users/m/Projects/engine/src/flutter/fml/synchronization/waitable_event_unittest.cc:161:15: error: Function 'rand' is obsolete because it implements a poor random number generator.  Use 'arc4random' instead [clang-analyzer-security.insecureAPI.rand,-warnings-as-errors]
          if (rand() % 2 == 0) {
              ^~~~
/Users/m/Projects/engine/src/flutter/fml/synchronization/waitable_event_unittest.cc:161:15: note: Function 'rand' is obsolete because it implements a poor random number generator.  Use 'arc4random' instead
          if (rand() % 2 == 0) {

Not sure why the impeller headers aren't found:

/Users/m/Projects/engine/src/out/host_debug/../../flutter/impeller/entity/content_context.h:12:10: error: 'flutter/impeller/entity/gradient_fill.frag.h' file not found [clang-diagnostic-error]
#include "flutter/impeller/entity/gradient_fill.frag.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Since that linter runs during the pre-push githooks I think those errors need to be fixed before this can merge, or pushes from Mac development machines will be blocked for commits that make changes to those files.

engine/tools/githooks/lib/src/pre_push_command.dart

Lines 40 to 46 in 6f99a71

    
           final io.File compileCommands = io.File(path.join( 
        
             flutterRoot, 
        
             '..', 
        
             'out', 
        
             'host_debug', 
        
             'compile_commands.json', 
        
           ));

jmagman · 2022-02-08T20:38:57Z

Here's the full output:
lint-output.txt

And here are the error (not warning) counts:

  30 clang-analyzer-core.StackAddressEscape
   1 clang-analyzer-cplusplus.NewDeleteLeaks
   1 clang-analyzer-optin.osx.cocoa.localizability.NonLocalizedStringChecker
   3 clang-analyzer-security.insecureAPI.rand
   1 clang-analyzer-security.insecureAPI.strcpy

jmagman · 2022-02-08T21:50:36Z

#31333 fixes macOS host_debug except the 30 clang-analyzer-core.StackAddressEscape errors.

jmagman · 2022-02-08T21:54:00Z

Our Objective-C code has a lot of inconsistencies around nullability.

I didn't see any nullability errors running the linter on top of this PR, which ones were you seeing?

zanderso · 2022-02-08T22:00:43Z

Our Objective-C code has a lot of inconsistencies around nullability.

I didn't see any nullability errors running the linter on top of this PR, which ones were you seeing?

The PR silences them in the .clang-tidy file.

zanderso · 2022-02-09T16:15:46Z

Thanks, Jenn! I think I've addressed the host_debug issues on macOS.

jmagman

The PR silences them in the .clang-tidy file.

Oh right, missed the -.

I think I've addressed the host_debug issues on macOS.

Tested locally, LGTM, thanks @zanderso!

…lutter/engine#31291)

* 2a8f388 Roll Skia from a424b619bc40 to ec0af1664478 (6 revisions) (flutter/engine#31352) * ba8f1c5 Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy (flutter/engine#31291) * 1eafb40 Use H5vcc CanvasKit implementation if it is detected. (flutter/engine#31191) * 6d99e90 Roll Dart SDK from 55c93c732da9 to b7eea441d7d1 (4 revisions) (flutter/engine#31353) * 935b46a Roll Fuchsia Mac SDK from 5_CZ81mTD... to 5CmPcHTb1... (flutter/engine#31356) * 244bb25 Roll Skia from ec0af1664478 to 9cb74e90792d (4 revisions) (flutter/engine#31357) * b6b5759 Roll Skia from 9cb74e90792d to 81d4b5d5b45d (6 revisions) (flutter/engine#31360) * 0f6db11 Roll Skia from 81d4b5d5b45d to 1f813e4c7f6d (1 revision) (flutter/engine#31362) * 321a482 Fix AccessibilityBridge crash due to invalid access during ReplaceSemanticsObject (flutter/engine#31351) * 2cc3abb Manual roll of ICU (flutter/engine#31132) * c1e843d Roll Fuchsia Linux SDK from 4VEg4eRJS... to YGS2LvlDy... (flutter/engine#31364) * fe08734 Roll Dart SDK from b7eea441d7d1 to 7e3310bbe1ed (2 revisions) (flutter/engine#31365) * 2062e57 Add trackpad gesture PointerData types (flutter/engine#28571) * 24fe585 Roll Skia from 1f813e4c7f6d to 5a2135af5623 (1 revision) (flutter/engine#31366) * 11bf86a Migrate string encoding conversions to FML (flutter/engine#31334) * 6d9f479 Roll Skia from 5a2135af5623 to 21a92dff8fdc (3 revisions) (flutter/engine#31368) * 417a05e Roll Dart SDK from 7e3310bbe1ed to a9cfcc289ed4 (1 revision) (flutter/engine#31369) * b04ed63 Change link to felt documentation (flutter/engine#31312) * 3e7515a Roll Fuchsia Mac SDK from 5CmPcHTb1... to ZA5ZzabQM... (flutter/engine#31370) * 90effff Revert "Add trackpad gesture PointerData types (#28571)" (flutter/engine#31375) * 3764a8f Change support for VM service message from "The Dart VM Service is listening" to "The Dart VM service is listening" (flutter/engine#31361) * 3d629c5 Fix html gradient rendering (#97762) (flutter/engine#31355) * 963c449 [Android] Show deprecation warnings for Android tests (flutter/engine#31246) * 0be895c Roll Dart SDK from a9cfcc289ed4 to 0041431f5ec7 (1 revision) (flutter/engine#31372) * 18f2faf Roll Skia from 21a92dff8fdc to c5d3326d767d (7 revisions) (flutter/engine#31373) * 1b762d3 Roll Fuchsia Linux SDK from YGS2LvlDy... to yDo1mhBKz... (flutter/engine#31374) * 7b6a7b6 Roll Skia from c5d3326d767d to b6dfd97c5290 (10 revisions) (flutter/engine#31377) * f55b161 [a11y] Delegate UTF8ToUTF16 to FML (flutter/engine#31376) * d36d25f TextEditingDelta Support for the Web (flutter/engine#28527) * aa17186 [fml] Use common FML string encoding utils (flutter/engine#31378) * 97d9ec3 Renamed the scenario tests target to be generic emulator tests. (flutter/engine#28919) * 902717f Roll Skia from b6dfd97c5290 to e1e2a858205f (3 revisions) (flutter/engine#31380) * 877f820 Roll Dart SDK from 0041431f5ec7 to a15d19a0d914 (2 revisions) (flutter/engine#31381) * 2d16729 Roll Skia from e1e2a858205f to 74ce095463e1 (2 revisions) (flutter/engine#31383) * 8d3c0fb [web] PathRef: do not use == with doubles in assertions (flutter/engine#31382) * d1164c1 Move recipes to repository folders. (flutter/engine#31367) * e031e07 Roll Skia from 74ce095463e1 to 82d65d0487bd (1 revision) (flutter/engine#31384) * 5140a44 Define thread priority enum and set thread priority for all threads in Engine (flutter/engine#30605) * 1e46918 Roll Dart SDK from a15d19a0d914 to a3736d4e9b1b (1 revision) (flutter/engine#31397) * 8a28948 Roll Fuchsia Linux SDK from yDo1mhBKz... to UHV3HWM3d... (flutter/engine#31398) * ca86c79 Roll Fuchsia Mac SDK from ZA5ZzabQM... to jjxstNbgZ... (flutter/engine#31399)

* 2a8f388 Roll Skia from a424b619bc40 to ec0af1664478 (6 revisions) (flutter/engine#31352) * ba8f1c5 Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy (flutter/engine#31291) * 1eafb40 Use H5vcc CanvasKit implementation if it is detected. (flutter/engine#31191) * 6d99e90 Roll Dart SDK from 55c93c732da9 to b7eea441d7d1 (4 revisions) (flutter/engine#31353) * 935b46a Roll Fuchsia Mac SDK from 5_CZ81mTD... to 5CmPcHTb1... (flutter/engine#31356) * 244bb25 Roll Skia from ec0af1664478 to 9cb74e90792d (4 revisions) (flutter/engine#31357) * b6b5759 Roll Skia from 9cb74e90792d to 81d4b5d5b45d (6 revisions) (flutter/engine#31360) * 0f6db11 Roll Skia from 81d4b5d5b45d to 1f813e4c7f6d (1 revision) (flutter/engine#31362) * 321a482 Fix AccessibilityBridge crash due to invalid access during ReplaceSemanticsObject (flutter/engine#31351) * 2cc3abb Manual roll of ICU (flutter/engine#31132) * c1e843d Roll Fuchsia Linux SDK from 4VEg4eRJS... to YGS2LvlDy... (flutter/engine#31364) * fe08734 Roll Dart SDK from b7eea441d7d1 to 7e3310bbe1ed (2 revisions) (flutter/engine#31365) * 2062e57 Add trackpad gesture PointerData types (flutter/engine#28571) * 24fe585 Roll Skia from 1f813e4c7f6d to 5a2135af5623 (1 revision) (flutter/engine#31366) * 11bf86a Migrate string encoding conversions to FML (flutter/engine#31334) * 6d9f479 Roll Skia from 5a2135af5623 to 21a92dff8fdc (3 revisions) (flutter/engine#31368) * 417a05e Roll Dart SDK from 7e3310bbe1ed to a9cfcc289ed4 (1 revision) (flutter/engine#31369) * b04ed63 Change link to felt documentation (flutter/engine#31312) * 3e7515a Roll Fuchsia Mac SDK from 5CmPcHTb1... to ZA5ZzabQM... (flutter/engine#31370) * 90effff Revert "Add trackpad gesture PointerData types (flutter#28571)" (flutter/engine#31375) * 3764a8f Change support for VM service message from "The Dart VM Service is listening" to "The Dart VM service is listening" (flutter/engine#31361) * 3d629c5 Fix html gradient rendering (flutter#97762) (flutter/engine#31355) * 963c449 [Android] Show deprecation warnings for Android tests (flutter/engine#31246) * 0be895c Roll Dart SDK from a9cfcc289ed4 to 0041431f5ec7 (1 revision) (flutter/engine#31372) * 18f2faf Roll Skia from 21a92dff8fdc to c5d3326d767d (7 revisions) (flutter/engine#31373) * 1b762d3 Roll Fuchsia Linux SDK from YGS2LvlDy... to yDo1mhBKz... (flutter/engine#31374) * 7b6a7b6 Roll Skia from c5d3326d767d to b6dfd97c5290 (10 revisions) (flutter/engine#31377) * f55b161 [a11y] Delegate UTF8ToUTF16 to FML (flutter/engine#31376) * d36d25f TextEditingDelta Support for the Web (flutter/engine#28527) * aa17186 [fml] Use common FML string encoding utils (flutter/engine#31378) * 97d9ec3 Renamed the scenario tests target to be generic emulator tests. (flutter/engine#28919) * 902717f Roll Skia from b6dfd97c5290 to e1e2a858205f (3 revisions) (flutter/engine#31380) * 877f820 Roll Dart SDK from 0041431f5ec7 to a15d19a0d914 (2 revisions) (flutter/engine#31381) * 2d16729 Roll Skia from e1e2a858205f to 74ce095463e1 (2 revisions) (flutter/engine#31383) * 8d3c0fb [web] PathRef: do not use == with doubles in assertions (flutter/engine#31382) * d1164c1 Move recipes to repository folders. (flutter/engine#31367) * e031e07 Roll Skia from 74ce095463e1 to 82d65d0487bd (1 revision) (flutter/engine#31384) * 5140a44 Define thread priority enum and set thread priority for all threads in Engine (flutter/engine#30605) * 1e46918 Roll Dart SDK from a15d19a0d914 to a3736d4e9b1b (1 revision) (flutter/engine#31397) * 8a28948 Roll Fuchsia Linux SDK from yDo1mhBKz... to UHV3HWM3d... (flutter/engine#31398) * ca86c79 Roll Fuchsia Mac SDK from ZA5ZzabQM... to jjxstNbgZ... (flutter/engine#31399)

flutter-dashboard bot added embedder Related to the embedder API platform-linux labels Feb 6, 2022

zanderso force-pushed the clang-tidy-warnings branch 4 times, most recently from d7ba7f2 to 33d0e65 Compare February 8, 2022 03:23

flutter-dashboard bot added platform-android platform-ios labels Feb 8, 2022

zanderso requested review from jmagman, iskakaushik, bdero and gaaclarke February 8, 2022 06:06

bdero approved these changes Feb 8, 2022

View reviewed changes

jmagman requested changes Feb 8, 2022

View reviewed changes

jmagman mentioned this pull request Feb 8, 2022

Fix insecureAPI, NonLocalizedStringChecker, NewDeleteLeaks macOS clang errors #31333

Closed

8 tasks

zanderso force-pushed the clang-tidy-warnings branch from 33d0e65 to 8f97cbf Compare February 9, 2022 05:33

Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy

131067f

zanderso force-pushed the clang-tidy-warnings branch from 8f97cbf to 131067f Compare February 9, 2022 07:04

zanderso requested a review from jmagman February 9, 2022 16:15

jmagman approved these changes Feb 9, 2022

View reviewed changes

zanderso merged commit ba8f1c5 into flutter:main Feb 9, 2022

zanderso deleted the clang-tidy-warnings branch February 9, 2022 20:26

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Engine from bed591d39448 to 5140a440c0b5 (38 revisions) flutter/flutter#98234

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

ba8f1c5 Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy (f…

4ada9cf

…lutter/engine#31291)

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Engine from bed591d39448 to 1e46918c13d9 (39 revisions) flutter/flutter#98246

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

ba8f1c5 Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy (f…

0240abe

…lutter/engine#31291)

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Engine from bed591d39448 to ca86c7938d6b (41 revisions) flutter/flutter#98249

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

ba8f1c5 Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy (f…

afa0366

…lutter/engine#31291)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy #31291

Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy #31291

	final io.File compileCommands = io.File(path.join(
	flutterRoot,
	'..',
	'out',
	'host_debug',
	'compile_commands.json',
	));

Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy #31291

Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy #31291

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment