Bug 1288425 - Make sure we skip invalid extensions when linting with --rev or --workdir, r=smacleod
☠☠ backed out by 2f3e55cbfea9 ☠ ☠
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Tue, 19 Jul 2016 13:50:25 -0400
changeset 348849 2141360b41377772483eda4f7b360ab1a4011cbf
parent 348848 acec32a2a1762b73b7dbce4c87fde3bd05562919
child 348850 fdbbce412740ddc6a4071faf4e4f99a22efdcc1d
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1288425 - Make sure we skip invalid extensions when linting with --rev or --workdir, r=smacleod Some linters, such as flake8, will lint invalid file extensions if you explicitly pass them in. E.g, |flake8 foobar.js| will result in flake8 attempting to lint a JS file. This is a problem because passing in files explicitly is exactly what the --rev/--workdir options do. If a developer modifies a JS file then runs |mach lint -l flake8 -w|, that JS file will get linted. To prevent this, mozlint needs to handle file extensions instead of relying on the underlying linter to do it. This patch adds an "extensions" config option to the LINTER dict, and will filter these files out as part of the 'filterpaths' steps. MozReview-Commit-ID: KYhC6SEySC3
--- a/python/mozlint/mozlint/pathutils.py
+++ b/python/mozlint/mozlint/pathutils.py
@@ -21,16 +21,20 @@ class FilterPath(object):
     def finder(self):
         if self._finder:
             return self._finder
         self._finder = FileFinder(
             self.path, find_executables=False, ignore=self.exclude)
         return self._finder
+    def ext(self):
+        return os.path.splitext(self.path)[1]
+    @property
     def exists(self):
         return os.path.exists(self.path)
     def isfile(self):
         return os.path.isfile(self.path)
@@ -53,46 +57,56 @@ class FilterPath(object):
         if b.startswith(a):
             return True
         return False
     def __repr__(self):
         return repr(self.path)
-def filterpaths(paths, include=None, exclude=None):
+def filterpaths(paths, linter, **lintargs):
     """Filters a list of paths.
-    Given a list of paths, and a list of include and exclude
-    directives, return the set of paths that should be linted.
+    Given a list of paths, and a linter definition plus extra
+    arguments, return the set of paths that should be linted.
     :param paths: A starting list of paths to possibly lint.
-    :param include: A list of include directives. May contain glob patterns.
-    :param exclude: A list of exclude directives. May contain glob patterns.
-    :returns: A tuple containing a list of file paths to lint, and a list
-              of file paths that should be excluded (but that the algorithm
-              was unable to apply).
+    :param linter: A linter definition.
+    :param lintargs: Extra arguments passed to the linter.
+    :returns: A list of file paths to lint.
-    if not include and not exclude:
+    include = linter.get('include')
+    exclude = lintargs.get('exclude', [])
+    exclude.extend(linter.get('exclude', []))
+    if not lintargs.get('use_filters', True) or (not include and not exclude):
         return paths
     include = map(FilterPath, include or [])
     exclude = map(FilterPath, exclude or [])
     # Paths with and without globs will be handled separately,
     # pull them apart now.
     includepaths = [p for p in include if p.exists]
     excludepaths = [p for p in exclude if p.exists]
     includeglobs = [p for p in include if not p.exists]
     excludeglobs = [p for p in exclude if not p.exists]
+    extensions = linter.get('extensions')
     keep = set()
     discard = set()
     for path in map(FilterPath, paths):
+        # Exclude bad file extensions
+        if extensions and path.isfile and path.ext not in extensions:
+            continue
+        if path.match(excludeglobs):
+            continue
         # First handle include/exclude directives
         # that exist (i.e don't have globs)
         for inc in includepaths:
             # Only excludes that are subdirectories of the include
             # path matter.
             excs = [e for e in excludepaths if inc.contains(e)]
             if path.contains(inc):
@@ -116,20 +130,21 @@ def filterpaths(paths, include=None, exc
         # Next handle include/exclude directives that
         # contain globs.
         if path.isfile:
             # If the specified path is a file it must be both
             # matched by an include directive and not matched
             # by an exclude directive.
             if not path.match(includeglobs):
-            elif path.match(excludeglobs):
-                continue
         elif path.isdir:
             # If the specified path is a directory, use a
             # FileFinder to resolve all relevant globs.
-            path.exclude = excludeglobs
+            path.exclude = [e.path for e in excludeglobs]
             for pattern in includeglobs:
                 for p, f in path.finder.find(pattern.path):
-    return ([f.path for f in keep], [f.path for f in discard])
+    # Only pass paths we couldn't exclude here to the underlying linter
+    lintargs['exclude'] = [f.path for f in discard]
+    return [f.path for f in keep]
--- a/python/mozlint/mozlint/types.py
+++ b/python/mozlint/mozlint/types.py
@@ -20,27 +20,21 @@ class BaseType(object):
         """Run `linter` against `paths` with `lintargs`.
         :param paths: Paths to lint. Can be a file or directory.
         :param linter: Linter definition paths are being linted against.
         :param lintargs: External arguments to the linter not defined in
                          the definition, but passed in by a consumer.
         :returns: A list of :class:`~result.ResultContainer` objects.
-        exclude = lintargs.get('exclude', [])
-        exclude.extend(linter.get('exclude', []))
-        if lintargs.get('use_filters', True):
-            paths, exclude = filterpaths(paths, linter.get('include'), exclude)
+        paths = filterpaths(paths, linter, **lintargs)
         if not paths:
             print("{}: no files to lint in specified paths".format(linter['name']))
