fix(code-health): profile regex rules match executable code, not prose
F21 (andrexibiza): PS-P05/PS-P06 ran their regexes over raw source lines, so a
comment such as `# Avoid env = os.environ.copy(); use the scoped builder.`, the same
sentence in a docstring, or a comment warning against `os.getenv("DISCORD_TOKEN")`
produced a blocking hit although nothing builds an env or reads a credential.
Before the regexes run, comment tokens (tokenize) and string statements (ast:
docstrings and bare string examples) are blanked in place, preserving every line
number and line break. Ordinary string literals inside code are kept, so the
env-key argument of `os.getenv("DISCORD_TOKEN")` still matches; the real
`env = os.environ.copy()` / `dict(os.environ)` / platform getenv stay flagged. If
tokenizing fails, the raw lines are used (more hits, never fewer).
This commit is contained in:
@@ -57,6 +57,55 @@ def _python_comments(text: str) -> dict[int, str]:
|
||||
return comments
|
||||
|
||||
|
||||
# Characters str.splitlines() breaks on; never blanked, so the blanked text keeps its line numbers.
|
||||
_LINE_BREAKS = frozenset("\n\r\x0b\x0c\x1c\x1d\x1e\x85\u2028\u2029")
|
||||
|
||||
|
||||
def _string_statements(tree: ast.Module):
|
||||
"""Docstrings and other bare string statements: prose, never executed."""
|
||||
for node in ast.walk(tree):
|
||||
if (isinstance(node, ast.Expr) and isinstance(node.value, ast.Constant)
|
||||
and isinstance(node.value.value, str)):
|
||||
yield node
|
||||
|
||||
|
||||
def _executable_lines(text: str, tree: ast.Module) -> list[str]:
|
||||
"""``text.splitlines()`` with comments and string statements blanked out.
|
||||
|
||||
The profile regex rules describe operations (`env = os.environ.copy()`), so a comment or a
|
||||
docstring that mentions one must not count as doing it. String literals inside code stay:
|
||||
`os.getenv("DISCORD_TOKEN")` is matched by its argument.
|
||||
"""
|
||||
rows = [list(line) for line in io.StringIO(text).readlines()] # ast/tokenize line numbering
|
||||
|
||||
def blank(line: int, start: int, end: int | None = None) -> None:
|
||||
row = rows[line - 1]
|
||||
for col in range(start, len(row) if end is None else end):
|
||||
if row[col] not in _LINE_BREAKS:
|
||||
row[col] = " "
|
||||
|
||||
def char_col(line: int, byte_col: int) -> int: # ast columns are UTF-8 byte offsets
|
||||
return len("".join(rows[line - 1]).encode("utf-8")[:byte_col].decode("utf-8", "ignore"))
|
||||
|
||||
try:
|
||||
for tok in tokenize.generate_tokens(io.StringIO(text).readline):
|
||||
if tok.type == tokenize.COMMENT:
|
||||
blank(tok.start[0], tok.start[1], tok.end[1])
|
||||
except (tokenize.TokenError, SyntaxError):
|
||||
return text.splitlines() # fail closed: every line stays visible to the rules
|
||||
for node in _string_statements(tree):
|
||||
last = node.end_lineno or node.lineno
|
||||
end = char_col(last, node.end_col_offset or 0)
|
||||
if last == node.lineno:
|
||||
blank(node.lineno, char_col(node.lineno, node.col_offset), end)
|
||||
continue
|
||||
blank(node.lineno, char_col(node.lineno, node.col_offset))
|
||||
for line in range(node.lineno + 1, last):
|
||||
blank(line, 0)
|
||||
blank(last, 0, end)
|
||||
return "".join("".join(row) for row in rows).splitlines()
|
||||
|
||||
|
||||
class Measurer:
|
||||
def __init__(self, repo: Path, ruff: list[str], known_env: set[str]) -> None:
|
||||
self.repo = repo
|
||||
@@ -116,15 +165,18 @@ class Measurer:
|
||||
if rule_applies(RULES_BY_ID[rule_id], fm.path):
|
||||
for row in sorted(set(checker(rules_tree, self.ctx))):
|
||||
fm.add_hit(rule_id, scopes.scope(row), row)
|
||||
self._regex(fm, scopes)
|
||||
self._regex(fm, scopes, text, tree)
|
||||
|
||||
def _regex(self, fm: FileMeasure, scopes) -> None:
|
||||
def _regex(self, fm: FileMeasure, scopes, text: str, tree: ast.Module) -> None:
|
||||
code_lines: list[str] | None = None
|
||||
for rule_id, pattern, path_re in self.regex_rules:
|
||||
if not rule_applies(RULES_BY_ID[rule_id], fm.path):
|
||||
continue
|
||||
if path_re and not path_re.search(fm.path):
|
||||
continue
|
||||
for index, line in enumerate(fm.lines, start=1):
|
||||
if code_lines is None:
|
||||
code_lines = _executable_lines(text, tree)
|
||||
for index, line in enumerate(code_lines, start=1):
|
||||
if pattern.search(line):
|
||||
fm.add_hit(rule_id, scopes.scope(index), index)
|
||||
|
||||
|
||||
@@ -153,6 +153,33 @@ def test_json_stdout_is_json_on_every_path(tmp_path, capsys, case, switch, files
|
||||
assert "code health:" in err # the human explanation moves to stderr
|
||||
|
||||
|
||||
# --- F21: profile regex rules see executable code, not prose -----------------------------------
|
||||
|
||||
|
||||
_FUNC = "import os\n\n\ndef build(cmd):\n{} return cmd\n"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("body, blocks", [
|
||||
(" # Avoid env = os.environ.copy(); use the scoped builder.\n", False),
|
||||
(' """Avoid env = os.environ.copy(); use the scoped builder."""\n', False),
|
||||
(' """Builder.\n\n Never env = os.environ.copy() here.\n """\n', False),
|
||||
(" # don't read os.getenv(\"DISCORD_TOKEN\") directly\n", False),
|
||||
(" cmd = [cmd] # not os.environ.get('SLACK_TOKEN')\n", False),
|
||||
(" 'inert example: env = os.environ.copy()'\n", False),
|
||||
# positive controls: the real operations, including a literal env-key argument
|
||||
(" env = os.environ.copy()\n cmd = (cmd, env)\n", True),
|
||||
(" token = os.getenv(\"DISCORD_TOKEN\")\n cmd = (cmd, token)\n", True),
|
||||
(" env = dict(os.environ) # copy it\n cmd = (cmd, env)\n", True),
|
||||
])
|
||||
def test_profile_regex_rules_ignore_comments_and_docstrings(tmp_path, capsys, body, blocks):
|
||||
repo, base = _ratchet_repo(tmp_path)
|
||||
head = _commit(repo, {"pkg/c.py": _FUNC.format(body)})
|
||||
code = cli.run(repo, base, head)
|
||||
out = capsys.readouterr().out
|
||||
assert code == (1 if blocks else 0), out
|
||||
assert ("PS-P05" in out or "PS-P06" in out) == blocks, out
|
||||
|
||||
|
||||
# --- allow comments may list several rules ----------------------------------------------------
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user