Skip to content

Commit

Permalink
linter suppression comments & strict mode
Browse files Browse the repository at this point in the history
  • Loading branch information
@mlin
mlin committed Apr 11, 2020
1 parent ca6e11f commit c675ce4
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 47 deletions.
53 changes: 41 additions & 12 deletions WDL/CLI.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import docker
from shlex import quote as shellquote
from datetime import datetime
from argparse import ArgumentParser, Action, SUPPRESS
from argparse import ArgumentParser, Action, SUPPRESS, RawDescriptionHelpFormatter
import importlib_metadata
from . import *
from ._util import (
Expand Down Expand Up @@ -135,42 +135,66 @@ def fill_common(subparser, path=True):

def fill_check_subparser(subparsers):
check_parser = subparsers.add_parser(
"check", help="Load and typecheck a WDL document; show an outline with lint warnings"
"check",
description="Load and typecheck a WDL document, showing an outline with lint warnings.\n\n"
"Individual lint warnings can be suppressed by a WDL comment containing !WarningName on the\n"
"same line or the following line, e.g.:\n"
" Int? foo = 42 # !UnnecessaryQuantifier\n"
" Int bar = foo + 1\n"
" # Lorem ipsum dolor sit (!OptionalCoercion)\n",
formatter_class=RawDescriptionHelpFormatter,
)
check_parser.add_argument(
"uri", metavar="URI", type=str, nargs="+", help="WDL document filename/URI"
)
check_parser.add_argument(
"--strict",
action="store_true",
help="exit with nonzero status code if any lint warnings are shown (in addition to syntax and type errors)",
)
check_parser.add_argument(
"--no-suppress",
dest="show_all",
action="store_true",
help="show lint warnings even if they have suppression comments",
)
check_parser.add_argument(
"--no-shellcheck",
dest="shellcheck",
action="store_false",
help="don't use shellcheck on task commands even if available, and suppress message if it isn't",
help="don't use shellcheck on task commands even if available, and suppress suggestion if it isn't",
)
return check_parser


def check(uri=None, path=None, check_quant=True, shellcheck=True, **kwargs):
def check(
uri=None, path=None, check_quant=True, shellcheck=True, strict=False, show_all=False, **kwargs
):
# Load the document (read, parse, and typecheck)
if not shellcheck:
Lint._shellcheck_available = False

shown = [0]
for uri1 in uri or []:
doc = load(uri1, path or [], check_quant=check_quant, read_source=read_source)

Lint.lint(doc)

# Print an outline
print(os.path.basename(uri1))
outline(doc, 0, show_called=(doc.workflow is not None))
outline(doc, 0, show_called=(doc.workflow is not None), show_all=show_all, shown=shown)

if shellcheck and Lint._shellcheck_available == False:
print(
"* Hint: install shellcheck (www.shellcheck.net) to check task commands. (--no-shellcheck suppresses this message)",
"* Suggestion: install shellcheck (www.shellcheck.net) to check task commands. (--no-shellcheck suppresses this message)",
file=sys.stderr,
)

if strict and shown[0]:
sys.exit(2)


def outline(obj, level, file=sys.stdout, show_called=True):
def outline(obj, level, file=sys.stdout, show_called=True, show_all=False, shown=None):
# recursively pretty-print a brief outline of the workflow
s = "".join(" " for i in range(level * 4))

Expand All @@ -179,18 +203,23 @@ def outline(obj, level, file=sys.stdout, show_called=True):
def descend(dobj=None, first_descent=first_descent):
# show lint for the node just prior to first descent beneath it
if not first_descent and hasattr(obj, "lint"):
for (pos, klass, msg) in sorted(obj.lint, key=lambda t: t[0]):
print(
"{} (Ln {}, Col {}) {}, {}".format(s, pos.line, pos.column, klass, msg),
file=file,
)
for (pos, klass, msg, suppressed) in sorted(obj.lint, key=lambda t: t[0]):
if show_all or not suppressed:
print(
f"{s} (Ln {pos.line}, Col {pos.column}) {klass}{' (suppressed)' if suppressed else ''}, {msg}",
file=file,
)
if shown:
shown[0] += 1
first_descent.append(False)
if dobj:
outline(
dobj,
level + (1 if not isinstance(dobj, Decl) else 0),
file=file,
show_called=show_called,
show_all=show_all,
shown=shown,
)

# document
Expand Down
56 changes: 41 additions & 15 deletions WDL/Lint.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,20 @@
Given a ``doc: WDL.Document``, the lint warnings can be retrieved like so::

lint = WDL.Lint.collect(WDL.Lint.lint(doc, descend_imports=False))
for (pos, lint_class, message) in lint:
for (pos, lint_class, message, suppressed) in lint:
assert isinstance(pos, WDL.SourcePosition)
assert isinstance(lint_class, str) and isinstance(message, str)
print(json.dumps({
"uri" : pos.uri,
"abspath" : pos.abspath,
"line" : pos.line,
"end_line" : pos.end_line,
"column" : pos.column,
"end_column" : pos.end_column,
"lint" : lint_class,
"message" : message,
}))
if not suppressed:
print(json.dumps({
"uri" : pos.uri,
"abspath" : pos.abspath,
"line" : pos.line,
"end_line" : pos.end_line,
"column" : pos.column,
"end_column" : pos.end_column,
"lint" : lint_class,
"message" : message,
}))

The ``descend_imports`` flag controls whether lint warnings are generated for imported documents
recursively (true, default), or otherwise only the given document (false).
Expand Down Expand Up @@ -46,7 +47,9 @@ class Linter(Walker.Base):
def __init__(self, auto_descend: bool = True, descend_imports: bool = True):
super().__init__(auto_descend=auto_descend, descend_imports=descend_imports)

def add(self, obj: Error.SourceNode, message: str, pos: Optional[Error.SourcePosition] = None):
def add(
self, obj: Error.SourceNode, message: str, pos: Optional[Error.SourcePosition] = None
) -> bool:
"""
Used by subclasses to attach lint to a node.

Expand All @@ -57,9 +60,31 @@ def add(self, obj: Error.SourceNode, message: str, pos: Optional[Error.SourcePos
assert not isinstance(obj, Expr.Base)
if pos is None:
pos = obj.pos

# check for suppressive comments
suppress = False
doc = obj
while not isinstance(doc, Tree.Document):
doc = getattr(doc, "parent")
assert isinstance(doc, Tree.Document)
for L in [pos.line, pos.end_line]:
# check the current line
comment = doc.source_comments[L - 1]
if comment and ("!" + self.__class__.__name__) in comment.text:
suppress = True
# check the following line if it has nothing but a comment
comment = doc.source_comments[L] if L < len(doc.source_comments) else None
if (
comment
and ("!" + self.__class__.__name__) in comment.text
and comment.text.strip() == doc.source_lines[L].strip()
):
suppress = True

if not hasattr(obj, "lint"):
obj.lint = []
obj.lint.append((pos, self.__class__.__name__, message))
obj.lint.append((pos, self.__class__.__name__, message, suppress)) # pyre-ignore
return True


_all_linters = []
Expand Down Expand Up @@ -112,7 +137,7 @@ def __call__(self, obj, descend: Optional[bool] = None):
def collect(doc):
"""
Recursively traverse the already-linted document and collect a flat list of
(tree node, linter_class, message)
(SourcePosition, linter_class, message, suppressed)
"""
collector = _Collector()
collector(doc)
Expand Down Expand Up @@ -606,7 +631,7 @@ def document(self, obj: Tree.Document) -> Any:
msg = "imported document namespace '{}' collides with {}struct type".format(
imp.namespace, "imported " if stb.value.imported else ""
)
self.add(obj, msg)
self.add(obj, msg, imp.pos)


@a_linter
Expand All @@ -622,6 +647,7 @@ def document(self, obj: Tree.Document) -> Any:
obj,
"no use of workflow, tasks, or structs defined in the imported document "
+ imp.namespace,
pos=imp.pos,
)


Expand Down
6 changes: 4 additions & 2 deletions test_corpi/contrived/contrived.wdl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
version 1.0

import "empty.wdl" as popular
import "empty.wdl" as contrived
import "empty.wdl" as contrived # !UnusedImport
# !NameCollision

struct contrived {
}
Expand All @@ -12,7 +13,7 @@ struct popular {

workflow contrived {
input {
String popular = "fox"
String popular = "fox" # !NameCollision
Int? fortytwo = 42
Float required
}
Expand All @@ -38,6 +39,7 @@ workflow contrived {
task popular {
input {
String popular
# Lorem ipsum dolor sit (!NameCollision)
String? opt
Float? i
Array[String]+ y = select_all(["${popular + i}"])
Expand Down
13 changes: 9 additions & 4 deletions tests/check.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ source tests/bash-tap/bash-tap-bootstrap
export PYTHONPATH="$SOURCE_DIR:$PYTHONPATH"
miniwdl="python3 -m WDL"

plan tests 37
plan tests 40

DN=$(mktemp -d --tmpdir miniwdl_check_tests_XXXXXX)
cd $DN
Expand Down Expand Up @@ -93,8 +93,9 @@ is "$(grep ' ^^^^^' trivial_type_error.err | wc -l)" "1" "trivial
cat << EOF > multi_error.wdl
task t {
Int? x
Int y = x
Int y = x # !OptionalCoercion
Array[Int] z = [x]
# Lorem ipsum dolor sit (!UnusedDeclaration)
command {}
}
EOF
Expand All @@ -107,8 +108,12 @@ is "$(grep ' ^' import_multi_error.err | wc -l)" "2" "import_mult
is "$(grep ' ^^^' import_multi_error.err | wc -l)" "1" "import_multi_error.wdl stderr marker 2"
$miniwdl check --no-shellcheck --no-quant-check import_multi_error.wdl > import_multi_error.no_quant_check.out
is "$?" "0" "import_multi_error.wdl --no-quant-check"
is "$(grep OptionalCoercion import_multi_error.no_quant_check.out | wc -l)" "2" "import_multi_error.wdl --no-quant-check OptionalCoercion"
is "$(grep UnusedDeclaration import_multi_error.no_quant_check.out | wc -l)" "2" "import_multi_error.wdl --no-quant-check UnusedDeclaration"
is "$(grep OptionalCoercion import_multi_error.no_quant_check.out | wc -l)" "1" "import_multi_error.wdl --no-quant-check OptionalCoercion"
is "$(grep UnusedDeclaration import_multi_error.no_quant_check.out | wc -l)" "1" "import_multi_error.wdl --no-quant-check UnusedDeclaration"
$miniwdl check --no-shellcheck --no-quant-check --strict --no-suppress import_multi_error.wdl > import_multi_error.no_quant_check.strict_all.out
is "$?" "2" "import_multi_error.wdl --no-quant-check --strict --no-suppress"
is "$(grep OptionalCoercion import_multi_error.no_quant_check.strict_all.out | wc -l)" "2" "import_multi_error.wdl --no-quant-check --strict --no-suppress OptionalCoercion"
is "$(grep UnusedDeclaration import_multi_error.no_quant_check.strict_all.out | wc -l)" "2" "import_multi_error.wdl --no-quant-check --strict --no-suppress UnusedDeclaration"

$miniwdl check --no-shellcheck $SOURCE_DIR/test_corpi/DataBiosphere/topmed-workflows/CRAM-no-header-md5sum/CRAM_md5sum_checker_wrapper.wdl > import_uri.out 2> import_uri.err
is "$?" "0" "URI import"
Expand Down
31 changes: 17 additions & 14 deletions tests/test_3corpi.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,20 @@ def test_api(self):
)
WDL.Lint._shellcheck_available = False
lint = WDL.Lint.collect(WDL.Lint.lint(doc, descend_imports=False))
for (pos, lint_class, message) in lint:
for (pos, lint_class, message, suppressed) in lint:
assert isinstance(pos, WDL.SourcePosition)
assert isinstance(lint_class, str) and isinstance(message, str)
print(json.dumps({
"uri" : pos.uri,
"abspath" : pos.abspath,
"line" : pos.line,
"end_line" : pos.end_line,
"column" : pos.column,
"end_column" : pos.end_column,
"lint" : lint_class,
"message" : message,
}))
if not suppressed:
print(json.dumps({
"uri" : pos.uri,
"abspath" : pos.abspath,
"line" : pos.line,
"end_line" : pos.end_line,
"column" : pos.column,
"end_column" : pos.end_column,
"lint" : lint_class,
"message" : message,
}))
self.assertEqual(len(lint), 1)

async def read_source(uri, path, importer_uri):
Expand Down Expand Up @@ -69,8 +70,10 @@ def t(self, fn=fn):
print(exn.node.pos)
raise
WDL.Lint.lint(doc)
for _, linter, _ in WDL.Lint.collect(doc):
for _, linter, _, suppressed in WDL.Lint.collect(doc):
test_klass._lint_count[linter] = 1 + test_klass._lint_count.get(linter, 0)
if suppressed:
test_klass._lint_count["_suppressions"] = 1 + test_klass._lint_count.get("_suppressions", 0)
print("\n" + os.path.basename(fn))
WDL.CLI.outline(doc, 0, show_called=(doc.workflow is not None))
WDL.copy_source(doc, tempfile.mkdtemp(prefix=f"miniwdl_test_copy_source_{prefix}"))
Expand Down Expand Up @@ -248,15 +251,15 @@ class dxWDL(unittest.TestCase):

@wdl_corpus(
["test_corpi/contrived/**"],
expected_lint={'UnusedImport': 4, 'NameCollision': 27, 'StringCoercion': 4, 'FileCoercion': 2, 'NonemptyCoercion': 1, 'UnnecessaryQuantifier': 2, 'UnusedDeclaration': 2, "IncompleteCall": 2, 'SelectArray': 1},
expected_lint={'_suppressions': 8, 'UnusedImport': 4, 'NameCollision': 27, 'StringCoercion': 4, 'FileCoercion': 2, 'NonemptyCoercion': 1, 'UnnecessaryQuantifier': 2, 'UnusedDeclaration': 2, "IncompleteCall": 2, 'SelectArray': 1},
blacklist=["check_quant", "incomplete_call"],
)
class Contrived(unittest.TestCase):
pass

@wdl_corpus(
["test_corpi/contrived/**"],
expected_lint={'UnusedImport': 6, 'NameCollision': 43, 'StringCoercion': 9, 'FileCoercion': 4, 'OptionalCoercion': 3, 'NonemptyCoercion': 2, 'UnnecessaryQuantifier': 4, 'UnusedDeclaration': 9, 'IncompleteCall': 3, 'ArrayCoercion': 2, 'SelectArray': 4},
expected_lint={'_suppressions': 16, 'UnusedImport': 6, 'NameCollision': 43, 'StringCoercion': 9, 'FileCoercion': 4, 'OptionalCoercion': 3, 'NonemptyCoercion': 2, 'UnnecessaryQuantifier': 4, 'UnusedDeclaration': 9, 'IncompleteCall': 3, 'ArrayCoercion': 2, 'SelectArray': 4},
check_quant=False,
blacklist=["incomplete_call"],
)
Expand Down

0 comments on commit c675ce4

Please sign in to comment.