From d62c7b3e678ce8aa052229c875df81a2a1ee17df Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Wed, 4 Jul 2018 10:03:07 +0200 Subject: [PATCH] shipit-static-analysis: Enhance issue body parser, fixes #1251, #1254 --- .../shipit_static_analysis/clang/tidy.py | 29 ++- src/shipit_static_analysis/tests/conftest.py | 20 ++ .../tests/mocks/clang-issues.txt | 66 +++++++ .../tests/mocks/clang-output.txt | 186 ++++++++++++++++++ .../tests/test_clang.py | 11 +- 5 files changed, 303 insertions(+), 9 deletions(-) create mode 100644 src/shipit_static_analysis/tests/mocks/clang-issues.txt create mode 100644 src/shipit_static_analysis/tests/mocks/clang-output.txt diff --git a/src/shipit_static_analysis/shipit_static_analysis/clang/tidy.py b/src/shipit_static_analysis/shipit_static_analysis/clang/tidy.py index 56d1fcc383..026e42a6dd 100755 --- a/src/shipit_static_analysis/shipit_static_analysis/clang/tidy.py +++ b/src/shipit_static_analysis/shipit_static_analysis/clang/tidy.py @@ -18,7 +18,9 @@ logger = get_logger(__name__) REGEX_HEADER = re.compile(r'^(.+):(\d+):(\d+): (warning|error|note): ([^\[\]\n]+)(?: \[([\.\w-]+)\])?$', re.MULTILINE) -REGEX_HAS_WARNINGS = re.compile(r'^(\d+) warnings present.$', re.MULTILINE) +REGEX_FOOTER = re.compile(r'^(Warning: [\.\w-]+ in .+:)|(Suppressed \d+ warnings)|(\d+ warnings? and \d+ errors? generated.)|(Error while processing)', re.MULTILINE) # noqa +REGEX_HAS_WARNINGS = re.compile(r'^(\d+) warnings|errors present.$', re.MULTILINE) + ISSUE_MARKDOWN = ''' ## clang-tidy {type} @@ -140,16 +142,31 @@ def parse_issues(self, clang_output, revision): if not headers: raise Exception('No clang-tidy header was found even though a clang output was provided') + def _remove_footer(start_pos, end_pos): + ''' + Build an issue body from clang-tidy output + and stops when an extra paylaod is detected (footer) + ''' + assert isinstance(start_pos, int) + assert isinstance(end_pos, int) + body = clang_output[start_pos:end_pos] + footer = REGEX_FOOTER.search(body) + if footer is None: + return body + return body[:footer.start()-1] + issues = [] for i, header in enumerate(headers): issue = ClangTidyIssue(header.groups(), revision) # Get next header if i+1 < len(headers): + # Parse body until next header next_header = headers[i+1] - issue.body = clang_output[header.end():next_header.start() - 1] + issue.body = _remove_footer(header.end(), next_header.start() - 1) else: - issue.body = clang_output[header.end():] + # Limit last element to 3 lines to avoid parsing extra metadatas + issue.body = _remove_footer(header.end(), header.end() + 3) if issue.is_problem(): # Save problem to append notes @@ -273,10 +290,8 @@ def as_text(self): self.check, ) - # Add body when it's more than 2 lines - # it generally contains useful info - lines = len(list(filter(None, self.body.split('\n')))) - if lines > 2: + # Always add body as it's been cleaned up + if self.body: body += '\n{}'.format(self.body) return body diff --git a/src/shipit_static_analysis/tests/conftest.py b/src/shipit_static_analysis/tests/conftest.py index 624df0b41d..2f611a4f33 100644 --- a/src/shipit_static_analysis/tests/conftest.py +++ b/src/shipit_static_analysis/tests/conftest.py @@ -415,3 +415,23 @@ def test_cpp(mock_config, mock_repository): path = os.path.join(mock_config.repo_dir, 'test.cpp') with open(path, 'w') as f: f.write(TEST_CPP) + + +@pytest.fixture +def mock_clang_output(): + ''' + Load a real case clang output + ''' + path = os.path.join(MOCK_DIR, 'clang-output.txt') + with open(path) as f: + return f.read() + + +@pytest.fixture +def mock_clang_issues(): + ''' + Load parsed issues from a real case (above) + ''' + path = os.path.join(MOCK_DIR, 'clang-issues.txt') + with open(path) as f: + return f.read() diff --git a/src/shipit_static_analysis/tests/mocks/clang-issues.txt b/src/shipit_static_analysis/tests/mocks/clang-issues.txt new file mode 100644 index 0000000000..f62dede7b3 --- /dev/null +++ b/src/shipit_static_analysis/tests/mocks/clang-issues.txt @@ -0,0 +1,66 @@ +Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] + + else // readability-else-after-return + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +-------------------- +Error: Methods annotated with MOZ_NO_DANGLING_ON_TEMPORARIES cannot be && ref-qualified [clang-tidy: mozilla-dangling-on-temporary] + + MOZ_NO_DANGLING_ON_TEMPORARIES int *get() && { return nullptr; } // mozilla-dangling-on-temporary + ^ +-------------------- +Error: Methods annotated with MOZ_NO_DANGLING_ON_TEMPORARIES must return a pointer [clang-tidy: mozilla-dangling-on-temporary] + + MOZ_NO_DANGLING_ON_TEMPORARIES int test() { return 0; } // mozilla-dangling-on-temporary + ^ +-------------------- +Warning: Redundant void argument list in function definition [clang-tidy: modernize-redundant-void-arg] + +JS::ObjectOpResult::failCantRedefineProp(void) // modernize-redundant-void-arg + ^~~~~ +-------------------- +Warning: Redundant void argument list in function definition [clang-tidy: modernize-redundant-void-arg] + +JS_GetImplementationVersion(void) + ^~~~~ +-------------------- +Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal] + + "\"string\", \"number\", or \"default\"", source); + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + R"("string", "number", or "default")" +-------------------- +Warning: Use auto when initializing with a cast to avoid duplicating the type name [clang-tidy: modernize-use-auto] + + uintptr_t u = reinterpret_cast(name); + ^~~~~~~~~ + auto +-------------------- +Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] + + AutoFile() + ^ +-------------------- +Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default] + + ~AutoFile() + ^ +-------------------- +Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default] + +JS::AutoSetAsyncStackForNewCalls::~AutoSetAsyncStackForNewCalls() + ^ +-------------------- +Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default] + +JSErrorNotes::JSErrorNotes() + ^ +-------------------- +Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default] + +JSErrorNotes::~JSErrorNotes() + ^ +-------------------- +Warning: Redundant void argument list in function definition [clang-tidy: modernize-redundant-void-arg] + +JS_PUBLIC_API(void) + ^~~~~ \ No newline at end of file diff --git a/src/shipit_static_analysis/tests/mocks/clang-output.txt b/src/shipit_static_analysis/tests/mocks/clang-output.txt new file mode 100644 index 0000000000..44903b3e5d --- /dev/null +++ b/src/shipit_static_analysis/tests/mocks/clang-output.txt @@ -0,0 +1,186 @@ +/nix/store/lhp5rw0dagi5mgqwr9i3x41240ba4ypz-gnumake-4.2.1/bin/make -C /app/tmp/sa-central/obj-x86_64-pc-linux-gnu -j4 -s -w pre-export +make: Entering directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu' +BUILDSTATUS TIERS pre-export +BUILDSTATUS TIER_START pre-export +make[1]: Entering directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu' +Elapsed: 0.00s; From dist/private: Kept 0 existing; Added/updated 0; Removed 0 files and 0 directories. +Elapsed: 0.00s; From dist/public: Kept 0 existing; Added/updated 0; Removed 0 files and 0 directories. +Elapsed: 0.00s; From dist/xpi-stage: Kept 18 existing; Added/updated 0; Removed 0 files and 0 directories. +Elapsed: 0.20s; From _tests: Kept 1023 existing; Added/updated 0; Removed 0 files and 0 directories. +Elapsed: 0.57s; From dist/include: Kept 5315 existing; Added/updated 0; Removed 0 files and 0 directories. +Elapsed: 0.35s; From dist/bin: Kept 2379 existing; Added/updated 0; Removed 0 files and 0 directories. +make[1]: Leaving directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu' +BUILDSTATUS TIER_FINISH pre-export +make: Leaving directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu' +/nix/store/lhp5rw0dagi5mgqwr9i3x41240ba4ypz-gnumake-4.2.1/bin/make -C /app/tmp/sa-central/obj-x86_64-pc-linux-gnu -j4 -s -w export +make: Entering directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu' +buildid.h.stub +source-repo.h.stub +BUILDSTATUS TIERS export +BUILDSTATUS TIER_START export +make[1]: Entering directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu' +make[2]: Entering directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/build' +application.ini.stub +application.ini.h.stub +make[2]: Leaving directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/build' +make[2]: Entering directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/browser/locales' +make[2]: Leaving directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/browser/locales' +make[2]: Entering directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/xpcom/xpidl' +make[2]: Leaving directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/xpcom/xpidl' +make[2]: Entering directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/toolkit/components/telemetry' +TelemetryEventData.h.stub +make[2]: Leaving directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/toolkit/components/telemetry' +make[1]: Leaving directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu' +BUILDSTATUS TIER_FINISH export +make: Leaving directory '/app/tmp/sa-central/obj-x86_64-pc-linux-gnu' +/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python /tmp/mozilla-state/clang-tools/clang/share/clang/run-clang-tidy.py -p /app/tmp/sa-central/obj-x86_64-pc-linux-gnu -j 0 js/src/jsapi.cpp devtools/client/shared/scroll.js js/src/ctypes/libffi/src/prep_cif.c devtools/client/menus.js devtools/client/definitions.js .arcconfig toolkit/components/telemetry/gen_event_data.py -clang-tidy-binary /tmp/mozilla-state/clang-tools/clang/bin/clang-tidy -clang-apply-replacements-binary /tmp/mozilla-state/clang-tools/clang/bin/clang-apply-replacements -checks=-*,clang-analyzer-deadcode.DeadStores,clang-analyzer-security.FloatLoopCounter,clang-analyzer-security.insecureAPI.getpw,clang-analyzer-security.insecureAPI.mkstemp,clang-analyzer-security.insecureAPI.mktemp,clang-analyzer-security.insecureAPI.rand,clang-analyzer-security.insecureAPI.strcpy,clang-analyzer-security.insecureAPI.UncheckedReturn,clang-analyzer-security.insecureAPI.vfork,misc-argument-comment,misc-assert-side-effect,misc-forward-declaration-namespace,misc-suspicious-missing-comma,misc-suspicious-semicolon,misc-unused-using-decls,modernize-avoid-bind,modernize-loop-convert,modernize-raw-string-literal,modernize-redundant-void-arg,modernize-shrink-to-fit,modernize-use-auto,modernize-use-bool-literals,modernize-use-equals-default,modernize-use-equals-delete,modernize-use-nullptr,modernize-use-override,mozilla-*,performance-faster-string-find,performance-for-range-copy,performance-inefficient-string-concatenation,performance-inefficient-vector-operation,performance-type-promotion-in-math-fn,performance-unnecessary-copy-initialization,performance-unnecessary-value-param,readability-container-size-empty,readability-else-after-return,readability-misleading-indentation,readability-redundant-control-flow,readability-redundant-smartptr-get,readability-redundant-string-cstr,readability-redundant-string-init,readability-uniqueptr-delete-release -extra-arg=-DMOZ_CLANG_PLUGIN -header-filter=jsapi.cpp|scroll.js|prep_cif.c|menus.js|definitions.js|.arcconfig|gen_event_data.py +Enabled checks: + clang-analyzer-core.CallAndMessage + clang-analyzer-core.DivideZero + clang-analyzer-core.DynamicTypePropagation + clang-analyzer-core.NonNullParamChecker + clang-analyzer-core.NullDereference + clang-analyzer-core.StackAddressEscape + clang-analyzer-core.UndefinedBinaryOperatorResult + clang-analyzer-core.VLASize + clang-analyzer-core.builtin.BuiltinFunctions + clang-analyzer-core.builtin.NoReturnFunctions + clang-analyzer-core.uninitialized.ArraySubscript + clang-analyzer-core.uninitialized.Assign + clang-analyzer-core.uninitialized.Branch + clang-analyzer-core.uninitialized.CapturedBlockVariable + clang-analyzer-core.uninitialized.UndefReturn + clang-analyzer-deadcode.DeadStores + clang-analyzer-security.FloatLoopCounter + clang-analyzer-security.insecureAPI.UncheckedReturn + clang-analyzer-security.insecureAPI.getpw + clang-analyzer-security.insecureAPI.mkstemp + clang-analyzer-security.insecureAPI.mktemp + clang-analyzer-security.insecureAPI.rand + clang-analyzer-security.insecureAPI.strcpy + clang-analyzer-security.insecureAPI.vfork + misc-argument-comment + misc-assert-side-effect + misc-forward-declaration-namespace + misc-suspicious-missing-comma + misc-suspicious-semicolon + misc-unused-using-decls + modernize-avoid-bind + modernize-loop-convert + modernize-raw-string-literal + modernize-redundant-void-arg + modernize-shrink-to-fit + modernize-use-auto + modernize-use-bool-literals + modernize-use-equals-default + modernize-use-equals-delete + modernize-use-nullptr + modernize-use-override + mozilla-arithmetic-argument + mozilla-assignment-in-assert + mozilla-can-run-script + mozilla-dangling-on-temporary + mozilla-explicit-operator-bool + mozilla-implicit-constructor + mozilla-kungfu-death-grip + mozilla-must-override + mozilla-must-return-from-caller + mozilla-must-use + mozilla-nan-expr + mozilla-needs-no-vtable-type + mozilla-no-addref-release-on-return + mozilla-no-auto-type + mozilla-no-duplicate-refcnt-member + mozilla-no-explicit-move-constructor + mozilla-non-memmovable-member + mozilla-non-memmovable-template-arg + mozilla-override-base-call + mozilla-override-base-call-usage + mozilla-paramtraits-enum + mozilla-refcounted-copy-constructor + mozilla-refcounted-inside-lambda + mozilla-scope + mozilla-sprintf-literal + mozilla-trivial-constructor-destructor + performance-faster-string-find + performance-for-range-copy + performance-inefficient-string-concatenation + performance-inefficient-vector-operation + performance-type-promotion-in-math-fn + performance-unnecessary-copy-initialization + performance-unnecessary-value-param + readability-container-size-empty + readability-else-after-return + readability-misleading-indentation + readability-redundant-control-flow + readability-redundant-smartptr-get + readability-redundant-string-cstr + readability-redundant-string-init + readability-uniqueptr-delete-release + + +Error while processing /app/tmp/sa-central/js/src/ctypes/libffi/src/prep_cif.c. +/app/tmp/sa-central/js/src/ctypes/libffi/include/ffi_common.h:23:12: error: 'alloca.h' file not found [clang-diagnostic-error] +# include + ^ +Warning: readability-else-after-return in /app/tmp/sa-central/js/src/ctypes/libffi/src/prep_cif.c: do not use 'else' after 'return' +/app/tmp/sa-central/js/src/ctypes/libffi/src/prep_cif.c:88:3: warning: do not use 'else' after 'return' [readability-else-after-return] + else // readability-else-after-return + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +796 warnings and 3 errors generated. +Error while processing /app/tmp/sa-central/js/src/jsapi.cpp. +/app/tmp/sa-central/js/src/jsapi.cpp:132:39: error: methods annotated with MOZ_NO_DANGLING_ON_TEMPORARIES cannot be && ref-qualified [mozilla-dangling-on-temporary] + MOZ_NO_DANGLING_ON_TEMPORARIES int *get() && { return nullptr; } // mozilla-dangling-on-temporary + ^ +/app/tmp/sa-central/js/src/jsapi.cpp:133:38: error: methods annotated with MOZ_NO_DANGLING_ON_TEMPORARIES must return a pointer [mozilla-dangling-on-temporary] + MOZ_NO_DANGLING_ON_TEMPORARIES int test() { return 0; } // mozilla-dangling-on-temporary + ^ +Warning: modernize-redundant-void-arg in /app/tmp/sa-central/js/src/jsapi.cpp: redundant void argument list in function definition +/app/tmp/sa-central/js/src/jsapi.cpp:244:42: warning: redundant void argument list in function definition [modernize-redundant-void-arg] +JS::ObjectOpResult::failCantRedefineProp(void) // modernize-redundant-void-arg + ^~~~~ +Warning: modernize-redundant-void-arg in /app/tmp/sa-central/js/src/jsapi.cpp: redundant void argument list in function definition +/app/tmp/sa-central/js/src/jsapi.cpp:648:29: warning: redundant void argument list in function definition [modernize-redundant-void-arg] +JS_GetImplementationVersion(void) + ^~~~~ +Warning: modernize-raw-string-literal in /app/tmp/sa-central/js/src/jsapi.cpp: escaped string literal can be written as a raw string literal +/app/tmp/sa-central/js/src/jsapi.cpp:1715:32: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal] + "\"string\", \"number\", or \"default\"", source); + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + R"("string", "number", or "default")" +Warning: modernize-use-auto in /app/tmp/sa-central/js/src/jsapi.cpp: use auto when initializing with a cast to avoid duplicating the type name +/app/tmp/sa-central/js/src/jsapi.cpp:3204:5: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto] + uintptr_t u = reinterpret_cast(name); + ^~~~~~~~~ + auto +Warning: modernize-use-equals-default in /app/tmp/sa-central/js/src/jsapi.cpp: use '= default' to define a trivial default constructor +/app/tmp/sa-central/js/src/jsapi.cpp:3830:5: warning: use '= default' to define a trivial default constructor [modernize-use-equals-default] + AutoFile() + ^ +Warning: modernize-use-equals-default in /app/tmp/sa-central/js/src/jsapi.cpp: use '= default' to define a trivial destructor +/app/tmp/sa-central/js/src/jsapi.cpp:3833:5: warning: use '= default' to define a trivial destructor [modernize-use-equals-default] + ~AutoFile() + ^ +Warning: modernize-use-equals-default in /app/tmp/sa-central/js/src/jsapi.cpp: use '= default' to define a trivial destructor +/app/tmp/sa-central/js/src/jsapi.cpp:5746:35: warning: use '= default' to define a trivial destructor [modernize-use-equals-default] +JS::AutoSetAsyncStackForNewCalls::~AutoSetAsyncStackForNewCalls() + ^ +Warning: modernize-use-equals-default in /app/tmp/sa-central/js/src/jsapi.cpp: use '= default' to define a trivial default constructor +/app/tmp/sa-central/js/src/jsapi.cpp:6968:15: warning: use '= default' to define a trivial default constructor [modernize-use-equals-default] +JSErrorNotes::JSErrorNotes() + ^ +Warning: modernize-use-equals-default in /app/tmp/sa-central/js/src/jsapi.cpp: use '= default' to define a trivial destructor +/app/tmp/sa-central/js/src/jsapi.cpp:6972:15: warning: use '= default' to define a trivial destructor [modernize-use-equals-default] +JSErrorNotes::~JSErrorNotes() + ^ +Warning: modernize-redundant-void-arg in /app/tmp/sa-central/js/src/jsapi.cpp: redundant void argument list in function definition +/app/tmp/sa-central/js/src/jsapi.cpp:7806:15: warning: redundant void argument list in function definition [modernize-redundant-void-arg] +JS_PUBLIC_API(void) + ^~~~~ +/app/tmp/sa-central/obj-x86_64-pc-linux-gnu/dist/system_wrappers/cstddef:3:15: error: 'cstddef' file not found [clang-diagnostic-error] +#include_next + ^ +Suppressed 786 warnings (786 in non-user code). +Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well. +ce-for-range-copy,performance-inefficient-string-concatenation,performance-inefficient-vector-operation,performance-type-promotion-in-math-fn,performance-unnecessary-copy-initialization,performance-unnecessary-value-param,readability-container-size-empty,readability-else-after-return,readability-misleading-indentation,readability-redundant-control-flow,readability-redundant-smartptr-get,readability-redundant-string-cstr,readability-redundant-string-init,readability-uniqueptr-delete-release -extra-arg=-DMOZ_CLANG_PLUGIN -p=/app/tmp/sa-central/obj-x86_64-pc-linux-gnu /app/tmp/sa-central/js/src/ctypes/libffi/src/prep_cif.c +11 warnings present. diff --git a/src/shipit_static_analysis/tests/test_clang.py b/src/shipit_static_analysis/tests/test_clang.py index 54563f9912..f564306ad2 100644 --- a/src/shipit_static_analysis/tests/test_clang.py +++ b/src/shipit_static_analysis/tests/test_clang.py @@ -178,7 +178,7 @@ def test_clang_tidy_checks(mock_config, mock_repository, mock_clang): 'Missing clang-tidy checks: {}'.format(', '.join(missing)) -def test_clang_tidy_parser(mock_config, mock_repository, mock_revision): +def test_clang_tidy_parser(mock_config, mock_repository, mock_revision, mock_clang_output, mock_clang_issues): ''' Test the clang-tidy (or mach static-analysis) parser ''' @@ -209,6 +209,13 @@ def test_clang_tidy_parser(mock_config, mock_repository, mock_revision): assert issues[0].line == 42 assert issues[0].check == 'mozilla-dangling-on-temporary' + # Real case + issues = clang_tidy.parse_issues(mock_clang_output, mock_revision) + assert len(issues) == 13 + sep = '\n' + '-'*20 + '\n' + summary = sep.join(issue.as_text() for issue in issues) + assert summary == mock_clang_issues + def test_as_text(mock_revision): ''' @@ -219,7 +226,7 @@ def test_as_text(mock_revision): issue = ClangTidyIssue(parts, mock_revision) issue.body = 'Dummy body withUppercaseChars' - assert issue.as_text() == 'Error: Dummy message withUppercaseChars [clang-tidy: dummy-check]' + assert issue.as_text() == 'Error: Dummy message withUppercaseChars [clang-tidy: dummy-check]\nDummy body withUppercaseChars' def test_as_markdown(mock_revision):