diff --git a/bot/code_review_bot/__init__.py b/bot/code_review_bot/__init__.py index e30f0d765..cdb0043ee 100644 --- a/bot/code_review_bot/__init__.py +++ b/bot/code_review_bot/__init__.py @@ -71,6 +71,37 @@ def is_publishable(self): raise Exception('Unsupported publication mode {}'.format(settings.publication)) + def as_diff(self): + ''' + Outputs as a diff block + ''' + assert self.diff is not None, 'Missing diff source' + fmt = '@@ -{line},{nb_minus} +{line},{nb_plus} @@\n{diff}' + + # Count the number of +/- + counts = {'common': 0, 'plus': 0, 'minus': 0} + clean_diff = [] + for line in self.diff.splitlines(): + if not line or line[0] == ' ': + key = 'common' + elif line[0] == '+': + key = 'plus' + elif line[0] == '-': + key = 'minus' + else: + # Skip invalid lines (like stderr output) + continue + + counts[key] += 1 + clean_diff.append(line) + + return fmt.format( + line=self.line, + diff='\n'.join(clean_diff), + nb_plus=counts['common'] + counts['plus'], + nb_minus=counts['common'] + counts['minus'], + ) + @abc.abstractmethod def validates(self): ''' diff --git a/bot/code_review_bot/report/phabricator.py b/bot/code_review_bot/report/phabricator.py index da51f63cf..bec15e51e 100644 --- a/bot/code_review_bot/report/phabricator.py +++ b/bot/code_review_bot/report/phabricator.py @@ -13,6 +13,7 @@ from code_review_bot import stats from code_review_bot.report.base import Reporter from code_review_bot.revisions import Revision +from code_review_bot.tasks.lint import MozLintIssue MODE_COMMENT = 'comment' MODE_HARBORMASTER = 'harbormaster' @@ -151,6 +152,12 @@ def comment_inline(self, revision, issue, existing_comments=[]): logger.warn('Will not publish inline comment on invalid path {}: {}'.format(issue.path, issue)) return + # Skip mozlint publication when a diff is available + # These issues are listed in an improvement patch + if isinstance(issue, MozLintIssue) and issue.diff is not None: + logger.info('Will not publish inline comment on formatting change: {}'.format(issue)) + return + # Check if comment is already posted comment = { 'diffID': revision.diff_id, diff --git a/bot/code_review_bot/revisions.py b/bot/code_review_bot/revisions.py index cd22809c5..60b3b64cb 100644 --- a/bot/code_review_bot/revisions.py +++ b/bot/code_review_bot/revisions.py @@ -172,7 +172,7 @@ def contains(self, issue): # Get modified lines for this issue modified_lines = self.lines.get(issue.path) if modified_lines is None: - logger.warn('Issue path is not in revision', path=issue.path, revision=self) + logger.debug('Issue path is not in revision', path=issue.path, revision=self) return False # Detect if this issue is in the patch diff --git a/bot/code_review_bot/tasks/base.py b/bot/code_review_bot/tasks/base.py index afa603796..fc53f57f3 100644 --- a/bot/code_review_bot/tasks/base.py +++ b/bot/code_review_bot/tasks/base.py @@ -108,10 +108,11 @@ def clean_path(self, path): path = path[1:] return path - def build_patches(self, artifacts): + def build_patches(self, artifacts, issues): ''' Some analyzers can provide a patch appliable by developers These patches are stored as Taskcluster artifacts and reported to developpers + They can also be built from issues produced earlier by the bot Output is a list of tuple (patch name as str, patch content as str) ''' return [] diff --git a/bot/code_review_bot/tasks/clang_format.py b/bot/code_review_bot/tasks/clang_format.py index 4bde6aab2..38fecc8d4 100644 --- a/bot/code_review_bot/tasks/clang_format.py +++ b/bot/code_review_bot/tasks/clang_format.py @@ -139,7 +139,7 @@ def parse_issues(self, artifacts, revision): for issue in issues ] - def build_patches(self, artifacts): + def build_patches(self, artifacts, issues): artifact = artifacts.get('public/code-review/clang-format.diff') if artifact is None: logger.warn('Missing or empty clang-format.diff') diff --git a/bot/code_review_bot/tasks/lint.py b/bot/code_review_bot/tasks/lint.py index cb22655b6..0052e6f0b 100644 --- a/bot/code_review_bot/tasks/lint.py +++ b/bot/code_review_bot/tasks/lint.py @@ -1,4 +1,7 @@ # -*- coding: utf-8 -*- +import itertools +from datetime import datetime + import structlog from libmozdata.phabricator import LintResult @@ -28,8 +31,7 @@ class MozLintIssue(Issue): ANALYZER = MOZLINT - def __init__(self, path, column, level, lineno, linter, message, rule, revision, **kwargs): - self.nb_lines = 1 + def __init__(self, path, column, level, lineno, linter, message, rule, revision, diff=None): self.column = column self.level = level self.line = lineno and int(lineno) or 0 # mozlint sometimes produce strings here @@ -38,13 +40,22 @@ def __init__(self, path, column, level, lineno, linter, message, rule, revision, self.rule = rule self.revision = revision self.path = path + self.diff = diff + + # Calc number of lines from patch when available + if isinstance(self.diff, str): + lines = self.diff.splitlines() + self.nb_lines = len(lines) + else: + self.nb_lines = 1 def __str__(self): return '{} issue {} {} line {}'.format( self.linter, self.level, self.path, - self.line, + # Display line range when multiple lines are in patch + '{}-{}'.format(self.line, self.line + self.nb_lines) if self.nb_lines > 1 else self.line, ) def build_extra_identifiers(self): @@ -75,8 +86,9 @@ def validates(self): A mozlint issues is publishable when: * file is not 3rd party * rule is not disabled + * issues without diff (those are published through a patch) ''' - return not self.is_third_party() and not self.is_disabled_rule() + return not self.is_third_party() and not self.is_disabled_rule() and self.diff is None def as_text(self): ''' @@ -178,8 +190,50 @@ def parse_issues(self, artifacts, revision): linter=issue['linter'], message=issue['message'], rule=issue['rule'], + diff=issue.get('diff'), ) for artifact in artifacts.values() for path, path_issues in artifact.items() for issue in path_issues ] + + def build_patches(self, artifacts, issues): + ''' + Build an improvement patch from issues with diff + Any issue on a file in patch will be posted + ''' + diff_issues = [ + i + for i in issues + if i.revision.has_file(i.path) and i.diff is not None + ] + if not diff_issues: + return [] + + header_fmt = '--- {path}\t{date}\n+++ {path}\t{date}\n' + + # Group issues by path + patch = '' + for path, path_issues in itertools.groupby(diff_issues, lambda i: i.path): + + if patch: + patch += '\n' + + # Add header for path + patch += header_fmt.format( + date=datetime.utcnow(), + path=path, + ) + + # Add each diff block, avoiding duplicates + # sorted by top line + chunks = [] + for issue in path_issues: + chunk = (issue.line, issue.as_diff()) + if chunk not in chunks: + chunks.append(chunk) + patch += '\n'.join(c[1] for c in sorted(chunks, key=lambda c: c[0])) + + return [ + ('mozlint', patch), + ] diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index d38640fda..eb0be38de 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -239,7 +239,7 @@ def _in_group(dep_id): logger.info('Found {} issues'.format(len(task_issues)), task=task.name, id=task.id) issues += task_issues - for name, patch in task.build_patches(artifacts): + for name, patch in task.build_patches(artifacts, task_issues): revision.add_improvement_patch(name, patch) except Exception as e: logger.warn('Failure during task analysis', task=settings.taskcluster.task_id, error=e) diff --git a/bot/tests/mocks/mozlint_rust_issues.diff b/bot/tests/mocks/mozlint_rust_issues.diff new file mode 100644 index 000000000..316fa4525 --- /dev/null +++ b/bot/tests/mocks/mozlint_rust_issues.diff @@ -0,0 +1,26 @@ +--- path/to/xx.rs 2019-01-01 00:00:00 ++++ path/to/xx.rs 2019-01-01 00:00:00 +@@ -33,7 +33,6 @@ + include!(concat!(env!("OUT_DIR"), "/bindings.rs")); + +-impl CraneliftFuncCompileInput +-{ ++impl CraneliftFuncCompileInput { + pub fn bytecode(&self) -> &[u8] { + use std::slice; + unsafe { slice::from_raw_parts(self.bytecode, self.bytecodeSize) } +@@ -100,2 +100,2 @@ + multi ++lines +-words +--- test.rs 2019-01-01 00:00:00 ++++ test.rs 2019-01-01 00:00:00 +@@ -202,7 +202,7 @@ + "Failed to generate bindings, flags: {:?}", + command_line_opts + ); +- }, ++ } + }; + + for fixup in fixups.iter() { \ No newline at end of file diff --git a/bot/tests/mocks/mozlint_rust_issues.json b/bot/tests/mocks/mozlint_rust_issues.json new file mode 100644 index 000000000..13771376f --- /dev/null +++ b/bot/tests/mocks/mozlint_rust_issues.json @@ -0,0 +1,86 @@ +{ + "missing.rs": [ + { + "column": null, + "diff": "does not matter", + "hint": null, + "level": "warning", + "lineno": 1, + "lineoffset": null, + "linter": "rust", + "message": "Reformat rust", + "path": "missing.rs", + "rule": null, + "source": null + } + ], + "path/to/xx.rs": [ + { + "column": null, + "diff": " multi\n+lines\n-words", + "hint": null, + "level": "warning", + "lineno": 100, + "lineoffset": null, + "linter": "rust", + "message": "Reformat rust", + "path": "path/to/xx.rs", + "rule": null, + "source": null + }, + { + "column": null, + "diff": "Warning: XXX on stderr\n include!(concat!(env!(\"OUT_DIR\"), \"/bindings.rs\"));\n\n-impl CraneliftFuncCompileInput\n-{\n+impl CraneliftFuncCompileInput {\n pub fn bytecode(&self) -> &[u8] {\n use std::slice;\n unsafe { slice::from_raw_parts(self.bytecode, self.bytecodeSize) }", + "hint": null, + "level": "warning", + "lineno": 33, + "lineoffset": null, + "linter": "rust", + "message": "Reformat rust", + "path": "path/to/xx.rs", + "rule": null, + "source": null + }, + { + "column": null, + "diff": "Warning: another one\n include!(concat!(env!(\"OUT_DIR\"), \"/bindings.rs\"));\n\n-impl CraneliftFuncCompileInput\n-{\n+impl CraneliftFuncCompileInput {\n pub fn bytecode(&self) -> &[u8] {\n use std::slice;\n unsafe { slice::from_raw_parts(self.bytecode, self.bytecodeSize) }", + "hint": null, + "level": "warning", + "lineno": 33, + "lineoffset": null, + "linter": "rust", + "message": "Reformat rust", + "path": "path/to/xx.rs", + "rule": null, + "source": null + } + ], + "test.rs": [ + { + "column": null, + "diff": " \"Failed to generate bindings, flags: {:?}\",\n command_line_opts\n );\n- },\n+ }\n };\n\n for fixup in fixups.iter() {\n", + "hint": null, + "level": "warning", + "lineno": 202, + "lineoffset": null, + "linter": "rust", + "message": "Reformat rust", + "path": "test.rs", + "rule": null, + "source": null + }, + { + "column": null, + "diff": " \"Failed to generate bindings, flags: {:?}\",\n command_line_opts\n );\n- },\n+ }\n };\n\n for fixup in fixups.iter() {\n", + "hint": null, + "level": "warning", + "lineno": 202, + "lineoffset": null, + "linter": "rust", + "message": "Reformat rust", + "path": "test.rs", + "rule": null, + "source": null + } + ] +} diff --git a/bot/tests/test_lint.py b/bot/tests/test_lint.py index 74f76d81d..7e6a6725e 100644 --- a/bot/tests/test_lint.py +++ b/bot/tests/test_lint.py @@ -2,6 +2,13 @@ # This Source Code Form is subject to the terms of the Mozilla Public # License, v. 2.0. If a copy of the MPL was not distributed with this # file, You can obtain one at http://mozilla.org/MPL/2.0/. +import json +import os +from datetime import datetime +from unittest.mock import Mock +from unittest.mock import patch + +from conftest import MOCK_DIR def test_flake8_rules(mock_config, mock_revision): @@ -29,7 +36,7 @@ def test_flake8_rules(mock_config, mock_revision): def test_as_text(mock_config, mock_revision): ''' - Test text export for ClangTidyIssue + Test text export for MozLintIssue ''' from code_review_bot.tasks.lint import MozLintIssue @@ -46,3 +53,68 @@ def test_as_text(mock_config, mock_revision): 'path': 'test.py', 'severity': 'error', } + + +def test_diff(mock_config, mock_revision): + ''' + Test diff parsing in MozLintIssue + ''' + from code_review_bot.tasks.lint import MozLintIssue + + mock_revision.lines = { + 'test.rs': [1, 2, 44], + } + + issue = MozLintIssue('test.rs', 1, 'error', 42, 'rustfmt', 'nodiff', 'dummy rule', mock_revision) + assert str(issue) == 'rustfmt issue error test.rs line 42' + assert issue.diff is None + assert issue.nb_lines == 1 + assert not mock_revision.contains(issue) + + diff = '''This +is +a +test''' + issue = MozLintIssue('test.rs', 1, 'error', 42, 'rustfmt', 'withdiff', 'dummy rule', mock_revision, diff=diff) + assert str(issue) == 'rustfmt issue error test.rs line 42-46' + assert issue.diff is not None + assert mock_revision.contains(issue) + + +@patch('code_review_bot.tasks.lint.datetime') +def test_diff_build(mock_datetime, mock_revision): + ''' + Build a full diff from a list of issues + ''' + from code_review_bot.tasks.lint import MozLintTask + + # Set a constant now datetime + mock_datetime.utcnow = Mock(return_value=datetime(2019, 1, 1)) + + task_status = { + 'task': {}, + 'status': { + 'status': 'completed', + }, + } + task = MozLintTask('someTaskId', task_status) + + mock_revision.lines = { + 'path/to/xx.rs': [1, 2, 3, ], + 'test.rs': [200, 201, 202, 300], + } + mock_revision.files = mock_revision.lines.keys() + + # Parse issues from mock mozlint artifact + path = os.path.join(MOCK_DIR, 'mozlint_rust_issues.json') + artifacts = { + 'public/code-review/mozlint.json': json.load(open(path)), + } + issues = task.parse_issues(artifacts, mock_revision) + assert len(issues) == 6 + + # Build patches from issues + patches = task.build_patches(artifacts, issues) + assert len(patches) == 1 + path = os.path.join(MOCK_DIR, 'mozlint_rust_issues.diff') + assert patches[0] == ('mozlint', open(path).read())