-        lintargs['exclude'] = exclude
         if self.batch:
             return self._lint(paths, linter, **lintargs)
         errors = []
             for p in paths:
                 result = self._lint(p, linter, **lintargs)
                 if result:
new file mode 100644
--- /dev/null
+++ b/python/mozlint/test/files/foobar.py
@@ -0,0 +1,2 @@
+# Oh no.. we called this variable foobar, bad!
+foobar = "a string"
--- a/python/mozlint/test/linters/external.lint
+++ b/python/mozlint/test/linters/external.lint
@@ -17,14 +17,14 @@ def lint(files, **lintargs):
                         LINTER, path=path, lineno=i+1, column=1, rule="no-foobar"))
     return results
     'name': "ExternalLinter",
     'description': "It's bad to have the string foobar in js files.",
     'include': [
-        '**/*.js',
-        '**/*.jsm',
+        'python/mozlint/test/files',
     'type': 'external',
+    'extensions': ['.js', '.jsm'],
     'payload': lint,
--- a/python/mozlint/test/test_roller.py
+++ b/python/mozlint/test/test_roller.py
@@ -15,18 +15,18 @@ from mozlint.errors import LintersNotCon
 here = os.path.abspath(os.path.dirname(__file__))
 class TestLintRoller(TestCase):
     def __init__(self, *args, **kwargs):
         TestCase.__init__(self, *args, **kwargs)
-        filedir = os.path.join(here, 'files')
-        self.files = [os.path.join(filedir, f) for f in os.listdir(filedir)]
+        self.filedir = os.path.join(here, 'files')
+        self.files = [os.path.join(self.filedir, f) for f in os.listdir(self.filedir)]
         self.lintdir = os.path.join(here, 'linters')
         names = ('string.lint', 'regex.lint', 'external.lint')
         self.linters = [os.path.join(self.lintdir, n) for n in names]
     def setUp(self):
         self.lint = LintRoller()
@@ -65,11 +65,16 @@ class TestLintRoller(TestCase):
     def test_roll_with_excluded_path(self):
         self.lint.lintargs = {'exclude': ['**/foobar.js']}
         result = self.lint.roll(self.files)
         self.assertEqual(len(result), 0)
+    def test_roll_with_invalid_extension(self):
+        self.lint.read(os.path.join(self.lintdir, 'external.lint'))
+        result = self.lint.roll(os.path.join(self.filedir, 'foobar.py'))
+        self.assertEqual(len(result), 0)
 if __name__ == '__main__':
--- a/python/mozlint/test/test_types.py
+++ b/python/mozlint/test/test_types.py
@@ -66,13 +66,13 @@ class TestLinterTypes(TestCase):
     def test_no_filter(self):
         self.lint.read(os.path.join(self.lintdir, 'explicit_path.lint'))
         result = self.lint.roll(self.files)
         self.assertEqual(len(result), 0)
         self.lint.lintargs['use_filters'] = False
         result = self.lint.roll(self.files)
-        self.assertEqual(len(result), 1)
+        self.assertEqual(len(result), 2)
 if __name__ == '__main__':
--- a/tools/lint/docs/create.rst
+++ b/tools/lint/docs/create.rst
@@ -48,16 +48,17 @@ Each ``.lint`` file must have a variable
 linter. Here are the supported keys:
 * name - The name of the linter (required)
 * description - A brief description of the linter's purpose (required)
 * type - One of 'string', 'regex' or 'external' (required)
 * payload - The actual linting logic, depends on the type (required)
 * include - A list of glob patterns that must be matched (optional)
 * exclude - A list of glob patterns that must not be matched (optional)
+* extensions - A list of file extensions to be considered (optional)
 * setup - A function that sets up external dependencies (optional)
 In addition to the above, some ``.lint`` files correspond to a single lint rule. For these, the
 following additional keys may be specified:
 * message - A string to print on infraction (optional)
 * hint - A string with a clue on how to fix the infraction (optional)
 * rule - An id string for the lint rule (optional)
--- a/tools/lint/flake8.lint
+++ b/tools/lint/flake8.lint
@@ -40,16 +40,17 @@ LINE_OFFSETS = {
     'E302': (-2, 3),
 """Maps a flake8 error to a lineoffset tuple.
 The offset is of the form (lineno_offset, num_lines) and is passed
 to the lineoffset property of `ResultContainer`.
+EXTENSIONS = ['.py', '.lint']
 results = []
 def process_line(line):
         res = json.loads(line)
     except ValueError:
@@ -92,31 +93,35 @@ def lint(files, **lintargs):
         return []
     cmdargs = [
         '--format', '{"path":"%(path)s","lineno":%(row)s,'
-    exclude = lintargs.get('exclude')
-    if exclude:
-        cmdargs += ['--exclude', ','.join(lintargs['exclude'])]
     # Run any paths with a .flake8 file in the directory separately so
     # it gets picked up. This means only .flake8 files that live in
     # directories that are explicitly included will be considered.
     # See bug 1277851
     no_config = []
     for f in files:
         if not os.path.isfile(os.path.join(f, '.flake8')):
+    # XXX For some reason passing in --exclude results in flake8 not using
+    # the local .flake8 file. So for now only pass in --exclude if there
+    # is no local config.
+    exclude = lintargs.get('exclude')
+    if exclude:
+        cmdargs += ['--exclude', ','.join(lintargs['exclude'])]
     if no_config:
     return results
     'name': "flake8",
@@ -124,11 +129,12 @@ LINTER = {
     'include': [
     'exclude': [],
+    'extensions': EXTENSIONS,
     'type': 'external',
     'payload': lint,