Support mozlint diff

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):
         '''

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,

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

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 []

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')

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),
+        ]

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)

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() {

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
+        }
+    ]
+}