#!/usr/bin/env python3 """Semantic checking: parse the generated code instead of pattern-matching it. WHY THIS EXISTS --------------- A violation_pattern is a regex over generated code. It catches textual signatures, never meaning. Two of the corpus's critical rules were recorded in NO_PATTERN_REASON as unreachable for exactly that reason: odoo.constrains-must-list-every-field-read odoo.stored-compute-needs-complete-depends Both need the same thing: a set comparison between what a decorator declares and what the method body actually reads. That is a tree walk, not a regex, and Python's ast module makes it tractable without leaving the standard library. WHAT A CHECKER OWES ------------------- The same contract a violation_pattern owes, enforced by corpus.py validate: it must fire on its rule's own 'wrong' sample and stay silent on its 'correct' sample. A checker that catches nothing is useless; one that fires on good code trains you to ignore it, which is worse. HONEST SCOPE ------------ Python, and Odoo's XML data files, which expat reads with line numbers. There is no JavaScript or TypeScript parser in the standard library, so the TS/JS side gets balance_js -- a delimiter-balance scanner that is string-, comment- and template-aware. That is a syntax smoke test, not a parser, and the layer above reports UNKNOWN rather than PASS for everything it cannot see. Stdlib only. """ import ast import builtins import collections import importlib.util import re import textwrap import xml.parsers.expat import context CHECKER_VERSION = "1.0.0" # Attributes reached off a recordset that are ORM plumbing rather than fields. # Deliberately short: every name added here is a field a checker will stop # noticing, so the list earns entries only for names that can never be one. ORM_ATTRS = frozenset([ "env", "ids", "id", "pool", "sudo", "with_context", "with_user", "browse", "_name", "_table", "_cr", "_uid", "_context", "_origin", "_fields", "_description", "_inherit", "_inherits", "_order", "_rec_name", ]) # ------------------------------------------------------------------ parsing def parse_python(code): """Parse, then compile. Returns (tree, error) with at most one error set. Both steps run because they fail on different things. ast.parse rejects bad grammar; compile additionally rejects constructs that parse but cannot be assembled -- return outside a function, await outside async def, duplicate parameter names. Generated code hits the second class more often than the first, because a model emits a plausible fragment lifted out of the context that would have made it legal. """ try: tree = ast.parse(code) except SyntaxError as exc: return None, _syntax_error(exc) try: compile(tree, "", "exec") except (SyntaxError, ValueError, TypeError) as exc: return tree, _syntax_error(exc) return tree, None def _syntax_error(exc): return (getattr(exc, "lineno", None), getattr(exc, "msg", None) or str(exc)) Fragment = collections.namedtuple( "Fragment", "tree error notes source line_offset") # The four labels build_messages asks for. When they appear INSIDE the fence the # block is not broken code, it is code followed by prose, and calling that a # syntax error is the kind of false alarm that gets a checker switched off. _LABEL_RE = re.compile( r"^(Enforcement layer|Rules applied|Edge cases handled|Not handled)\s*:", re.I) def parse_fragment(code): """Parse generated code that may be a fragment lifted out of a class. WHY THIS IS NOT JUST parse_python: an Odoo answer is very often a method shown at the indentation it had inside its model class, and a bare ast.parse calls that 'unexpected indent'. Reporting a correct answer as uncompilable is the worst failure a syntax layer has available -- it is wrong, it is loud, and it masks every layer beneath it. Three repairs are attempted, cheapest first, and each one that works is RECORDED rather than hidden. The note is the finding: 'the labelled sections were inside the code fence' is a real thing to tell the user, it just is not a syntax error. line_offset is how many lines were added in front of the original code, so a caller reporting line numbers can subtract it and stay honest about where in the answer the problem is. """ tree, error = parse_python(code) if tree is not None and error is None: return Fragment(tree, None, [], code, 0) notes = [] candidate = code lines = candidate.splitlines() for index, line in enumerate(lines): if _LABEL_RE.match(line.strip()): candidate = "\n".join(lines[:index]).rstrip() notes.append("the labelled answer sections were inside the code fence") break if notes: tree, error = parse_python(candidate) if tree is not None and error is None: return Fragment(tree, None, notes, candidate, 0) dedented = textwrap.dedent(candidate) if dedented != candidate: tree, error = parse_python(dedented) if tree is not None and error is None: notes.append("the block is indented: a method shown inside its class body") return Fragment(tree, None, notes, dedented, 0) candidate = dedented # Ragged indentation dedent cannot fix -- a method whose decorator sits at # one level and whose body sits at another. Give it a class to live in. wrapped = "class _Fragment:\n" + "\n".join( (" " + line if line.strip() else line) for line in candidate.splitlines()) tree, wrapped_error = parse_python(wrapped) if tree is not None and wrapped_error is None: notes.append("the block only parses as a class body fragment") return Fragment(tree, None, notes, wrapped, 1) return Fragment(None, error or wrapped_error, notes, code, 0) # --------------------------------------------------------- path resolution def _attr_path(node, aliases): """Resolve an attribute chain to a dotted field path relative to self. rec.line_ids.price with rec bound to self yields ('line_ids', 'price'). Returns None when the base is not something known to be self-derived, which is the honest answer for a name the checker never saw bound. A chain may start from records picked by position. rec.line_ids[0].price and rec.line_ids.sorted("sequence")[:1].price both yield ('line_ids', 'price'), because indexing or slicing a recordset gives records of the same path. Stopping at the subscript left the price unread, and a compute that takes its value from the first line looked as if it read nothing from it. A chain may start from a call, too, when the call hands back records of the same path: rec.sudo().partner_id.name reads partner_id.name as surely as rec.partner_id.name does. Which calls those are is _resolve_iterable's list, the one a loop's iterable already goes through, so a loop and a read cannot disagree about it (ARCHITECTURE 3.78). """ segments = [] cursor = node while isinstance(cursor, ast.Attribute): segments.append(cursor.attr) cursor = cursor.value if isinstance(cursor, ast.Subscript) and _indexes_records(cursor): base = _resolve_iterable(cursor.value, aliases) elif isinstance(cursor, ast.Call): base = _resolve_iterable(cursor, aliases) elif isinstance(cursor, ast.Name): base = aliases.get(cursor.id) else: return None if base is None: return None return base + tuple(reversed(segments)) def _indexes_records(subscript): """Does this subscript pick records by position: x[0], x[-1], x[1:]? record[name] is the other thing a subscript on a recordset does, and it reads the field called name, not records of the same path. So a subscript is looked through only when its index is a literal integer or a slice. rec.line_ids[i] could be either, and stays unresolved. """ index = subscript.slice if isinstance(index, ast.Slice): return True if isinstance(index, ast.UnaryOp) and isinstance(index.op, ast.USub): index = index.operand return isinstance(index, ast.Constant) and type(index.value) is int def _aliases(fn): """Names that stand for a recordset, and the path each reaches it by. self is the root. 'for rec in self' makes rec the root as well. 'for line in rec.line_ids' makes line the path ('line_ids',), which is what turns a later line.price into the dependency line_ids.price. Resolved to a fixpoint rather than in source order, because a comprehension can bind a name from a loop variable introduced further down the tree and a single forward pass would miss the second hop. A name assigned records, pending = rec.review_ids.sorted("sequence")[:1], stays unbound, and so does a loop over one. Binding it when every value assigned to it resolves to one path is exact, and was measured: on odoo core the depends check's reads through such names read 37 of 113 true, 28 of them smart defaults, the same ratio as the lambda binding (ARCHITECTURE 3.72 and 3.80). Measured again on the tree facts, which decide 27 of the 113, it still adds more false findings than true (3.87). """ aliases = {"self": ()} for _ in range(6): before = len(aliases) for node in ast.walk(fn): if isinstance(node, (ast.For, ast.AsyncFor, ast.comprehension)): target, source = node.target, node.iter else: continue if not isinstance(target, ast.Name): continue # tuple unpacking: no single recordset to bind path = _resolve_iterable(source, aliases) if path is not None: aliases[target.id] = path if len(aliases) == before: break return aliases def _resolve_iterable(node, aliases): """The recordset path an expression iterates over, if it is one. Unwraps the calls that return a recordset of the same shape, and a subscript by position, which picks records out of one. mapped() is excluded on purpose: it returns values, not records, and treating it as a recordset would invent dependencies that do not exist. """ if isinstance(node, ast.Name): return aliases.get(node.id) if isinstance(node, ast.Attribute): return _attr_path(node, aliases) if isinstance(node, ast.Subscript) and _indexes_records(node): return _resolve_iterable(node.value, aliases) if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute): if node.func.attr in ("filtered", "sorted", "exists", "with_context", "sudo"): return _resolve_iterable(node.func.value, aliases) return None def _called_attrs(fn): """Attribute nodes sitting in a Call's function position. rec.write(...) is a method, not a field. Without this every ORM call would be reported as an undeclared field read, which is the fastest possible way to make a checker worth ignoring. """ return {id(node.func) for node in ast.walk(fn) if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute)} def _message_only_attrs(fn): """Attribute nodes reachable only from inside a raise statement. Found by crosscheck: the checker fired on the 'correct' sample of odoo.constrains-iterate-recordset, which reads rec.display_name to build a ValidationError message. A field read to phrase an error does not decide whether the invariant holds, so it cannot decide when the constraint must re-run either -- demanding it in @api.constrains would be noise. A field read in both the condition and the message is still caught, because only the nodes inside the raise are excluded, not the field name. A message assembled into a variable and raised on the next line is the same read for the same reason, and the inline form was all this covered. That missed the spelling OCA actually uses: msg = self.env._("... %(journal)s ...", journal=journal.display_name) raise ValidationError(msg) which reported display_name as a field the constraint depends on. Two more spellings came out of reading OpenSPP and odoo core on 2026-09-26. A log line is a message too: `_logger.warning("... %s", rec.name)` reads a field to phrase something, and five of OpenSPP's live false positives were exactly that. And a message can be collected before it is raised -- `sold_out.append(_("... %(name)s", name=event.name))`, then `raise ValidationError("\\n".join(sold_out))` -- which reads the same way as the assignment form above. """ excluded = set() raised = set() for node in ast.walk(fn): if isinstance(node, ast.Call) and _is_log_call(node): for inner in ast.walk(node): excluded.add(id(inner)) continue if not isinstance(node, ast.Raise): continue for inner in ast.walk(node): excluded.add(id(inner)) if isinstance(inner, ast.Name): raised.add(inner.id) if raised: for node in ast.walk(fn): if (isinstance(node, ast.Assign) and len(node.targets) == 1 and isinstance(node.targets[0], ast.Name) and node.targets[0].id in raised): for inner in ast.walk(node.value): excluded.add(id(inner)) elif (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr in ("append", "extend") and isinstance(node.func.value, ast.Name) and node.func.value.id in raised): for arg in node.args: for inner in ast.walk(arg): excluded.add(id(inner)) return excluded LOG_METHODS = frozenset(["debug", "info", "warning", "warn", "error", "exception", "critical", "log"]) def _is_log_call(call): """`_logger.warning(...)`, `logger.info(...)`, `logging.error(...)`.""" func = call.func if not (isinstance(func, ast.Attribute) and func.attr in LOG_METHODS): return False owner = func.value name = owner.id if isinstance(owner, ast.Name) else \ owner.attr if isinstance(owner, ast.Attribute) else "" return name == "logging" or name.lower().lstrip("_").endswith(("logger", "log")) def _field_reads(fn, aliases): """Every field path the body READS, as (path, lineno). Store context is excluded: rec.total = ... is the compute writing its result, and counting it would make every stored compute demand a dependency on the field it produces. """ skip = _called_attrs(fn) | _message_only_attrs(fn) reads = [] for node in ast.walk(fn): if not isinstance(node, ast.Attribute) or id(node) in skip: continue if not isinstance(node.ctx, ast.Load): continue path = _attr_path(node, aliases) # An Odoo field name never starts with an underscore -- that space is # reserved for ORM internals and mixin configuration, and a field there # would collide with them. ORM_ATTRS names the specific ones this code # meets; the prefix is the general rule behind the list. Without it, a # mixin reading self._time_window_overlap_check_field was told to add it # to @api.constrains, where the ORM ignores it. if path and path[0] not in ORM_ATTRS and not path[0].startswith("_"): reads.append((path, node.lineno)) return reads def _named_reads(fn, aliases): """Field paths a recordset method names in a string, as (path, lineno). account.invoice_ids.mapped("amount_residual") reads invoice_ids.amount_residual exactly as sum(inv.amount_residual for inv in account.invoice_ids) does, but the only attribute access in it is invoice_ids. Read by attributes alone, the first spelling looked fully declared by @api.depends("invoice_ids") and the second did not, so the checker was silent on the more idiomatic of the two. mapped() was the first of these (ARCHITECTURE 3.66). Its siblings name what they read the same way: filtered("a.b") tests a.b on every record, grouped("f") keys on f, sorted("f") orders on f, and filtered_domain() reads the field of every leaf written out in its domain. _named_paths says which paths each call names. Only literals are followed. mapped(name) stays unread: what a variable holds is a guess from here, and every guess in this checker is a finding somebody has to read. A lambda stays unread too, and that one is not a guess -- its parameter is each record of the receiver, exactly. It was measured instead, and what the lambdas in these trees test is mostly not a dependency (ARCHITECTURE 3.72, and again on the tree facts, 3.87). The receiver resolves the way a loop's iterable does, through filtered(), sorted(), sudo() and a subscript by position. A call inside a raise or a log line is phrasing a message, as an attribute read there is. """ quiet = _message_only_attrs(fn) reads = [] for node in ast.walk(fn): if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute)) \ or id(node) in quiet: continue named = _named_paths(node) if not named: continue base = _resolve_iterable(node.func.value, aliases) if base is None: continue for segments in named: path = base + segments if path[0] not in ORM_ATTRS and not path[0].startswith("_"): reads.append((path, node.lineno)) return reads # Recordset methods whose one argument can be a field path. mapped("a.b") and # filtered("a.b") follow it through the relations; grouped("f") keys on f. PATH_ARGUMENT_METHODS = frozenset(["mapped", "filtered", "grouped"]) def _named_paths(call): """The field paths, relative to its receiver, that a recordset call names.""" method = call.func.attr if method in PATH_ARGUMENT_METHODS: if len(call.args) != 1 or call.keywords: return [] path = _literal_path(call.args[0]) return [path] if path else [] if method == "sorted": keys = list(call.args[:1]) + [k.value for k in call.keywords if k.arg == "key"] if len(keys) == 1 and isinstance(keys[0], ast.Constant) \ and isinstance(keys[0].value, str): return _order_fields(keys[0].value) return [] if method == "filtered_domain": if len(call.args) == 1 and not call.keywords \ and isinstance(call.args[0], (ast.List, ast.Tuple)): return _domain_fields(call.args[0]) return [] def _literal_path(node): """('a', 'b') for the string literal "a.b"; None for anything else.""" if not (isinstance(node, ast.Constant) and isinstance(node.value, str)): return None return _path_segments(node.value) def _path_segments(text): """('a', 'b') for "a.b"; None when the text is not a dotted field path.""" segments = tuple(text.strip().split(".")) return segments if all(segment.isidentifier() for segment in segments) else None ORDER_WORDS = frozenset(["asc", "desc", "nulls", "first", "last"]) def _order_fields(spec): """The field paths an order sorts on: "date desc, id" names date and id. Odoo 18 takes one field name here, as itemgetter(key). Odoo 19 takes an order: comma-separated terms, each a field with an optional .property and :granularity, then asc or desc and nulls first or last (_sorted_order_to_function in odoo/orm/models.py, 19.0). A spec with any other word in it is read not at all rather than in part. """ paths = [] for term in spec.split(","): words = term.split() if not words or any(word.lower() not in ORDER_WORDS for word in words[1:]): return [] path = _path_segments(words[0].split(":")[0]) if path is None: return [] paths.append(path) return paths def _domain_fields(domain): """The field path of every leaf a domain literal writes out. [("state", "=", "done"), ("partner_id.country_id", "in", ids)] names state and partner_id.country_id. A leaf whose operator is any or not any holds a domain on the related records, read through the leaf's own field: ("line_ids", "any", [("paid", "=", True)]) names line_ids and line_ids.paid. "&", "|" and "!" name nothing, and neither does a leaf built at run time. """ paths = [] for leaf in domain.elts: if not (isinstance(leaf, (ast.Tuple, ast.List)) and len(leaf.elts) == 3): continue field, operator, value = leaf.elts path = _literal_path(field) if path is None: continue paths.append(path) if isinstance(operator, ast.Constant) and operator.value in ("any", "not any") \ and isinstance(value, (ast.List, ast.Tuple)): paths.extend(path + inner for inner in _domain_fields(value)) return paths # A decorator that is present and cannot be read statically is neither absent # nor empty. @api.constrains(*FIELDS) used to collapse into "declares nothing", # which made every field the body reads a missing declaration and phrased it # "declares only nothing". UNREADABLE = object() def _note_unreadable(gaps, fn, decorator): """Record a method whose decorator could not be read. Not a finding. Nothing is wrong with @api.constrains(*FIELDS); it is simply outside what a static pass can decide. Reporting it as a violation is a false positive, and reporting nothing at all is the silence this corpus exists to avoid -- so it goes to the gap channel, where `check` prints it and no gate counts it. Once per method, however many checkers meet it. Two of them read @api.constrains, and the same unread method twice is one gap printed twice by `check` and listed twice by fieldtest. """ if gaps is not None: gap = ("{0}() has an @api.{1} whose arguments are not string literals, so " "what it declares could not be read -- that method was not " "checked".format(fn.name, decorator)) if gap not in gaps: gaps.append(gap) def _decorator(fn, module, name): """The @.(...) call decorating fn, or None. One definition of which decorator that is, so a finding anchored on its line and the arguments read from it cannot come from two different decorators. """ for dec in fn.decorator_list: if not isinstance(dec, ast.Call): continue func = dec.func if isinstance(func, ast.Attribute) and func.attr == name: if isinstance(func.value, ast.Name) and func.value.id == module: return dec elif isinstance(func, ast.Name) and func.id == name: return dec return None def _decorator_args(fn, module, name): """String literal arguments of @.(...). Three answers, not two: None when the decorator is absent, UNREADABLE when it is present and its arguments are not all string literals, and the list otherwise. An empty list stays an empty list -- @api.constrains() really does declare nothing, and that is readable. """ dec = _decorator(fn, module, name) if dec is None: return None if any(not (isinstance(a, ast.Constant) and isinstance(a.value, str)) for a in dec.args): return UNREADABLE return [a.value for a in dec.args] def _functions(tree): return [n for n in ast.walk(tree) if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef))] # ------------------------------------------------------------- the checkers def constrains_lists_every_field_read(code, gaps=None): """odoo.constrains-must-list-every-field-read @api.constrains fires only when a listed field appears in the write vals. A constraint that reads date_end while declaring only date_start never runs when date_end alone is written, and the invalid record is stored in silence. Only the first segment of each path is compared, because @api.constrains silently ignores dotted names -- declaring partner_id.country_id registers nothing, so a body reading it needs partner_id declared instead. That asymmetry with @api.depends is its own rule (odoo.constrains-ignores-dotted-names, constrains_declares_a_dotted_name below); this checker only respects it. Reads are the ones the depends check makes: attribute accesses, including one after a subscript by position, and the fields a recordset method names in a string (_named_reads). `if any(self.mapped("date_end"))` under @api.constrains("date_start") was silent. Measured before it was taken: OCA +1, OpenSPP 0, core +3, all read, 3 true. The false one filters on a non-stored compute, which @api.constrains cannot name: a class this rule already had for attribute reads (ARCHITECTURE 3.71). That class is answered now, for a field the constraint's own class declares. Odoo never runs a constraint for a field that is neither stored nor inversible, and warns "@constrains parameter ... is not writeable" when one is named, so telling the author to name it was advice that cannot be taken. What can trigger the check is a write of what the field is computed from: the roots of its compute's @api.depends, or the first field of its related path, followed through any of those that is unwriteable too (_unwriteable_inputs). A constraint that names them all is covered and reported nothing; one that does not is told which of them it is missing. When the inputs cannot be read, the finding says the field cannot be named rather than asking for it (ARCHITECTURE 3.82). """ tree, _ = parse_python(code) if tree is None: return [] findings = [] for fn in _functions(tree): declared = _decorator_args(fn, "api", "constrains") if declared is None: continue if declared is UNREADABLE: _note_unreadable(gaps, fn, "constrains") continue registered = {d.split(".")[0] for d in declared} missing = {} aliases = _aliases(fn) for path, lineno in _field_reads(fn, aliases) + _named_reads(fn, aliases): if path[0] not in registered: missing.setdefault(path[0], lineno) names = ", ".join(sorted(registered)) clause = "declares only {0}".format(names) if names else "declares nothing" owner = _owner_of(tree, fn) declarations = _class_fields(owner) if owner is not None else {} for field, lineno in sorted(missing.items()): if _writeable(declarations.get(field)) is not False: findings.append((lineno, "{0}() reads '{1}' but @api.constrains {2} -- " "writing '{1}' alone skips the check entirely".format( fn.name, field, clause))) continue inputs = _unwriteable_inputs(field, declarations, owner) if inputs is None: findings.append((lineno, "{0}() reads '{1}', which is not stored, so no " "write of '{1}' ever runs the check -- @api.constrains " "{2}, and needs the fields '{1}' is computed from " "instead".format(fn.name, field, clause))) continue lacking = sorted(inputs - registered) if not lacking: continue findings.append((lineno, "{0}() reads '{1}', which is not stored and is computed " "from {2}, but @api.constrains {3} -- writing {4} alone " "skips the check entirely".format( fn.name, field, ", ".join(sorted(inputs)), clause, " or ".join("'{0}'".format(name) for name in lacking)))) return findings def _class_fields(cls): """{field name: {keyword: value node}} for the fields a class body declares. Only `name = fields.Something(...)` statements directly in the body. A field declared in another module, or by a mixin, is not here, and a caller has to read that absence as unknown rather than as anything else. """ return {name: {k.arg: k.value for k in call.keywords if k.arg} for name, call in _class_field_calls(cls).items()} def _class_field_calls(cls): """{field name: the fields.Something(...) call declaring it}, as _class_fields.""" out = {} for statement in cls.body: if not (isinstance(statement, ast.Assign) and len(statement.targets) == 1 and isinstance(statement.targets[0], ast.Name) and isinstance(statement.value, ast.Call)): continue func = statement.value.func if not (isinstance(func, ast.Attribute) and isinstance(func.value, ast.Name) and func.value.id == "fields"): continue out[statement.targets[0].id] = statement.value return out def _writeable(declaration): """Can a write of this field run a constraint naming it? True, False or None. Odoo 18 runs a constraint when one of the names it declares is among the fields written, and a field that is neither stored nor inversible never is: _constraint_methods warns "@constrains parameter ... is not writeable" for it, and that is all (odoo/models.py, 18.0 at 3c3e3b3). From the declaration, per _get_attrs and setup_related in odoo/fields.py at the same commit: compute= not stored unless store=True; inversible with an inverse= related= not stored unless store=True; readonly unless readonly=False, and inversible only when it is not readonly neither a plain field, stored None when the declaration is absent or says it with something other than a literal: unknown, which is never read as not writeable. """ if declaration is None: return None if "compute" not in declaration and "related" not in declaration: return True store = declaration.get("store") if store is not None: if not isinstance(store, ast.Constant): return None if store.value: return True if "compute" in declaration: return "inverse" in declaration readonly = declaration.get("readonly") if readonly is None: return False if not isinstance(readonly, ast.Constant): return None return not readonly.value def _unwriteable_inputs(field, declarations, cls, seen=None): """The writeable fields an unwriteable field is computed from, or None. The roots of its compute's @api.depends, or the first field of its related path: @api.constrains names fields of its own model only, so a dependency through a relation is reached by naming the relation, as a read through one is. A root that is itself unwriteable is followed to what it is computed from. None when any step cannot be read: a compute method not in this class, @api.depends absent or not literal, a related path that is not a string. `id` never changes and is left out. """ seen = set(seen or ()) | {field} declaration = declarations[field] related = declaration.get("related") if related is not None: if not (isinstance(related, ast.Constant) and isinstance(related.value, str)): return None roots = {related.value.split(".")[0]} else: compute = declaration["compute"] if isinstance(compute, ast.Constant) and isinstance(compute.value, str): name = compute.value elif isinstance(compute, ast.Name): name = compute.id else: return None method = next((node for node in cls.body if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name), None) if method is None: return None depends = _decorator_args(method, "api", "depends") if depends is None or depends is UNREADABLE: return None roots = {path.split(".")[0] for path in depends} inputs = set() for root in roots - {"id"}: if root in seen: continue if _writeable(declarations.get(root)) is False: deeper = _unwriteable_inputs(root, declarations, cls, seen) if deeper is None: return None inputs |= deeper else: inputs.add(root) return inputs def constrains_declares_a_dotted_name(code, gaps=None): """odoo.constrains-ignores-dotted-names @api.constrains registers plain field names. A dotted one such as partner_id.country_id is accepted and the module installs, but it can never be among the written fields _validate_fields compares the declaration with, so the ORM never runs the check for it. Odoo 18 logs "@constrains parameter ... is not a field name" the first time the model collects its constraints, and that is all (odoo/models.py, 18.0 at 3c3e3b3). Was a regex for a quoted name of two segments in lowercase letters and underscores. partner_id.country_id.code has three, so where the regex wanted the closing quote there was a dot; and the class had no digits, so a name through any l10n_ field was invisible to it. _decorator_args reads the arguments exactly, and a computed list -- @api.constrains(*FIELDS), or the callable form Odoo also accepts -- goes to the gap channel rather than being guessed at. One finding per method, on the decorator line, where the names are and where a marker goes. The message says what is left to trigger the check: nothing when every name is dotted, and only the plain names when some are. 0 dotted names in 815 @api.constrains across odoo core, OCA and OpenSPP, so this moves nothing measured there. It is for code those trees do not hold, and for generated answers, where @api.depends beside it takes the same dotted name and means it. ARCHITECTURE 3.70. """ tree, _ = parse_python(code) if tree is None: return [] findings = [] for fn in _functions(tree): declared = _decorator_args(fn, "api", "constrains") if declared is None: continue if declared is UNREADABLE: _note_unreadable(gaps, fn, "constrains") continue dotted = [name for name in declared if "." in name] if not dotted: continue plain = [name for name in declared if "." not in name] effect = ("the ORM runs the check only when {0} is written".format( " or ".join(plain)) if plain else "the ORM never runs the check") findings.append(( _decorator(fn, "api", "constrains").lineno, "{0}() declares {1} in @api.constrains, which ignores dotted names " "-- {2}".format(fn.name, ", ".join("'{0}'".format(name) for name in dotted), effect))) return findings def _declared_covers(read, declared): """Is a field read already covered by something @api.depends declares? Exactly, or by a longer path that runs through it. @api.depends('a.b') registers the whole chain, so the ORM recomputes when a changes as well as when a.b does, and a body reading rec.a needs nothing further. Demanding the prefix be spelled out again reported correct code as stale: 42 of this checker's 104 hits on OpenSPP were exactly that, 40% of its output. The converse is the rule itself and must not follow: a declared 'a' does NOT cover a read of a.b, which is the stale value this exists to find. """ return any(d == read or d.startswith(read + ".") for d in declared) # Fields the ORM adds to every model itself, with a declaration this corpus # can read and a default of NOT stored. A model that wants one of these in the # database says so in its own file, and then the ordinary storedness test sees # it. Only display_name is here: the other automatic fields -- id, create_uid, # create_date, write_uid, write_date -- are never computed by module code, so # listing them would be describing a shape nobody writes. FRAMEWORK_UNSTORED = frozenset(["display_name"]) def depends_covers_compute_reads(code, facts=None, gaps=None): """odoo.stored-compute-needs-complete-depends A stored compute recomputes only when a declared dependency changes. A body reading line_ids.price while declaring only line_ids keeps a stale total in the database after a price edit, and every report built on that column is quietly wrong from then on. Full dotted paths are compared, not roots: Odoo needs line_ids.price spelled out, and line_ids alone does not imply it. That asymmetry runs one way only, and reading it as symmetric was this checker's largest source of false positives. A declared line_ids.price DOES cover a body that reads rec.line_ids -- the dependency is on the whole chain, so the ORM already recomputes when the relation itself changes. See _declared_covers. Two things are NOT reported, both learned by running this over 35k lines of OCA code rather than over the corpus's own samples: A path ending in .id. An id cannot change without the relation changing, so a read of journal_id.account_id.id is covered by journal_id.account_id, and one that is not declared is reported under that name. Odoo would take the .id spelling without a word: id is a field of every model, and resolve_depends keeps the relation it ends on and adds a trigger on id, which fires when a related record is deleted (odoo/models.py, odoo/fields.py, 18.0 at 3c3e3b3). Reporting it asked for a longer spelling of what was declared already. 13 of 121 findings on OCA were this. A read of a field this same method COMPUTES. Reading your own output mid-compute is usually a bug, but the fix is never "declare a dependency on it": Odoo drops a field from its own @api.depends without a word. resolve_depends skips it ("do not make self trigger itself", odoo/fields.py, 18.0 at 3c3e3b3), and every recompute and onchange runs off the triggers it builds, so the entry does nothing. Emitting advice that cannot be followed is worse than saying nothing, and there is no rule in the corpus about self-reads for this to become. 48 of 121 findings on OCA were reads of a computed field, most of them the method's own. A path through the output is exempt too, and there the premise is thinner. Odoo skips only the field's own name, so @api.depends("total.x") is kept, and the compute re-runs when x changes on the record total points to. Measured with the exemption narrowed to the bare field (ARCHITECTURE 3.73): OCA and OpenSPP 0, core +37, all read. 17 true, 16 of them a smart default checking its own earlier choice against configuration elsewhere; 20 false, where what total points to cannot change or write() refuses the change. So the exemption stays whole. What a method computes is what it assigns and what the file declares with compute="" (_computed_fields). A helper handed the records and the field name, update() and write() fill a field with no attribute store to see, and that field used to read as a dependency of the method producing it: calendar.recurrence's _compute_rrule on core, and the two OpenSPP findings already classified false as self-read. Core 2,358 -> 2,357, OpenSPP 163 -> 161, nothing added (ARCHITECTURE 3.72). And a third, added after classifying the survivors: a compute whose field is declared in this same file WITHOUT store=True. The rule is about STORED computes -- "written once and never recomputed... survives restarts" -- and none of that happens to a value the database never holds. 30 of 95 findings on OCA were non-stored computes, outside the rule's own scope. That test is deliberately conditional. Most of this checker's work is on generated snippets, where no field declaration is present at all -- the rule's own wrong sample is a bare @api.depends and a method body. So the file's evidence is used when it exists and the rule's assumption stands when it does not: proven-not-stored is dropped, unknown stays a finding. Reads are attribute accesses plus the fields a recordset method names in a string (_named_reads), and every exemption above applies to both. mapped() came first: sum(rec.invoice_ids.mapped("amount_residual")) under a bare @api.depends("invoice_ids") was silent while the same read spelled as a generator was reported. Core 2,336 -> 2,345; all 9 read, 8 true. filtered(), sorted(), grouped() and filtered_domain() followed, with a read after a subscript by position (rec.line_ids[0].amount, _attr_path): OCA +1, OpenSPP +6, core +17, all 24 read, 15 true. The 9 false are classes this checker already had for attribute reads (ARCHITECTURE 3.71). Three answers come from the tree when one was walked, each only where the module giving it is installed with this one (ARCHITECTURE 3.83): A compute whose fields every declaration in a complete walk leaves unstored is outside the rule, as one declared so in this file always was (_unstored_in_tree). Odoo merges @api.depends from every definition of the method in the model's MRO, so a dependency another definition declares is declared (_merge_into_declared). Only from the modules this one depends on: a sibling that overrides the same compute is not always installed. Odoo's triggers are transitive, through stored and unstored fields alike (get_field_trigger_tree, odoo/modules/registry.py at 3c3e3b3). A declared field that is computed or related covers what it depends on in turn (_covered_transitively). """ tree, _ = parse_python(code) if tree is None: return [] storedness = _compute_storedness(tree) computed = _computed_fields(tree) closure = context.module_closure(facts, (facts or {}).get("module")) findings = [] for fn in _functions(tree): declared = _decorator_args(fn, "api", "depends") if declared is None: continue if declared is UNREADABLE: _note_unreadable(gaps, fn, "depends") continue if storedness.get(fn.name) is False: continue owner = _owner_of(tree, fn) if owner is not None and _is_transient(owner): # A wizard's stored compute is written and read inside one # interaction. "Written once and never recomputed, survives # restarts, diverges from reality" is about a row that outlives the # thing it was computed from, and this one does not. continue model = _class_model(owner)[0] if owner is not None else None if storedness.get(fn.name) is None and _unstored_in_tree(facts, model, fn.name): continue registered = set(declared) | _merge_into_declared(facts, closure, model, fn.name) produced = _assigned_attrs(fn) if produced and produced <= FRAMEWORK_UNSTORED and \ storedness.get(fn.name) is None: # A field the framework adds to every model, which this file does # not redeclare. "Unknown storedness" is the right reading for an # ordinary field declared in another module; it is the wrong one # here, because the framework's own declaration is visible and # says not stored. odoo/models.py adds display_name as # `fields.Char(automatic=True, compute='_compute_display_name', # search=...)` with no store, so an override that omits a # dependency cannot leave a stale row: there is no row. # # 121 of the 1051 findings this rule made on odoo core were # _compute_display_name overrides of exactly that shape. The # narrowing costs nothing measurable: all 10 hand-verified true # positives that compute display_name -- every one of them in # OpenSPP -- declare `store=True` on it in their own file, so # storedness reads True for them and they are untouched. continue missing = {} aliases = _aliases(fn) own = produced | computed.get(fn.name, set()) for path, lineno in _field_reads(fn, aliases) + _named_reads(fn, aliases): while len(path) > 1 and path[-1] in ORM_ATTRS: path = path[:-1] if path[0] in own: continue dotted = ".".join(path) if _declared_covers(dotted, registered): continue if not _covered_transitively(facts, closure, model, registered, dotted): missing.setdefault(dotted, lineno) if missing: # `nexa check` reads the modules this one depends on only now, for a # read the module's own declarations leave uncovered; most files never # have one (ARCHITECTURE 3.86). More declarations can only cover more, # so the reads left are the only ones to ask about again. wider = context.with_dependencies(facts) if wider is not None: reach = context.module_closure(wider, wider.get("module")) merged = registered | _merge_into_declared(wider, reach, model, fn.name) missing = {dotted: lineno for dotted, lineno in missing.items() if not _declared_covers(dotted, merged) and not _covered_transitively(wider, reach, model, merged, dotted)} for dotted, lineno in sorted(missing.items()): findings.append((lineno, "{0}() reads '{1}' but @api.depends does not declare it -- " "the stored value goes stale when '{1}' changes".format( fn.name, dotted))) return findings def _class_model(cls): """(model, named, inherited) for a class, read as Odoo reads it. `_name`, or failing that a single `_inherit`. Odoo names a class that inherits several models without a `_name` after the class itself (odoo/models.py, 18.0 at 3c3e3b3); that is no model the tree declares, so it is None here. """ name = inherit = None for node in cls.body: if not (isinstance(node, ast.Assign) and len(node.targets) == 1 and isinstance(node.targets[0], ast.Name)): continue if node.targets[0].id == "_name": name = _literal_string(node.value) elif node.targets[0].id == "_inherit": inherit = _literal_strings(node.value) inherit = inherit or [] return (name or (inherit[0] if len(inherit) == 1 else None), name is not None, inherit) def _lineage(facts, model, closure): """The model and every model it inherits, transitively, by the inheritance the modules in `closure` declare, or every module's when closure is None.""" parents = facts.get("orm_parents") or {} seen, stack = {model}, [model] while stack: for module, parent in parents.get(stack.pop(), ()): if (closure is None or module in closure) and parent not in seen: seen.add(parent) stack.append(parent) return seen def _unstored_in_tree(facts, model, method): """Does every declaration the walk holds leave this compute's fields unstored? Only from a complete walk that names the model and everything it inherits: "nothing stores it" is true only of everything read, and an extension cannot see the declaration that created the field. Adding compute= to a field without store=True makes it unstored, whatever an earlier declaration of it said (_get_attrs, odoo/fields.py at 3c3e3b3), so one store=True anywhere is what keeps it stored. """ if not (facts and facts.get("walked") and facts.get("complete") and model): return False named = facts.get("orm_named") or set() lineage = _lineage(facts, model, None) if any(m not in named for m in lineage): return False declared = facts.get("orm_fields") or {} found = False for field in {f for m in lineage for f, rows in declared.get(m, {}).items() if any(d["compute"] == method for _, d in rows)}: found = True stores = [d["store"] for m in lineage for _, d in declared.get(m, {}).get(field, ())] if any(store is True or store == "?" for store in stores): return False return found def _merged_depends(facts, closure, model, method): """The @api.depends of every definition of `method` in the model's MRO that a module in `closure` declares.""" if not facts or closure is None or not model: return set() methods = facts.get("orm_methods") or {} merged = set() for m in _lineage(facts, model, closure): for module, depends in methods.get(m, {}).get(method, ()): if module in closure and depends: merged.update(depends) return merged def _merge_into_declared(facts, closure, model, method): """What Odoo adds to this method's own @api.depends from its other definitions: the depends check's use of _merged_depends.""" return _merged_depends(facts, closure, model, method) def _covered_transitively(facts, closure, model, declared, read): """Does a declared dependency reach `read` through what the fields on its way depend on? Each step of a declared path is a field of the model the path has reached so far. A computed one depends on its compute's merged @api.depends and a related one on its path, both relative to that model, so a step replaces the rest of the path with the field's own dependency and keeps the prefix that reached it. A relational field moves the next step to its comodel; a related one stops there, since its comodel is not in its declaration. Only declarations from modules in `closure` count. The search goes toward the read. A step keeps its prefix, so a step taken where the path has already left the read's can never come back to it, and is not taken. Every path met is then a prefix of the read followed by one dependency some field declares, a finite set, so the search ends without a limit, and its answer does not depend on the order it looks in. """ if not facts or closure is None or not model: return False target = tuple(read.split(".")) queue = collections.deque(sorted(tuple(path.split(".")) for path in declared)) seen = set(queue) while queue: path = queue.popleft() if path[:len(target)] == target: return True current = model for index in range(min(len(path), len(target))): rows = _declarations(facts, closure, current, path[index]) for row in rows: for dependency in _field_dependencies(facts, closure, current, row): reached = path[:index] + tuple(dependency.split(".")) if reached not in seen: seen.add(reached) queue.append(reached) if path[index] != target[index]: break comodel = next((row["comodel"] for row in rows if row["comodel"]), None) if comodel is None: break current = comodel return False def _declarations(facts, closure, model, field): """Every declaration of `field` on the model's lineage from a module in `closure`, in a fixed order.""" fields = facts.get("orm_fields") or {} return [row for m in sorted(_lineage(facts, model, closure)) for module, row in fields.get(m, {}).get(field, ()) if module in closure] def _field_dependencies(facts, closure, model, row): """What one declaration of a field makes it depend on, relative to its model.""" if row["related"]: return [row["related"]] if row["compute"]: return sorted(_merged_depends(facts, closure, model, row["compute"])) return [] def _test_atoms(fn): """Every condition tested by an if or while in this function, with and/or flattened. `a and (b or c)` yields a, b, c.""" atoms = [] stack = [node.test for node in ast.walk(fn) if isinstance(node, (ast.If, ast.While))] while stack: node = stack.pop() if isinstance(node, ast.BoolOp): stack.extend(node.values) else: atoms.append(node) return atoms def _is_literal_comparison(node): """A comparison of something against a literal, and nothing more. Deliberately narrow: this decides whether a constraint is doing ONLY what a CHECK can do, so anything it does not recognise has to count against the claim, never for it. """ if not isinstance(node, ast.Compare): return False operands = [node.left] + list(node.comparators) return any(isinstance(o, ast.Constant) and not isinstance(o.value, bool) and isinstance(o.value, (int, float, str)) for o in operands) def _compute_storedness(tree): """{compute method name: True|False} from the field declarations in scope. A name absent from this map is unknown, not unstored: the declaration may live in another file, or the code may be a fragment with no declaration at all. Callers must treat absence and False differently. """ stored = {} for node in ast.walk(tree): if not isinstance(node, ast.Call) or not isinstance(node.func, ast.Attribute): continue compute = store = None for keyword in node.keywords: if keyword.arg == "compute" and isinstance(keyword.value, ast.Constant): compute = keyword.value.value elif keyword.arg == "store" and isinstance(keyword.value, ast.Constant): store = keyword.value.value if isinstance(compute, str): # A field extended in two places counts as stored if either stores # it: one declaration that writes to the database is enough for the # value to go stale in it. stored[compute] = bool(store) or bool(stored.get(compute)) return stored def _computed_fields(tree): """{compute method name: the field names this file declares it computes}. `account_type = fields.Selection(compute="_compute_account_type", ...)` maps _compute_account_type to {"account_type"}. That is the method's output whether or not the body assigns it where _assigned_attrs can see: a helper handed the records and the field name, update({...}), write({...}) and rec[name] = ... all fill it without an attribute store. """ out = collections.defaultdict(set) for node in ast.walk(tree): if not (isinstance(node, ast.Assign) and len(node.targets) == 1 and isinstance(node.targets[0], ast.Name) and isinstance(node.value, ast.Call)): continue for keyword in node.value.keywords: if (keyword.arg == "compute" and isinstance(keyword.value, ast.Constant) and isinstance(keyword.value.value, str)): out[keyword.value.value].add(node.targets[0].id) return out def _assigned_attrs(fn): """Attribute names this function writes: rec.total = ... -> {'total'}. These are the compute's outputs. _field_reads already drops the assignment itself, but a body that writes rec.total and elsewhere READS rec.total.name would otherwise be told to depend on the field it is producing. """ names = set() for node in ast.walk(fn): targets = [] if isinstance(node, ast.Assign): targets = node.targets elif isinstance(node, (ast.AugAssign, ast.AnnAssign)): targets = [node.target] for target in targets: for inner in ast.walk(target): if isinstance(inner, ast.Attribute) and isinstance(inner.ctx, ast.Store): names.add(inner.attr) return names _SQL_COLUMNS = re.compile(r"\b(?:UNIQUE|CHECK)\s*\(([^)]*)\)", re.I) def _searches_self(call): """Is this search on THIS model, rather than on another one? `self.search(...)` and `self.sudo().search(...)` are uniqueness on this table, which is what a UNIQUE entry replaces. `self.env["other"].search(...)` is uniqueness across tables, which no constraint on this table can express. """ receiver = call.func.value while isinstance(receiver, ast.Call) and isinstance(receiver.func, ast.Attribute): receiver = receiver.func.value return isinstance(receiver, ast.Name) and receiver.id == "self" def _owner_of(tree, fn): """The ClassDef a function is defined in, or None for a module-level one.""" for node in ast.walk(tree): if isinstance(node, ast.ClassDef) and any(child is fn for child in node.body): return node return None def _columns_already_constrained(tree, series=None): """{class node id: column names a SQL constraint on that class already covers} Two spellings, and neither works everywhere. `_code_unique = models.Constraint("UNIQUE(code)", msg)` is the 19 one: odoo 18.0 core has no models.Constraint at all and declares _sql_constraints in 174 files. `_sql_constraints = [(name, "UNIQUE(code)", msg)]` is the classic one and is IGNORED from 19: the registry logs a warning at load and the constraint is never created. So on a module that pins 19 or later, a _sql_constraints block is not a column being spoken for -- it is a column that nobody is watching and everybody thinks somebody is. Reading it as satisfaction made this checker silent on exactly the file that carries the bug: OpenSPP's spp.cr.type.reason.document declared (cr_type_id, reason) that way, duplicates accumulated unnoticed from 19.0.3.0.0, and the fix (208d9758) re-declared it as models.Constraint. This checker said nothing either side of that commit. `series` unknown keeps the old reading. Firing on every 16, 17 and 18 module that did exactly the right thing is a worse trade than missing the 19 case, and `check` already reports the absent manifest as a gap. Column names are read out of the SQL text, which is crude and sufficient: the question is only whether this column is already spoken for. """ dead_legacy = _series_at_least(series, 19.0) out = {} for node in ast.walk(tree): if not isinstance(node, ast.ClassDef): continue columns = set() for statement in node.body: if not isinstance(statement, ast.Assign): continue if dead_legacy and _assigns_name(statement, "_sql_constraints"): continue for literal in ast.walk(statement): if isinstance(literal, ast.Constant) and isinstance(literal.value, str): for group in _SQL_COLUMNS.findall(literal.value): columns.update( re.findall(r"[A-Za-z_][A-Za-z0-9_]*", group)) out[id(node)] = columns return out def _assigns_name(statement, name): return any(isinstance(t, ast.Name) and t.id == name for t in statement.targets) def legacy_sql_constraints_on_19(code, facts=None): """odoo.sql-constraints-are-ignored-from-19 A class-body `_sql_constraints` assignment that Odoo 19 will not read. From 19 the registry logs one warning at load and creates nothing, so every UNIQUE and CHECK in the block is a constraint nobody is enforcing and everybody thinks somebody is. The incident is OpenSPP's 208d9758 (#394): spp.cr.type.reason.document declared (cr_type_id, reason) this way, duplicates were saved from 19.0.3.0.0, and the fix re-declared it as models.Constraint and added a lint of its own because nothing in CI had caught it. The rest of that tree had already moved, 72 files of models.Constraint to this one holdout. Which series, three answers: 19 or later reported. 16 to 18 silent. _sql_constraints is the ONLY spelling there -- odoo 18.0 core declares it in 174 files and has no models.Constraint -- so a finding would be advice that cannot be taken. OCA's twenty are all 18.0. unknown silent when a tree was walked and does not pin a series, which is odoo core itself: its manifests carry versions like "1.0", and 174 findings there would all be wrong. Reported, advisory, when NOTHING was walked -- a bare snippet, validate's own samples -- because "nobody looked" and "looked and it does not say" are different answers, and silence on the first would hide the rule entirely. An empty list declares nothing and is not reported. """ facts = facts or {} series = facts.get("odoo_series") if _series_at_least(series, 19.0): grade = None elif series is None and not facts.get("walked"): grade = UNPROVEN else: return [] tree, _ = parse_python(code) if tree is None: return [] findings = [] for cls in ast.walk(tree): if not isinstance(cls, ast.ClassDef): continue for statement in cls.body: if isinstance(statement, ast.Assign): targets, value = statement.targets, statement.value elif isinstance(statement, ast.AnnAssign) and statement.value is not None: targets, value = [statement.target], statement.value else: continue if not any(isinstance(t, ast.Name) and t.id == "_sql_constraints" for t in targets): continue if isinstance(value, (ast.List, ast.Tuple)) and not value.elts: continue findings.append(( statement.lineno, "{0} declares _sql_constraints, which Odoo 19 ignores{1}: the " "registry logs a warning and creates no constraint, so nothing " "enforces it -- declare each one as models.Constraint".format( cls.name, "" if grade is None else " (the series is unknown here; this holds from 19)"), grade)) return findings def _series_at_least(series, floor): """Is this module pinned to `floor` or later? False when nothing said.""" try: return float(series) >= floor except (TypeError, ValueError): return False def prefer_sql_constraint_applies(code, facts=None): """odoo.prefer-sql-constraint Fires on an @api.constrains that PostgreSQL could have enforced itself. Two shapes qualify, and deliberately nothing else: uniqueness the body calls search or search_count. That is a uniqueness test with a race inside it -- two concurrent transactions both find nothing, both insert, and the duplicate lands. UNIQUE does not race. bound the body does nothing but compare its own declared columns against literals. CHECK(qty > 0) holds for every writer forever, including the ones that never load the ORM. NOT covered, and this is now a settled decision rather than an omission. CHECK(date_end >= date_start) is perfectly good SQL, so the obvious reading is that recall was left on the table. Two things were weighed, and widening lost on both: It would fire on another rule's correct sample. The canonical two-field constraint in this corpus is odoo.constrains-must-list-every-field-read's own fix -- constrains('date_start', 'date_end') guarding date_end < date_start. A widened checker flags it, `nexa crosscheck` reports a standing hit forever, and the two rules appear to disagree when they do not. A correct sample that another checker condemns is a corpus defect; that is exactly why odoo.constrains-iterate-recordset's sample was rewritten rather than exempted. The suggested replacement is weaker than what it replaces. PostgreSQL treats a CHECK as satisfied when its expression evaluates to NULL, so CHECK(date_end >= date_start) enforces nothing on a row where either date is still empty -- which, for two optional columns, is most rows for most of their life. The Python constraint it would replace usually carries an explicit `if rec.date_start and rec.date_end` guard saying the same thing on purpose. Advising the swap without that caveat reads as a tightening and is not one. A single-column bound does not have the same problem in practice: a numeric column with a default is rarely NULL. So the gap stays, pinned by tests/test_semantics.py::test_two_column_comparison_is_silent. Reopening it means answering both points, not just observing that the SQL is valid. Out of scope by definition: anything reaching a related record, calling a method, or touching self.env. A CHECK genuinely cannot express those, which is what odoo.check-constraint-cannot-cross-tables exists to say. This replaced a regex matching only the search_count shape, which meant the rule claimed ground its own checker never patrolled. TWO NARROWINGS, BOTH FROM `nexa recall --aim` --------------------------------------------- Placed against the method each fix edited, this rule scored 0 on-target and 47 other-symbol out of 49 -- the worst signal in the corpus by that measure. Reading the 85 findings on OpenSPP gave two classes where the advice cannot be followed at all, which is worse than advice nobody wanted: The search is on ANOTHER model. `self.env["other.model"].search(...)` tests uniqueness across tables, and a UNIQUE on this one cannot express that. `endpoint_route_handler._check_route_unique_across_models` is the clearest case: it loops over every consumer model in the registry. That belongs to check-constraint-cannot-cross-tables, which already says so. The class ALREADY declares the constraint being recommended. A model carrying `models.Constraint("UNIQUE(code)", ...)` or an `_sql_constraints` entry for the same column has taken the advice; the Python check beside it is belt and braces, and telling the author to do the thing they did is how a checker gets switched off. Both spellings are read, because Odoo 17 introduced the second and OpenSPP uses both. """ tree, _ = parse_python(code) if tree is None: return [] guarded = _columns_already_constrained( tree, (facts or {}).get("odoo_series")) findings = [] for fn in _functions(tree): declared = _decorator_args(fn, "api", "constrains") if declared is None or declared is UNREADABLE: continue roots = {d.split(".")[0] for d in declared} if roots and roots <= guarded.get(id(_owner_of(tree, fn)), set()): continue # the class already declares this constraint searching = [n for n in ast.walk(fn) if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) and n.func.attr in ("search", "search_count", "search_read") and _searches_self(n)] if searching: findings.append((searching[0].lineno, "{0}() tests uniqueness in Python -- two concurrent " "transactions can both pass it and both insert; a UNIQUE " "models.Constraint cannot race".format(fn.name))) continue if any(isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) and n.func.attr in ("search", "search_count", "search_read") for n in ast.walk(fn)): continue # searches another model: not this rule's # Anything the database cannot see for itself disqualifies the whole # function. Two exemptions, both necessary: calls inside the raise, # because ValidationError(...) is how the finding gets reported rather # than part of what is being decided, and the decorator list, because # @api.constrains(...) is itself a Call and walking the whole function # node would let every constraint disqualify itself. in_raise = _message_only_attrs(fn) body = [node for stmt in fn.body for node in ast.walk(stmt)] if any(isinstance(n, ast.Call) and id(n) not in in_raise for n in body): continue reads = _field_reads(fn, _aliases(fn)) if not reads or any(len(path) != 1 or path[0] not in roots for path, _ in reads): continue # Every condition has to BE a literal comparison, not merely contain # one. `if record.storage == "url" and not record.url:` is a guard plus # a presence check, and only the guard is an ast.Compare -- so the old # test saw a column compared to a literal, concluded the function did # nothing else, and said so. That shape is five of the eight hits this # checker produced across two OCA repositories. if any(not _is_literal_comparison(atom) for atom in _test_atoms(fn)): continue for node in ast.walk(fn): if not isinstance(node, ast.Compare): continue operands = [node.left] + list(node.comparators) if not any(isinstance(o, ast.Constant) and isinstance(o.value, (int, float, str)) and not isinstance(o.value, bool) for o in operands): continue findings.append((node.lineno, "{0}() compares its own declared column against a literal " "and does nothing else -- the same rule as a CHECK in " "models.Constraint holds for every writer, including " "imports, XML-RPC and sudo".format(fn.name))) break return findings def ondelete_leaves_a_dangling_read(code, facts=None): """odoo.ondelete-is-a-decision Fires on a Many2one that declares no ondelete AND is read through somewhere in the same module. The ORM default for an OPTIONAL Many2one is 'set null'. Delete the parent and the reference is nulled; read it afterwards and Odoo hands back an empty recordset, whose attributes evaluate to False rather than raising. That is the rule's whole complaint -- "dangling references that later reads treat as empty" -- and it needs a read to happen at all. Two exemptions, both because the default is already correct there: required=True Odoo defaults a required Many2one to 'restrict', which is what this rule recommends. 56 of the 198 hits on OCA were fields already doing the right thing. related= a related field mirrors another record's value and takes its deletion behaviour from the source. no column a compute= without store=True is never written, so there is no foreign key for ondelete to govern. TransientModel a wizard's record is vacuumed. The harm here is a nulled reference read long afterwards, which needs a row that lives long enough for the parent to be deleted under it. NOT flagged, by choice: a Many2one declared and never dereferenced. Nulling a reference nobody reads through changes no answer, and flagging it produced 4.1 findings per 1000 lines of reviewed OCA code -- enough to bury the other nine Odoo rules under one that is a policy rather than a defect. That makes the checker narrower than the rule, which still asks for an explicit ondelete on every Many2one. The gap is deliberate and is the same one required=True already had. """ tree, _ = parse_python(code) if tree is None: return [] declared = _many2one_without_ondelete(tree) if not declared: return [] findings = [] for name, (lineno, comodel) in sorted(declared.items(), key=lambda kv: kv[1][0]): reads = _dereferences_of(tree, name) if not reads: continue trigger = context.parent_is_deleted(facts, comodel) if trigger == context.ABSENT: # The tree defines this parent and never deletes it. No delete, no # nulled reference, no dangling read -- the rule's entire complaint # is conditional on a thing that does not happen here. continue message = ("{0} declares no ondelete, so the ORM defaults it to " "'set null' -- and {0}.{1} on line {2} reads through it, " "which yields False rather than raising once the parent " "is deleted".format(name, reads[0][0], reads[0][1])) if trigger == context.PRESENT: message += (", and this tree does delete {0}".format(comodel)) findings.append((lineno, message, "defect" if trigger == context.PRESENT else None)) return findings def _many2one_without_ondelete(tree): """{field name: (line, comodel)} for every fields.Many2one(...) that leaves deletion behaviour to the ORM. required=True and related= are exempt. The comodel travels with the declaration because the trigger is a question about the PARENT -- whether anything deletes it -- and by the time the finding is written the call node is gone.""" found = {} transient = _transient_spans(tree) for node in ast.walk(tree): if not isinstance(node, ast.Assign) or len(node.targets) != 1: continue target = node.targets[0] if not isinstance(target, ast.Name): continue call = node.value if not (isinstance(call, ast.Call) and isinstance(call.func, ast.Attribute) and call.func.attr == "Many2one"): continue if _within(transient, node.lineno) or _has_no_stored_column(call): continue exempt = False for keyword in call.keywords: if keyword.arg in ("ondelete", "related"): exempt = True elif keyword.arg == "required" and _is_true(keyword.value): exempt = True if not exempt: found[target.id] = (node.lineno, _comodel_of(call)) return found def _is_true(node): return isinstance(node, ast.Constant) and node.value is True def _is_sql_view(cls): """True for a model declaring `_auto = False`: a SQL view, not a table. Odoo builds no table for one and therefore no foreign key, so `ondelete` is not a decision anybody can make about it -- there is no constraint for a value to be chosen for. Two of the 15 sampled ondelete findings on odoo core were report models of exactly this kind, hr.contract.history and purchase.bill.line.match. """ for node in cls.body: if not isinstance(node, ast.Assign): continue if any(isinstance(t, ast.Name) and t.id == "_auto" for t in node.targets): return isinstance(node.value, ast.Constant) and node.value.value is False return False def _transient_spans(tree): """Line ranges of every class this rule family does not apply to. A wizard's record is vacuumed, so the failure both ondelete-is-a-decision and stored-compute-needs-complete-depends describe -- a value written once and read wrong much later -- has no time in which to happen. A `_auto = False` model has no table at all, which rules out the same failure for a different reason: see _is_sql_view. Returned as spans because the declarations and methods they gate are found by walking the whole tree, which loses the class on the way down. check_company_missing_on_company_owned_relation already had this exemption and iterates classes directly; these two do not, hence the spans. """ return [(n.lineno, getattr(n, "end_lineno", n.lineno)) for n in ast.walk(tree) if isinstance(n, ast.ClassDef) and (_is_transient(n) or _is_sql_view(n))] def _within(spans, lineno): return any(start <= lineno <= end for start, end in spans) def _has_no_stored_column(call): """Does this field declaration put nothing in the database? compute= without store=True is calculated on read and never written, so there is no column for ondelete to act on and nothing for check_company to constrain. Both rules are about what the database HOLDS, and advice about a column that does not exist cannot be taken. """ computed = stored = False for keyword in call.keywords: if keyword.arg == "compute": computed = True elif keyword.arg == "store": stored = _is_true(keyword.value) return computed and not stored def _dereferences_of(tree, field): """[(attribute, line)] for every UNGUARDED .. in the tree. The outer attribute is what makes it a read THROUGH the relation rather than a read OF it. `rec.partner_id` alone is a recordset test and is falsy-safe; `rec.partner_id.name` is the one that quietly becomes False. Reads inside a function that tests the field for truthiness are left out. The rule's complaint is that a nulled reference "yields False rather than raising", and an author who wrote `if not self.model_id: return` has already answered it -- the reference may dangle and nothing reads through it when it does. Measured on OpenSPP, that guard is the majority idiom, and flagging it made the difference between a rule about a defect and a rule about a construct. Function-level, deliberately. The dominant shape is a guard at the top of a method covering everything below it, and following the value any further than that is dataflow analysis this does not have. A read is placed by the field's name, not by the model it is read on. `mapped(".")` is a read too and is left out: measured on the three trees it added one finding, a quant's location read in stock.location's file, and five of the thirteen such reads were another model's field of the same name (ARCHITECTURE 3.79). """ guarded = set() for fn in _functions(tree): if _tests_for_emptiness(fn, field): guarded.update(id(node) for node in ast.walk(fn)) reads = [] for node in ast.walk(tree): if not isinstance(node, ast.Attribute) or id(node) in guarded: continue inner = node.value if not (isinstance(inner, ast.Attribute) and inner.attr == field): continue if node.attr in ORM_ATTRS: continue reads.append((node.attr, node.lineno)) return sorted(reads, key=lambda r: r[1]) def _tests_for_emptiness(fn, field): """Does this function check whether . is set, anywhere in it? `if not self.model_id: return`, `if rec.partner_id:`, `rec.partner_id and rec.partner_id.name`, and the ternary spelling all count. What is looked for is the field appearing as a CONDITION rather than as a value, which is the difference between asking whether it is there and assuming it is. """ def names_the_field(node): while isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.Not): node = node.operand return isinstance(node, ast.Attribute) and node.attr == field for node in ast.walk(fn): tests = [] if isinstance(node, (ast.If, ast.While, ast.IfExp)): tests.append(node.test) elif isinstance(node, ast.BoolOp): tests.extend(node.values) elif isinstance(node, ast.Assert): tests.append(node.test) elif isinstance(node, ast.comprehension): tests.extend(node.ifs) for test in tests: if names_the_field(test): return True if isinstance(test, ast.BoolOp) and any( names_the_field(value) for value in test.values): return True return False # Core models that carry a company_id of their own, so a relation to one can # point across a tenant boundary. Curated rather than derived: Nexa has no Odoo # registry to ask, and a guessed list would fire on every Many2one in the corpus. COMPANY_OWNED = frozenset([ "account.journal", "account.account", "account.payment.term", "account.analytic.account", "stock.warehouse", "stock.location", "stock.picking.type", "product.pricelist", "res.partner.bank", ]) # Models that ARE the company, or are scoped to one by construction. A # check_company on a field of res.company has nothing to compare against; Odoo's # own fields.py carries a special branch for exactly that case. NOT_COMPANY_SCOPED = frozenset(["res.company", "res.users"]) def check_company_missing_on_company_owned_relation(code, facts=None): """odoo.check-company-on-relations Fires on a Many2one or Many2many to a company-owned model, declared without check_company, in a model that is itself company-scoped. All three conditions matter, and the regex that preceded this could express only the first two. Firing without the third is what produced 17 of its 37 hits on OCA: six were on res.company itself, where check_company has nothing to compare against -- odoo/fields.py has a branch for that case; eleven were TransientModel wizards. A wizard's records do not persist, and the harm this rule names is a multi-company client seeing another tenant's data on their DOCUMENT. Flagging wizards is not wrong so much as it is a different, much smaller problem wearing a critical rule's severity. "Company-scoped" is read from the code, not guessed: the class declares a company_id field of its own. A class that only inherits one cannot be resolved without an Odoo registry, so it is skipped rather than assumed -- silence over a false critical. A module-level declaration with no class around it is treated as one implicit model body. That is the shape a generated answer arrives in, and real modules always put fields in a class, so it costs nothing on real code. """ trigger = context.multi_company_in_use(facts) if trigger == context.ABSENT: # One company cannot leak to another. The rule is about a boundary this # codebase does not have, and every finding it could make here would be # about a harm with no mechanism. return [] tree, _ = parse_python(code) if tree is None: return [] classes = [n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)] scopes = classes if classes else [tree] findings = [] for scope in scopes: if isinstance(scope, ast.ClassDef): if _model_names(scope) & NOT_COMPANY_SCOPED: continue if _is_transient(scope) or _is_sql_view(scope): # A SQL view has no table, so no foreign key for check_company # to add a guard to: the rows come from the query. OpenSPP's # fund_report was the one live false positive left for this rule. continue if not _declares_company_id(scope): continue for node in ast.walk(scope): if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr in ("Many2one", "Many2many")): continue comodel = _comodel_of(node) if comodel not in COMPANY_OWNED: continue if any(k.arg == "check_company" for k in node.keywords): continue if any(k.arg == "related" for k in node.keywords): # A mirror of a field on another record. The constraint belongs # on the source, and the source is flagged in its own right -- # so this was also reporting the same decision twice. continue if _has_no_stored_column(node): # No column, nothing to constrain. continue message = ("relation to {0} with no check_company, on a model " "that has its own company_id -- a record in one " "company can reference another company's {0}" .format(comodel)) if trigger == context.PRESENT: message += ", and this codebase runs multi-company" findings.append((node.lineno, message, "defect" if trigger == context.PRESENT else None)) return findings def _model_names(cls): """Every model name the class claims: _name and _inherit, string forms.""" names = set() for node in cls.body: if not isinstance(node, ast.Assign) or len(node.targets) != 1: continue target = node.targets[0] if not (isinstance(target, ast.Name) and target.id in ("_name", "_inherit")): continue value = node.value if isinstance(value, ast.Constant) and isinstance(value.value, str): names.add(value.value) elif isinstance(value, (ast.List, ast.Tuple)): for item in value.elts: if isinstance(item, ast.Constant) and isinstance(item.value, str): names.add(item.value) return names def _is_transient(cls): for base in cls.bases: if isinstance(base, ast.Attribute) and base.attr == "TransientModel": return True if isinstance(base, ast.Name) and base.id == "TransientModel": return True return False def _declares_company_id(scope): body = scope.body if isinstance(scope, (ast.ClassDef, ast.Module)) else [] for node in body: if not isinstance(node, ast.Assign) or len(node.targets) != 1: continue target = node.targets[0] if isinstance(target, ast.Name) and target.id == "company_id": return True return False def _comodel_of(call): """The model a relational field points at, positional or keyword.""" if call.args and isinstance(call.args[0], ast.Constant) \ and isinstance(call.args[0].value, str): return call.args[0].value for keyword in call.keywords: if keyword.arg == "comodel_name" and isinstance(keyword.value, ast.Constant): return keyword.value.value return None # ---------------------------------------------------- the hierarchy checker # Either spelling clears the rule. _check_recursion still works on 18.0 -- it # warns and delegates to _has_cycle -- so code carrying it is out of date, not # unguarded. CYCLE_GUARDS = frozenset(["_has_cycle", "_check_recursion"]) def parent_can_form_a_cycle(code): """odoo.recursion-check-on-hierarchy Fires on a `parent_id` Many2one that points at its own model, in a model nothing protects from a cycle. Was a regex for `parent_id = fields.Many2one`, cleared by either guard's name anywhere in the file. It could not ask the two questions the harm depends on, and both produced false positives that were classified true: Does the field point at its own model? A `parent_id` to a different model is an ordinary foreign key and cannot close a loop. OCA's account.cash.deposit.line.parent_id points at account.cash.deposit. Does the framework already refuse the cycle? A model with `_parent_store = True` keeps parent_path, and _parent_store_update raises "Recursion Detected." on any write that would make a record its own ancestor (odoo/models.py, present since 2019). Six of the seven verdicts once marked true -- account.asset.group on OCA, and spp.area, spp.area.type, spp.audit.rule, spp.consent.purpose and spp.hazard.category on OpenSPP -- were models doing exactly this. The one that survived both questions is the shape the rule is for: spp.dms.directory declares a parent_path CHAR but never sets _parent_store, so nothing maintains it and nothing refuses a loop, while its complete_name and root_directory_id computes recurse up the chain. Self-reference is read through the three spellings real modules use: a string, the class's own `_name` variable, and a module constant both are assigned from (`_name = _type_model`). A comodel that cannot be resolved keeps the old reading and fires, and so does a module-level declaration with no class around it -- the shape a generated answer arrives in. """ tree, _ = parse_python(code) if tree is None: return [] for node in ast.walk(tree): name = node.attr if isinstance(node, ast.Attribute) else \ node.id if isinstance(node, ast.Name) else None if name in CYCLE_GUARDS: return [] constants = _module_string_constants(tree) classes = [n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)] findings = [] for scope in classes or [tree]: own = None if isinstance(scope, ast.ClassDef): if _parent_store_guards_parent_id(scope) or _is_sql_view(scope): # A view is written by nobody: its parent comes from the query # over the tables underneath, which is where a guard belongs. continue own = _own_model_name(scope, constants) for node in scope.body: if not (isinstance(node, ast.Assign) and len(node.targets) == 1 and isinstance(node.targets[0], ast.Name) and node.targets[0].id == "parent_id" and isinstance(node.value, ast.Call) and isinstance(node.value.func, ast.Attribute) and node.value.func.attr == "Many2one"): continue if not node.value.args and not any(k.arg == "comodel_name" for k in node.value.keywords): # `parent_id = fields.Many2one(tracking=3)` names no model: it # re-declares attributes of a field declared elsewhere, and the # relation, with any guard on it, lives with that declaration. continue comodel = _resolved_comodel(node.value, constants, own) if own is not None and comodel is not None and comodel != own: continue findings.append((node.lineno, "parent_id points at {0} itself and nothing refuses a " "cycle -- a record made its own ancestor loops every " "recursive read until the worker is killed. " "_parent_store = True would refuse it, and so would an " "@api.constrains calling _has_cycle()".format( comodel or own or "its own model"))) return findings def _module_string_constants(tree): """NAME = "string" at module level: how a model name gets shared.""" found = {} for node in tree.body: if (isinstance(node, ast.Assign) and len(node.targets) == 1 and isinstance(node.targets[0], ast.Name) and isinstance(node.value, ast.Constant) and isinstance(node.value.value, str)): found[node.targets[0].id] = node.value.value return found def _class_assignment(cls, attr): """The value a class-body assignment gives `attr`, chained ones included. Odoo's own test models write `_name = _description = 'x'`; reading only single-target assignments left those classes nameless, so a parent_id to a different model read as unresolvable and fired. """ for node in cls.body: if isinstance(node, ast.Assign) and any( isinstance(t, ast.Name) and t.id == attr for t in node.targets): return node.value return None def _own_model_name(cls, constants): """The model this class defines or extends, when one name says it.""" value = _class_assignment(cls, "_name") or _class_assignment(cls, "_inherit") if isinstance(value, ast.Constant) and isinstance(value.value, str): return value.value if isinstance(value, ast.Name): return constants.get(value.id) return None def _parent_store_guards_parent_id(cls): """True when the ORM itself refuses a cycle through parent_id.""" store = _class_assignment(cls, "_parent_store") if not _is_true(store): return False # _parent_store maintains the field _parent_name names, parent_id unless # said otherwise. A model storing some other chain leaves parent_id bare. name = _class_assignment(cls, "_parent_name") return name is None or (isinstance(name, ast.Constant) and name.value == "parent_id") def _resolved_comodel(call, constants, own): literal = _comodel_of(call) if literal is not None: return literal expressions = [call.args[0]] if call.args else [] expressions += [k.value for k in call.keywords if k.arg == "comodel_name"] for expr in expressions: if isinstance(expr, ast.Name): if expr.id == "_name": return own if expr.id in constants: return constants[expr.id] return None # ------------------------------------------------------ the XML data checker # Models whose records an administrator tunes through the UI after install, so # that an upgrade rewriting them undoes a decision somebody made on purpose. # # Chosen by measurement, not by opinion. Odoo core puts its own records of each # under noupdate almost without exception -- ir.config_parameter 26 of 28, # ir.sequence 16 of 17, decimal.precision 7 of 8, and stages 30 of 34 across # seven stage models -- while views, actions and reports, which must upgrade, # sit at 0 to 2%. ir.cron is the exception to that method: core protects only # 39 of 93. It came in on an incident: OpenSPP's #473 is two crons and two # parameters in one file with no noupdate, and names the harm in its title -- # an admin who changed an interval or disabled a cron is reset by the next # upgrade. #479 fixed it with noupdate and a migration. Reading every cron # finding across the three trees then narrowed it to one shape: see # editable_record_without_noupdate. # # ir.rule is out on purpose, although core protects 566 of 598. A record rule # frozen by noupdate never receives the security fix its module ships later, # which is a trade-off a checker has no business deciding for anybody. EDITABLE_DATA_MODELS = frozenset([ "ir.config_parameter", "ir.sequence", "decimal.precision", "ir.cron", ]) def _is_editable_data_model(model): return model in EDITABLE_DATA_MODELS or (model or "").endswith(".stage") def _is_false_literal(text): return text.strip().lower() in ("false", "0") def parse_xml(code): """(True, None), or (False, (line, message)) when expat cannot read it.""" parser = xml.parsers.expat.ParserCreate() try: parser.Parse(code.encode("utf-8"), True) except xml.parsers.expat.ExpatError as exc: return False, (exc.lineno, xml.parsers.expat.errors.messages.get( exc.code, "not well-formed")) return True, None # An Odoo data document. An answer from the model can carry one inside prose # and Python, where the whole text is not XML; the documents inside it still are. _XML_DOCUMENT = re.compile(r"<(odoo|openerp)\b.*?", re.S) def _xml_documents(code): """(line offset, text) for each Odoo data document the code holds.""" ok, _ = parse_xml(code) if ok: return [(0, code)] return [(code.count("\n", 0, m.start()), m.group(0)) for m in _XML_DOCUMENT.finditer(code)] def _records_outside_noupdate(document): """(line, model, xml id, ships disabled) for each no noupdate="1" reaches. noupdate is inherited from the enclosing , or , and an inner element can switch it back off, so it is tracked per element. `ships disabled` is whether the record sets its `active` field false. """ stack, found = [], [] current = {} parser = xml.parsers.expat.ParserCreate() def start(tag, attrs): inherited = stack[-1] if stack else False if attrs.get("noupdate") is not None: inherited = attrs["noupdate"].strip().lower() in ("1", "true") stack.append(inherited) if tag == "record" and not inherited: current.clear() current.update(line=parser.CurrentLineNumber, model=attrs.get("model"), id=attrs.get("id"), disabled=False, text=None) elif tag == "field" and current and attrs.get("name") == "active": if attrs.get("eval") is not None: current["disabled"] = _is_false_literal(attrs["eval"]) else: current["text"] = [] def chars(data): if current.get("text") is not None: current["text"].append(data) def end(tag): stack.pop() if tag == "field" and current.get("text") is not None: current["disabled"] = _is_false_literal("".join(current["text"])) current["text"] = None elif tag == "record" and current: found.append((current["line"], current["model"], current["id"], current["disabled"])) current.clear() parser.StartElementHandler, parser.EndElementHandler = start, end parser.CharacterDataHandler = chars try: parser.Parse(document.encode("utf-8"), True) except xml.parsers.expat.ExpatError: return [] return found def editable_record_without_noupdate(code): """odoo.noupdate-on-user-editable-data Fires on each of a model administrators tune (see EDITABLE_DATA_MODELS) that no noupdate="1" reaches, at the record's own line. Was a regex for `` immediately followed by `, for any record at all -- a view, a menu, an action, all of which have to upgrade -- and one noupdate block anywhere silenced every record outside it. OpenSPP's spp_starter_sp_mis config_parameters.xml after its own fix still carries a parameter outside the block, and the regex called the file clean. It had also never been fired at real code: fieldtest and recall read .py files only, and this rule reads XML. Its `unproven` grade was a statement about where the tools looked. """ findings = [] for offset, document in _xml_documents(code): for line, model, xid, disabled in _records_outside_noupdate(document): if not _is_editable_data_model(model): continue if model == "ir.cron" and not disabled: # A cron's schedule is usually the mechanism, not a setting: # 57 of 62 cron findings across the three trees read false, and # core keeps 54 of its own 93 crons updatable on purpose. The # shape that held up ships disabled for an operator to switch # on -- which is exactly the decision an upgrade then undoes. continue findings.append((offset + line, "{0} record '{1}' has no noupdate=\"1\" -- every module " "upgrade rewrites it with the shipped value, silently " "undoing what an administrator changed. Adding noupdate " "now does not reach databases that already have it: " "ir_model_data.noupdate also needs flipping in a " "migration".format(model, xid or "?"))) return findings # ------------------------------------------------------- the cursor checkers # The names an Odoo database cursor is reached by. self.env.cr and self._cr are # the two the framework provides; `cr` alone is the argument name every # migration script and post-init hook uses. Anything else called .execute() or # .commit() is somebody else's object and none of our business. CURSOR_NAMES = frozenset(["cr", "_cr", "cursor", "_cursor"]) def _cursor_call(node, method): """True when this Call is .cr.(...). The attribute immediately before the method name is what decides it, so self.env.cr.execute, self._cr.execute and a bare cr.execute all match while connection.execute and session.commit do not. """ if not isinstance(node, ast.Call): return False func = node.func if not isinstance(func, ast.Attribute) or func.attr != method: return False owner = func.value if isinstance(owner, ast.Attribute): return owner.attr in CURSOR_NAMES if isinstance(owner, ast.Name): return owner.id in CURSOR_NAMES return False def _root_name(node): """The name a dotted chain starts from. self.env.cr -> 'self'.""" while isinstance(node, ast.Attribute): node = node.value return node.id if isinstance(node, ast.Name) else None def _independent_cursors(scope): """Names in this scope standing for a cursor the caller does not own. `with Registry(...).cursor() as new_cr` opens a SECOND connection, and an Environment built on it commits its own transaction. That is not the caller's unit of work being split -- it is the documented way to write a batch job that must not hold one transaction open for the whole run, and the rule's own instruction is "let the caller own the transaction", which this already does. Bound POSITIVELY, by watching the cursor be opened, rather than by treating anything not spelled self.env.cr as independent. The two field instances of this rule are exactly the two sides of that line: OpenSPP's expiry cron commits self.env.cr inside its loop and is a true positive its author marked and kept, while OCA's batch_unlink commits a cursor it opened three lines earlier and is not. """ names = set() for _ in range(3): before = len(names) for node in ast.walk(scope): if isinstance(node, ast.withitem): call = node.context_expr if (isinstance(call, ast.Call) and isinstance(call.func, ast.Attribute) and call.func.attr == "cursor" and isinstance(node.optional_vars, ast.Name)): names.add(node.optional_vars.id) elif (isinstance(node, ast.Assign) and len(node.targets) == 1 and isinstance(node.targets[0], ast.Name) and isinstance(node.value, ast.Call)): # api.Environment(new_cr, ...) carries the new cursor forward. if any(isinstance(a, ast.Name) and a.id in names for a in node.value.args): names.add(node.targets[0].id) if len(names) == before: break return names def commit_inside_a_loop(code): """odoo.commit-inside-a-loop-leaves-half-written-data A commit ends the transaction the ORM is working inside. Done once per iteration, the loop stops being one unit of work: iteration seven raising leaves iterations one to six already written, the caller's except clause has nothing left to roll back, and the user sees an error over data that stayed. Only loops, not every commit. A commit in a cron batch or a migration script is a deliberate decision about batch size, and a rule that fired on all of them would be argued with rather than read. Inside a loop over a recordset it is almost never deliberate -- it is somebody flushing as they go -- and that is the shape this reports. The receiver decides, not the method name: `.commit()` on anything that is not a cursor belongs to some other library. """ tree, _ = parse_python(code) if tree is None: return [] # Which names mean an independent cursor, per function. Scoped rather than # gathered over the file, so a `new_cr` opened in one method cannot excuse a # commit in another that happens to reuse the name. spans = [(fn.lineno, getattr(fn, "end_lineno", fn.lineno), _independent_cursors(fn)) for fn in _functions(tree)] def independent_at(lineno): best = None for start, end, names in spans: if start <= lineno <= end and (best is None or start > best[0]): best = (start, names) return best[1] if best else set() findings = [] for loop in ast.walk(tree): if not isinstance(loop, (ast.For, ast.AsyncFor, ast.While)): continue # The loop's own body, not its else clause: an else runs once, after # the loop, and a commit there is a commit after a loop. for statement in loop.body: for node in ast.walk(statement): if not _cursor_call(node, "commit"): continue if _root_name(node.func.value) in independent_at(node.lineno): continue findings.append(( node.lineno, "cr.commit() inside a loop -- an exception on a later " "iteration leaves the earlier ones written, and the " "caller cannot roll the batch back")) return sorted(set(findings)) def sql_built_by_string_formatting(code): """odoo.sql-needs-query-parameters cr.execute("... id = %s", (rec.id,)) is correct and contains a percent sign. cr.execute("... id = %s" % rec.id) is the bug and contains the same percent sign. A regex cannot tell them apart, which is why this is an AST pass: the difference is whether the substitution happens in Python before the cursor sees the string, and that is a BinOp rather than a character. Four spellings, all of them the same mistake: "..." % value BinOp with Mod "...".format(value) a call to .format on a string f"...{value}..." JoinedStr "..." + value BinOp with Add, one side a string Only the FIRST argument is examined. The second is the parameter tuple and building that with formatting is somebody's business, not a SQL injection. """ tree, _ = parse_python(code) if tree is None: return [] findings = [] for node in ast.walk(tree): if not _cursor_call(node, "execute") or not node.args: continue how = _how_the_query_was_built(node.args[0]) if how: findings.append(( node.args[0].lineno, "the query handed to cr.execute() is built by {0} -- pass the " "values as the second argument so the driver quotes them".format(how))) return sorted(set(findings)) def _how_the_query_was_built(node): """The name of the string operation that produced this query, or "".""" if isinstance(node, ast.JoinedStr): return "an f-string" if isinstance(node, ast.BinOp): if isinstance(node.op, ast.Mod): return "percent formatting" if isinstance(node.op, ast.Add) and (_is_a_string(node.left) or _is_a_string(node.right)): return "string concatenation" if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) \ and node.func.attr == "format" and not _is_composed_sql(node.func.value): return "str.format()" return "" def _is_composed_sql(node): """psycopg2's sql.SQL("... {}").format(sql.Identifier(name)) is the CORRECT way to parameterise a table or column name, and it spells .format(). Found by running this over 229,600 lines of OpenSPP: the one finding that was wrong rather than merely suppressed was this construct, on a DROP INDEX whose identifier the surrounding code had already validated. A checker that tells an author to stop using the safe API is worse than one that says nothing. """ if not isinstance(node, ast.Call): return False func = node.func if isinstance(func, ast.Attribute): return func.attr in ("SQL", "Identifier", "Composed", "Literal") return isinstance(func, ast.Name) and func.id in ("SQL", "Identifier") def _is_a_string(node): return ((isinstance(node, ast.Constant) and isinstance(node.value, str)) or isinstance(node, ast.JoinedStr)) # Odoo's exception hierarchy, read out of odoo/exceptions.py and identical in # 16.0 through 18.0. Every one of these subclasses UserError, which is why # catching UserError first makes the specific handler below it dead code. ODOO_EXCEPTION_PARENT = { "AccessDenied": "UserError", "AccessError": "UserError", "MissingError": "UserError", "ValidationError": "UserError", "UserError": "Exception", "RedirectWarning": "Exception", } def _is_subclass_of(inner, outer): """Would `except outer` catch something raised as `inner`? Odoo's hierarchy comes from the table above; everything else is asked of the interpreter, so builtins stay correct without a second list to maintain. An unknown name answers False -- a class this pass has never heard of is not evidence of anything. """ if inner == outer: return True seen = set() cursor = inner while cursor in ODOO_EXCEPTION_PARENT and cursor not in seen: seen.add(cursor) cursor = ODOO_EXCEPTION_PARENT[cursor] if cursor == outer: return True if outer in ("BaseException", "Exception") and ( inner in ODOO_EXCEPTION_PARENT or _builtin_exception(inner)): return outer == "BaseException" or inner != "BaseException" first, second = _builtin_exception(inner), _builtin_exception(outer) return bool(first and second and issubclass(first, second)) def _builtin_exception(name): found = getattr(builtins, name, None) return found if isinstance(found, type) and issubclass(found, BaseException) else None def _handler_names(handler): """The exception names one `except` clause catches, tuples included.""" node = handler.type if node is None: return [] # a bare except, which Python forces last parts = node.elts if isinstance(node, ast.Tuple) else [node] names = [] for part in parts: if isinstance(part, ast.Name): names.append(part.id) elif isinstance(part, ast.Attribute): names.append(part.attr) # odoo.exceptions.AccessError return names def handler_order_shadows_a_subclass(code): """odoo.except-order-follows-the-hierarchy Python tries handlers in order and takes the first that matches, so a base class placed above a subclass makes the lower handler dead code. Nothing warns: it parses, it imports, it runs, and the specific branch is simply never entered. AccessDenied, AccessError, MissingError and ValidationError all subclass UserError -- read out of odoo/exceptions.py in 16.0 and 18.0, where the hierarchy is identical. Builtins are asked of the interpreter rather than listed, so `except Exception` above `except KeyError` is the same finding for the same reason. HOW THIS RULE CAME ABOUT, INCLUDING THE PART THAT WAS WRONG ----------------------------------------------------------- A tally over the 62 OpenSPP fixes the corpus was silent on said NINE of them added the name `AccessError` or `MissingError`, which read as the largest missing-rule cluster in that list. This checker was written from that count. It catches none of those nine. Reading the actual diffs afterwards -- which the count was standing in for, and should not have been -- the before-state is one handler, not two in the wrong order: try: service.submit(cr) except UserError as e: raise HTTPException(status_code=409, ...) AccessError is not mishandled there, it is unhandled: swallowed by the base class and reported as 409 Conflict. Nothing shadows anything, and "caught the base class without distinguishing a subclass" is not statically decidable as a defect, because `except UserError` is correct in most code. So the tally was a bad proxy for the diffs, which is the same mistake SCOPE_TOKENS makes when it counts a commit in scope for mentioning ValidationError. The construct this DOES catch is a real defect and the rule is kept for it, but with nothing behind it: zero findings on 229,600 lines of OpenSPP and no incident in any history measured here. It ships `advisory` on the second ground -- unproven rather than a preference -- which is exactly what that label is for. """ tree, _ = parse_python(code) if tree is None: return [] findings = [] for node in ast.walk(tree): if not isinstance(node, ast.Try): continue earlier = [] for handler in node.handlers: names = _handler_names(handler) for outer, outer_line in earlier: for inner in names: if inner != outer and _is_subclass_of(inner, outer): findings.append(( handler.lineno, "'except {0}' on line {1} already catches {2}, so " "this handler never runs -- {2} subclasses {0}".format( outer, outer_line, inner))) earlier.extend((name, handler.lineno) for name in names) return sorted(set(findings)) def depends_names_unread_fields(code): """odoo.depends-must-not-name-unread-fields The other direction of depends_covers_compute_reads, and a milder problem: a declared dependency the body never reads does not make the value stale, it makes the value recompute on writes that cannot change it. Harvested from OpenSPP's history rather than reasoned about. Two fix commits, both of them a leftover: @api.depends("partner_id", "pathway_id", "assessment_date") def _compute_name(self): # reads partner_id and pathway_id only @api.depends("topic_ids") def _compute_session_count(self): # counts rows on another model The second is why this is worth reporting at all. The decorator is the only statement anywhere of what a stored value depends on, and a reader trusts it. Left naming a field an older body used to read, it hides that this compute has no expressible dependency at all -- it queries a different model -- and the staleness that follows looks accounted for. Deliberately permissive, because a wrong entry costs a recompute and a false finding costs trust: A string literal anywhere in the body counts as a read. filtered("x"), mapped("a.b") and sorted(key=...) name fields without an attribute access ever appearing. A declared path is covered by any read that shares a prefix with it in either direction, so declaring line_ids while reading line_ids.price is fine and so is the reverse. A body that calls a method on self, or hands a record to any call, is left alone entirely. The read is then inside the callee, this pass cannot follow it, and the honest answer is not to guess. Measured on OpenSPP, this is the largest false-positive class: a compute that declares five fields and passes the record to a resolver that reads all five. A body that reads a field the decorator does not declare is left alone too. The two are already out of step, and that disagreement belongs to depends_covers_compute_reads -- reporting it twice from opposite directions is two findings about one confusion. It is also how a compute reading another stored compute looks from here, and those declare the upstream source on purpose. "Reads" here are the ones that rule makes, fields named in a string (_named_reads) among them, so the two agree about what the body reads. """ tree, _ = parse_python(code) if tree is None: return [] findings = [] for fn in _functions(tree): declared = _decorator_args(fn, "api", "depends") if not declared or declared is UNREADABLE: continue if _calls_a_method_on_self(fn) or _hands_a_record_to_a_call(fn) \ or _queries_another_model(fn) or _calls_super(fn): # Not exempt: a body that only assigns constants. On core four of # them were a stub for overrides or a deliberate reset, and on # OpenSPP and OCA three were leftovers classified true by hand -- # "Legacy method", a target model that is always res.partner. The # file cannot tell intent, so that shape stays a finding. continue aliases = _aliases(fn) read = set(".".join(path) for path, _ in _field_reads(fn, aliases) + _named_reads(fn, aliases)) produced = set(_assigned_attrs(fn)) if _reads_outside_the_declarations(read - produced, declared): continue read |= produced read |= _string_literals(fn) unread = [entry for entry in declared if not _covered_by_a_read(entry, read)] if unread: # One finding per method, not per entry. Three leftovers on one # decorator is one thing to fix, and three lines all pointing at # the same def read as three problems. findings.append(( fn.lineno, "{0}() declares {1} in @api.depends and never reads {2} -- the " "value recomputes on writes that cannot change it, and the " "decorator stops describing the method".format( fn.name, ", ".join(repr(str(e)) for e in unread), "them" if len(unread) > 1 else "it"))) return sorted(set(findings)) def _calls_super(fn): """Does the body call super()? Odoo merges @api.depends across the MRO: Field.get_depends walks every override of the compute and extends one list with each one's _depends (odoo/fields.py, 18.0 at 3c3e3b3). So an override's decorator ADDS triggers for the chain, and the fields it names are read by the parent's body, reached through super(). Core writes it as a comment: "EXTENDS 'web' to add dependencies". Its own body reading nothing is the point. Three of the fifteen sampled on odoo core were this, and all 40 findings the exemption removed from core were an override that delegates to super(). """ for node in _walk_body(fn): if (isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "super"): return True return False def _reads_outside_the_declarations(read, declared): """Does the body read a field no declared entry covers? Prefix-wise in both directions, so reading partner_id.name against a declared partner_id is inside the declarations and reading intervention_count against a declared intervention_ids is outside them. """ for path in read: if not any(path == entry or path.startswith(entry + ".") or entry.startswith(path + ".") for entry in declared): return True return False def _hands_a_record_to_a_call(fn): """Does any call in the body receive a record as an argument? resolve(scope) can read every field on scope, so a dependency declared for its benefit is correct and invisible here. A domain built from rec.id is not this: an id is an integer, and no callee reads a field through one. """ aliases = _aliases(fn) for node in _walk_body(fn): if not isinstance(node, ast.Call): continue for argument in list(node.args) + [kw.value for kw in node.keywords]: if _is_record_valued(argument, aliases): return True return False QUERY_METHODS = frozenset(["search", "search_count", "search_read", "read_group", "_read_group", "execute", "_search"]) def _queries_another_model(fn): """Does the body count or fetch rows from somewhere other than self? The largest class of false finding here by a wide margin: 17 of the 31 this reported on OpenSPP. A compute whose value comes from rows in ANOTHER model has no dependency @api.depends can express -- the ORM cannot watch a table this record has no field for -- so whatever the author declared is their best available approximation, usually the inverse One2many, and it often works. Telling them to delete it removes the only recompute trigger the value has. The cost of the exclusion is real and stated rather than hidden: one of the two harvested incidents this checker was written from is exactly this shape -- @api.depends("topic_ids") on a compute that counted rows on another model -- and is no longer reported. Reading it, the author's fix was to decide there was no correct dependency at all, which is a judgement this pass has no way to make for them. """ for node in _walk_body(fn): if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) \ and node.func.attr in QUERY_METHODS: return True return False def _is_record_valued(node, aliases): """Is this argument a record, or a field of one, rather than a plain value? Collection literals are looked inside, because a domain is a list of tuples and a vals dict is a dict of them. Attribute chains are NOT walked into: the Name at the base of rec.id is not itself an argument, and counting it made a search domain look like a record being handed over. """ if isinstance(node, ast.Name): return node.id in aliases if isinstance(node, ast.Attribute): path = _attr_path(node, aliases) return bool(path) and path[0] not in ORM_ATTRS \ and not path[0].startswith("_") if isinstance(node, (ast.List, ast.Tuple, ast.Set)): return any(_is_record_valued(item, aliases) for item in node.elts) if isinstance(node, ast.Dict): return any(_is_record_valued(item, aliases) for item in list(node.keys) + list(node.values) if item) return False def _covered_by_a_read(entry, read): """A declared path is covered by any read sharing a prefix with it.""" for path in read: if path == entry or path.startswith(entry + ".") \ or entry.startswith(path + "."): return True return False def _walk_body(fn): """Every node in the function's BODY, decorators excluded. ast.walk(fn) includes the decorator list, and @api.depends("x") holds the string "x". Counted as evidence the body mentions x, every declared entry proves itself and the checker silently finds nothing -- which is exactly what it did on the incident it was written from. """ for statement in fn.body: for node in ast.walk(statement): yield node def _string_literals(fn): """Every string in the body, plus each dotted path inside one. filtered("is_late") and mapped("line_ids.price") name fields without an attribute access ever appearing, and a declared dependency on either is correct. Splitting the dotted ones lets a declared prefix match too. """ out = set() for node in _walk_body(fn): if isinstance(node, ast.Constant) and isinstance(node.value, str): text = node.value.strip() if text: out.add(text) out.update(re.findall(r"[A-Za-z_][A-Za-z0-9_.]*", text)) return out def _calls_a_method_on_self(fn): """Does this body call a method ON the record -- rec._helper(), self._go()? The read is then inside the helper, where this pass cannot follow it, and a dependency declared for the helper's benefit is correct. Reporting it would be telling the author to delete a line that is doing its job. Length one, deliberately. rec._helper() resolves to the path ('_helper',) and can read anything; rec.line_ids.filtered(...) resolves to ('line_ids', 'filtered') and is an ORM call on one field, which reaches no other. Taking both lost the incident this checker was written from, where the body's only call was .replace() on a string. """ aliases = _aliases(fn) for node in _walk_body(fn): if not isinstance(node, ast.Call): continue func = node.func if not isinstance(func, ast.Attribute): continue path = _attr_path(func, aliases) if path is not None and len(path) == 1: return True return False # ------------------------------------------------ evidence for the triggers # # Two rules in the corpus are conditional on something no single file contains. # `ondelete` only bites if the parent is ever actually deleted; `check_company` # only bites if the codebase runs multi-company at all. Both conditions are # answerable from the tree, and these are the per-file halves of that answer -- # context.tree_facts aggregates them and the checkers read the aggregate. # # Everything here returns EVIDENCE FOUND, never evidence absent. One file not # deleting a model says nothing; the whole tree not deleting it is a claim, and # the only place entitled to make it is the thing that saw every file. # How a codebase shows that more than one company is really in play. A plain # `company_id` is not on this list on purpose: nearly every Odoo model declares # one, in single- and multi-company deployments alike, so it separates nothing. # These four are the machinery you only write when several companies exist -- # a user belonging to more than one, code reading the active set, or an author # who has already met the rule. MULTI_COMPANY_MARKERS = ("company_ids", "allowed_company_ids", "env.companies", "check_company") def models_declared(code): """Model names this file DEFINES, from `_name`. Not `_inherit`. The distinction decides what a tree is entitled to say about deletion. A model defined here is one whose code and access rules this tree owns, so "nothing in the tree deletes it" is a claim about the whole of it. A model merely extended with `_inherit` is somebody else's -- core Odoo's, usually -- and the delete could be anywhere in a codebase this walk never saw. """ tree, _ = parse_python(code) if tree is None: return set() names = set() for cls in [n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)]: for node in cls.body: if not isinstance(node, ast.Assign) or len(node.targets) != 1: continue target = node.targets[0] if not (isinstance(target, ast.Name) and target.id == "_name"): continue if (isinstance(node.value, ast.Constant) and isinstance(node.value.value, str)): names.add(node.value.value) return names # Field types whose first positional argument names the comodel. For every other # type the first positional argument is the label. RELATIONAL_FIELDS = frozenset(["Many2one", "One2many", "Many2many"]) def orm_declarations(code): """What a file declares about its models, for the tree facts. One record per class that names a model: the model, the models it inherits (`_inherit` beyond itself, which put their methods in its MRO), the fields it declares and the literal @api.depends of each method that has one. Read the way Odoo reads them (odoo/models.py, 18.0 at 3c3e3b3): `_name`, or failing that a single `_inherit`; a class inheriting several models without a `_name` is named after itself there, and is left out here. A field is {"compute", "related", "store", "inverse", "comodel"}. `store` is True, False, None when the declaration does not say, or "?" when it says it with something other than a literal. A method's depends are the list, or None when @api.depends is there and cannot be read. `named` says the class gives the model its `_name`, rather than extending it. """ # ast.parse alone: the compile step parse_python adds reports errors this # never reads, and took about a quarter of the time (ARCHITECTURE 3.84). try: tree = ast.parse(code) except SyntaxError: return [] records = [] for cls in [n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)]: model, named, inherit = _class_model(cls) if model is None: continue fields, methods = {}, {} for field, call in _class_field_calls(cls).items(): keywords = {k.arg: k.value for k in call.keywords if k.arg} store = keywords.get("store") compute = keywords.get("compute") comodel = None if call.func.attr in RELATIONAL_FIELDS: comodel = (_literal_string(call.args[0]) if call.args else _literal_string(keywords.get("comodel_name"))) fields[field] = { "compute": (compute.value if isinstance(compute, ast.Constant) and isinstance(compute.value, str) else compute.id if isinstance(compute, ast.Name) else None), "related": _literal_string(keywords.get("related")), "store": (None if store is None else store.value if isinstance(store, ast.Constant) else "?"), "inverse": "inverse" in keywords, "comodel": comodel, } for fn in cls.body: if not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)): continue depends = _decorator_args(fn, "api", "depends") if depends is not None: methods[fn.name] = None if depends is UNREADABLE else list(depends) records.append({"model": model, "named": named, "parents": [p for p in inherit if p != model], "fields": fields, "methods": methods}) return records def _literal_string(node): return node.value if isinstance(node, ast.Constant) and isinstance(node.value, str) \ else None def _literal_strings(node): """['a', 'b'] for "a" or ["a", "b"]; None when any part is not a literal.""" single = _literal_string(node) if single is not None: return [single] if isinstance(node, (ast.List, ast.Tuple)): values = [_literal_string(element) for element in node.elts] return values if all(v is not None for v in values) else None return None def models_unlinked(code): """Model names this file gives evidence of DELETING. Three shapes, which between them are how a delete is spelled in practice: self.env['x.y'].search(...).unlink() the model is named in the call self.unlink() inside a class, so the class's own models are what gets deleted def unlink(self) an override. Nobody writes one for a model nothing deletes, and it is the single clearest statement that deletion is a path this code expects. A local name assigned from `self.env['x.y']` and unlinked later resolves too, because splitting the search and the unlink across two lines is the common spelling and a checker that missed it would call the model undeleted. """ tree, _ = parse_python(code) if tree is None: return set() owners = {} # class node -> the models it speaks for for cls in [n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)]: owners[cls] = _model_names(cls) found = set() # An override is a statement about the model the class declares. for cls, names in owners.items(): for node in cls.body: if (isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == "unlink"): found |= names # Local names that hold a recordset of a known model. bound = {} for node in ast.walk(tree): if (isinstance(node, ast.Assign) and len(node.targets) == 1 and isinstance(node.targets[0], ast.Name)): model = _env_model_in(node.value) if model: bound[node.targets[0].id] = model for node in ast.walk(tree): if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr == "unlink"): continue receiver = node.func.value model = _env_model_in(receiver) if model: found.add(model) continue if isinstance(receiver, ast.Name) and receiver.id in bound: found.add(bound[receiver.id]) continue # `self.unlink()` and `record.unlink()` inside a model body: the models # the enclosing class declares are the ones being deleted. if isinstance(receiver, ast.Name) and receiver.id == "self": found |= _enclosing_models(tree, node, owners) return found def _env_model_in(node): """The model name in an `env['x.y']` subscript anywhere inside node, if the expression is rooted in one. `self.env['x'].search(...)` and `self.env['x'].browse(i)` both resolve; a bare `records` does not.""" if node is None: return None for inner in ast.walk(node): if not isinstance(inner, ast.Subscript): continue value = inner.value if not (isinstance(value, ast.Attribute) and value.attr == "env"): continue key = inner.slice if isinstance(key, ast.Constant) and isinstance(key.value, str): return key.value return None def _enclosing_models(tree, target, owners): """The models declared by the class that lexically contains `target`.""" for cls, names in owners.items(): for node in ast.walk(cls): if node is target: return names return set() def declares_multi_company(code): """True if this file shows the codebase really runs more than one company. Textual on purpose. The markers are attribute names and a keyword argument rather than a shape, so an AST pass would spell the same search at more length, and the question is only ever "does this appear anywhere". """ view = strip_comments(code, "python") return any(marker in view for marker in MULTI_COMPANY_MARKERS) # ------------------------------------------- where a value came from # # Four rules were classified false for the same reason: each matched a # variable's NAME and called that evidence about its value. `total == 0` is a # float comparison in one function and an exact integer test in the next; # `filename` says nothing about whether a caller chose it. Telling those apart # needs the assignment, which is what this section reads. # # It is a local trace and nothing more. One file, one function, no imports # followed and no types inferred -- which is enough for the shapes that were # actually misread, and the limit is stated here rather than discovered later. def _function_of(tree): """Maps every node to the function that encloses it, innermost winning. Lambdas count. `filtered(lambda x: x.qty == 0)` is where one of the misreadings lived, and a lambda binds its arguments exactly as a def does. """ owner = {} scopes = [n for n in ast.walk(tree) if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda))] # Outermost first, so an inner scope overwrites the outer one's claim. scopes.sort(key=lambda n: (getattr(n, "lineno", 0), getattr(n, "col_offset", 0))) for fn in scopes: for node in ast.walk(fn): owner[id(node)] = fn return owner def _final_name(node): """The last identifier in a Name or Attribute, or None for anything else.""" if isinstance(node, ast.Name): return node.id if isinstance(node, ast.Attribute): return node.attr return None def _chain_root(node): """The leftmost Name in a chain of attributes, calls and subscripts.""" while True: if isinstance(node, ast.Attribute): node = node.value elif isinstance(node, ast.Subscript): node = node.value elif isinstance(node, ast.Call): node = node.func else: break return node.id if isinstance(node, ast.Name) else None def _targets(node): """Every Name a statement binds, whatever the binding form.""" found = [] for sub in ast.walk(node): if isinstance(sub, ast.Name) and isinstance(sub.ctx, (ast.Store, ast.Del)): found.append(sub.id) return found def _bound_in(fn): """Names this scope binds: parameters, assignments, loops, withs, imports.""" if fn is None: return set() names = set() args = getattr(fn, "args", None) if args is not None: for group in (args.args, args.posonlyargs, args.kwonlyargs): names.update(a.arg for a in group or []) for extra in (args.vararg, args.kwarg): if extra is not None: names.add(extra.arg) for node in ast.walk(fn): if isinstance(node, (ast.Assign, ast.AnnAssign, ast.AugAssign, ast.For, ast.AsyncFor, ast.comprehension, ast.NamedExpr)): names.update(_targets(node)) elif isinstance(node, (ast.With, ast.AsyncWith)): for item in node.items: if item.optional_vars is not None: names.update(_targets(item.optional_vars)) elif isinstance(node, ast.ExceptHandler) and node.name: names.add(node.name) elif isinstance(node, (ast.Import, ast.ImportFrom)): names.update((a.asname or a.name).split(".")[0] for a in node.names) elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): if node is not fn: names.add(node.name) return names def _bound_at_module(tree): """Names the file itself binds at any level outside a function body.""" names = set() for node in ast.walk(tree): if isinstance(node, (ast.Assign, ast.AnnAssign, ast.AugAssign)): names.update(_targets(node)) elif isinstance(node, (ast.Import, ast.ImportFrom)): names.update((a.asname or a.name).split(".")[0] for a in node.names) elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): names.add(node.name) return names # Reads that are external by their own name, whatever the surrounding file # does with them. `request` is werkzeug's, reached through odoo.http; the # context is whatever the caller put there; both parsers turn somebody else's # bytes into objects. EXTERNAL_ROOTS = frozenset(["request"]) EXTERNAL_CALLS = frozenset(["loads", "literal_eval", "parse_qs", "parse_qsl"]) CONTEXT_READS = ("_context", "context") def _is_external(node, fn, tree): """True when this expression's value came from outside the file. Two ways to qualify. Either the expression names a source that is external by construction -- `request`, a parsed payload, the context a caller handed in -- or its root name is one this file never binds, which is the same answer arrived at by elimination: a name that is not a parameter, not assigned, not a loop or with target, not imported and not a builtin is a value from somewhere this file cannot see. The elimination half is what makes the rules' own 'wrong' samples fire, because a sample is a fragment and its inputs are free names by definition. It is also what keeps them quiet on the four findings that were classified false: `file_config`, `filename`, `full_path` and `raw_value` are all bound a few lines above the join. """ for sub in ast.walk(node): if isinstance(sub, ast.Attribute) and sub.attr in CONTEXT_READS: return True if isinstance(sub, ast.Call): attr = _final_name(sub.func) if attr in EXTERNAL_CALLS: return True root = _chain_root(node) if root is None: return False if root in EXTERNAL_ROOTS: return True if root in ("self", "cls"): return False if root in dir(builtins): return False return root not in _bound_in(fn) and root not in _bound_at_module(tree) # Calls whose result is a whole number, so `== 0` on it is exact and the # float rule has nothing to say. search_count and len are counts by # definition; `.count()` is one on a recordset, a string and a list alike. INT_VALUED_CALLS = frozenset(["search_count", "count", "len", "bit_length", "index", "ord"]) _COUNT_KEY = re.compile(r"(?:^__count$|_count$)") def _integer_fields(tree): """Field names the file declares as fields.Integer.""" names = set() for node in ast.walk(tree): if not isinstance(node, ast.Assign) or not isinstance(node.value, ast.Call): continue func = node.value.func if isinstance(func, ast.Attribute) and func.attr == "Integer": names.update(_targets(node)) return names def _counts_rows(fn): """True when this scope runs a COUNT query, so a fetched cell is an int.""" if fn is None: return False for node in ast.walk(fn): if not (isinstance(node, ast.Call) and _final_name(node.func) == "execute"): continue for arg in node.args[:1]: try: text = ast.unparse(arg) except Exception: # pragma: no cover continue if "count(" in text.lower(): return True return False def _assigned_values(fn, name): """Values assigned to `name` in this scope, or None if it is bound some way the trace cannot read -- a loop target, a with, an unpacking.""" if fn is None: return None values = [] for node in ast.walk(fn): if isinstance(node, ast.Assign): for target in node.targets: if isinstance(target, ast.Name) and target.id == name: values.append(node.value) elif name in _targets(target): return None # unpacked: no single value elif isinstance(node, ast.AnnAssign): if isinstance(node.target, ast.Name) and node.target.id == name \ and node.value is not None: values.append(node.value) elif isinstance(node, (ast.For, ast.AsyncFor, ast.comprehension)): if name in _targets(node.target): return None elif isinstance(node, (ast.With, ast.AsyncWith)): for item in node.items: if item.optional_vars is not None \ and name in _targets(item.optional_vars): return None elif isinstance(node, ast.AugAssign): if isinstance(node.target, ast.Name) and node.target.id == name: values.append(node.value) return values or None def _int_valued(node, fn, tree, depth=0): """True when this expression is certainly a whole number. Certainly, not probably: every answer here SUPPRESSES a finding, so the unreadable cases must come back False and leave the finding standing. A name assigned twice is only an integer if both assignments are. """ if depth > 4: # pragma: no cover return False if isinstance(node, ast.Constant): return isinstance(node.value, int) and not isinstance(node.value, bool) if isinstance(node, ast.Call): if _final_name(node.func) in INT_VALUED_CALLS: return True return False if isinstance(node, ast.Subscript): # read_group hands back rows keyed '_count', and the key is # written as an f-string as often as a literal. key = node.slice if isinstance(key, ast.Constant) and isinstance(key.value, str): if _COUNT_KEY.search(key.value): return True if isinstance(key, ast.JoinedStr) and key.values: tail = key.values[-1] if isinstance(tail, ast.Constant) and isinstance(tail.value, str) \ and _COUNT_KEY.search(tail.value): return True # cr.fetchone()[0] of a SELECT COUNT(*) is the third spelling of a # count, and the query is the only place that says so. inner = node.value if isinstance(inner, ast.Call) and _final_name(inner.func) in ( "fetchone", "fetchall") and _counts_rows(fn): return True return False if isinstance(node, ast.BinOp) and isinstance(node.op, (ast.Add, ast.Sub, ast.Mult, ast.Mod, ast.FloorDiv)): return (_int_valued(node.left, fn, tree, depth + 1) and _int_valued(node.right, fn, tree, depth + 1)) if isinstance(node, ast.Attribute): return node.attr in _integer_fields(tree) if isinstance(node, ast.Name): if node.id in _integer_fields(tree): return True values = _assigned_values(fn, node.id) if not values: return False return all(_int_valued(v, fn, tree, depth + 1) for v in values) return False # ------------------------------------------------ the rewritten checkers _MONEY_NAME = re.compile(r"price|amount|qty|quantity|total|balance", re.I) _RESOLVES_THE_PATH = frozenset(["commonpath", "commonprefix", "relpath", "is_relative_to", "realpath", "abspath"]) def float_equality_against_zero(code): """odoo.float-comparison-needs-float-utils `== 0` or `!= 0` on a value whose name says it is money or a quantity, where the assignment does not say it is a whole number. The regex this replaces asked only the first half. Classified by hand it was 2 true and 5 false, and every one of the five compared an INTEGER to zero, where `==` is exact and the rule has nothing to say: `search_count(...)`, a read_group `_count`, `cr.fetchone()[0]` of a SELECT COUNT(*), and `x.qty` where the same file declares qty as fields.Integer. All five were named `total`, `group_total` or `total_population`, and so were both true positives -- `total = sum(sorted_amounts)` is the same spelling with a float behind it. The name could never have separated them. What it cannot see, and so still reports: a float that arrives as a parameter, and an integer from a helper in another file. Both leave the finding standing, which is the direction a suppressing trace has to err in. """ tree, _ = parse_python(code) if tree is None: return [] owner = _function_of(tree) findings, seen = [], set() for node in ast.walk(tree): if not isinstance(node, ast.Compare) or len(node.ops) != 1: continue if not isinstance(node.ops[0], (ast.Eq, ast.NotEq)): continue left, right = node.left, node.comparators[0] for value, other in ((left, right), (right, left)): if not (isinstance(other, ast.Constant) and isinstance(other.value, (int, float)) and not isinstance(other.value, bool) and other.value == 0): continue name = _final_name(value) if not name or not _MONEY_NAME.search(name): continue if _int_valued(value, owner.get(id(value)), tree): break if node.lineno in seen: break seen.add(node.lineno) findings.append(( node.lineno, "'{0}' is compared with == against zero -- binary floats do " "not land on it exactly, so use float_is_zero with the " "field's precision".format(name), None)) break return findings def path_join_takes_an_external_name(code): """python.path-join-does-not-contain-a-path os.path.join where one component came from outside the file, and the result is not resolved and compared before it is used. The regex this replaces matched component NAMES -- `filename`, `form`, `params` -- and was 0 true, 4 false. All four join a configured directory to a name the module itself produced: a timestamped backup filename, a literal from a list declared in the same function, a module path out of get_module_path. Not one was a fragment a caller could point out of the directory, which is the only thing the rule is about. So the question moved from what the component is called to where its value came from, and 'outside the file' is the answer _is_external gives. The rule's own wrong sample joins `request.args['name']` and still fires. This reports less than the regex did, on purpose: the regex's entire output on 294,000 lines was false, and a checker that cries wolf gets switched off. A traversal reached through a helper in another module is outside what a local trace can see and is not reported. """ tree, _ = parse_python(code) if tree is None: return [] owner = _function_of(tree) findings = [] for node in ast.walk(tree): if not (isinstance(node, ast.Call) and _final_name(node.func) == "join"): continue # os.path.join, not str.join and not pathlib's. parent = node.func.value if isinstance(node.func, ast.Attribute) else None if not (isinstance(parent, ast.Attribute) and parent.attr == "path"): continue fn = owner.get(id(node)) # The fix is to resolve and compare, and it is written beside the # join. Scoped to the enclosing function rather than the file: a # realpath somewhere else in a 900-line model says nothing about this # call. A join at module scope -- which is what a rule's own sample # is -- is scoped to the module for the same reason. scope = fn if fn is not None else tree if any(_final_name(c.func) in _RESOLVES_THE_PATH for c in ast.walk(scope) if isinstance(c, ast.Call)): continue for arg in node.args[1:]: if _is_external(arg, fn, tree): try: shown = ast.unparse(arg) except Exception: # pragma: no cover shown = "the component" findings.append(( node.lineno, "os.path.join is handed {0}, which came from outside this " "file -- an absolute component discards the base directory " "entirely and '..' walks out of it, so resolve both and " "compare before opening anything".format(shown), None)) break return findings # Modules whose presence in the imports makes a file an HTTP edge. Not # werkzeug: Odoo imports werkzeug.urls to build links in model files, and one # such file on core was the only thing a wider gate added. _HTTP_MODULES = ("fastapi", "pydantic", "odoo.http", "starlette") def _datetime_class_names(tree): """Names the datetime CLASS is bound to: `datetime`, plus any alias.""" names = {"datetime"} for node in ast.walk(tree): if isinstance(node, ast.ImportFrom) and node.module == "datetime": names.update(a.asname for a in node.names if a.name == "datetime" and a.asname) return names def _speaks_http(tree): for node in ast.walk(tree): if isinstance(node, ast.ImportFrom) and node.module: if node.module.startswith(_HTTP_MODULES): return True if node.module == "odoo" and any(a.name == "http" for a in node.names): return True elif isinstance(node, ast.Import): if any(a.name.startswith(_HTTP_MODULES) for a in node.names): return True return False class _NaiveNow(object): """Which expressions carry a naive now(), and as a time or as a date. The sources are the ones the regex matched -- datetime.now() and utcnow() with empty parentheses -- and nothing more. date.today() and fields.Date.today() are the same UTC date and were measured rather than added: 48 and 114 sites on OpenSPP, 31 and 217 on core. Adding them would turn a narrowing into a widening, and fields.Date.today() is the framework's own spelling of the value. """ def __init__(self, tree): self.names = _datetime_class_names(tree) self.owner = _function_of(tree) self.at = None def _is_source(self, node): if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute)): return False if node.args or node.keywords: return False if node.func.attr == "utcnow": return True return node.func.attr == "now" and _final_name(node.func.value) in self.names def carries(self, node, depth=0): """None, "datetime" or "date"; the source's line is left in self.at.""" if node is None or depth > 5: return None if self._is_source(node): self.at = node.lineno return "datetime" if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute): inner = self.carries(node.func.value, depth + 1) if node.func.attr == "date" and not node.args and inner: return "date" if node.func.attr == "replace" and inner: # .replace(tzinfo=...) is the fix; any other replace keeps it naive. return None if any(k.arg == "tzinfo" for k in node.keywords) else inner if node.func.attr == "get" and len(node.args) == 2: # vals.get("date", datetime.now()): the default is the value # whenever the caller did not supply one. return self.carries(node.args[1], depth + 1) return None if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add): return self.carries(node.left, depth + 1) or self.carries(node.right, depth + 1) if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Sub): # now - timedelta is a time; now - then is a duration. left = self.carries(node.left, depth + 1) return None if self.carries(node.right, depth + 1) else left if isinstance(node, (ast.BoolOp, ast.IfExp)): parts = node.values if isinstance(node, ast.BoolOp) else [node.body, node.orelse] for part in parts: found = self.carries(part, depth + 1) if found: return found return None if isinstance(node, ast.Name): for value in _assigned_values(self.owner.get(id(node)), node.id) or (): found = self.carries(value, depth + 1) if found: return found return None def serialised(self, node): """`x.isoformat()` or `str(x)` of a naive value: text with no offset.""" if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) \ and node.func.attr == "isoformat" and not node.args: return self.carries(node.func.value) if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) \ and node.func.id == "str" and node.args: return self.carries(node.args[0]) return None def _offset_written_by_hand(parent_of, node): """`f"{now.isoformat()}Z"` or `now.isoformat() + "Z"`: labelled UTC.""" up = parent_of.get(id(node)) while isinstance(up, ast.FormattedValue): up = parent_of.get(id(up)) tails = [] if isinstance(up, ast.JoinedStr): tails = [v.value for v in up.values if isinstance(v, ast.Constant)] elif isinstance(up, ast.BinOp) and isinstance(up.op, ast.Add) \ and isinstance(up.right, ast.Constant): tails = [up.right.value] return any(isinstance(t, str) and t.startswith(("Z", "+00")) for t in tails) def _names_a_date(key): """A vals key or domain field spelled as a date: date, date_*, *_date.""" if not (isinstance(key, ast.Constant) and isinstance(key.value, str)): return False k = key.value return "datetime" not in k and (k == "date" or k.startswith("date_") or k.endswith("_date")) def naive_time_leaves_or_dates_a_day(code): """python.naive-and-aware-datetimes-do-not-compare The two shapes that survived reading, and only those. The regex this replaces fired on every datetime.now() and utcnow(). Its premise was that a naive local time reaching the ORM is wrong by the server's offset, and inside Odoo there is no offset: `import odoo` pins TZ=UTC, and fields.Datetime.now() is datetime.now() itself (ARCHITECTURE 3.56). Re-read on that basis it was 3 true of 33 on OpenSPP and OCA and 0 of 15 sampled on core. What stands does not depend on the server clock: A. A naive timestamp LEAVING the system with no offset: its isoformat() anywhere, or the value itself as a keyword of a class being built (a pydantic schema, a response object) or in a dict the function returns, in a file that imports fastapi, pydantic, starlette or odoo.http. The consumer cannot tell the zone. Two consent API responses on OpenSPP, both commits recall credits the rule with, and the xades:SigningTime of two Spanish e-invoicing modules on core. B. The server's UTC DATE used as a calendar day: a .date() of it into a vals key or domain field spelled as a date, a Date field's default, or an attribute named for a date; and a naive now() itself into a key named exactly `date`, which Odoo truncates to UTC's day. OCA's purchase_unreconciled dates a write-off that way, so for a company ahead of UTC the early hours land on the day before. What it leaves alone on purpose: a comparison (the fiscal-year cron was a day ahead by design), a filename or label (display only), a JWT payload (PyJWT reads naive as UTC), an ISO string with a Z written after it, and a value that only meets other naive UTC values inside the ORM. A key ending in _date can be a Datetime field (stock's scheduled_date), which is why a naive now() into one is not reported and its .date() is -- and why l10n_in's demo invoices, `'invoice_date': datetime.now()` into a Date field, are missed. The isoformat() clause costs three findings OpenSPP's classification reads false: a generated-at line in two report scripts and a computed_at in a result dict. """ tree, _ = parse_python(code) if tree is None: return [] now = _NaiveNow(tree) parent_of = {} for node in ast.walk(tree): for child in ast.iter_child_nodes(node): parent_of[id(child)] = node returned = set() for fn in _functions(tree): for node in ast.walk(fn): if isinstance(node, ast.Return) and node.value is not None: returned.add(id(node.value)) if isinstance(node.value, ast.Name): returned.update(id(v) for v in _assigned_values(fn, node.value.id) or ()) findings, seen = [], set() def report(text): if now.at not in seen: seen.add(now.at) findings.append((now.at, text, None)) # An ISO string is text for somebody else to read, wherever it is built. # The HTTP gate alone lost two on core: the Spanish e-invoicing modules # write datetime.now().isoformat() into xades:SigningTime, an xsd:dateTime # that a reader with no offset takes as local time. for node in ast.walk(tree): if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) \ and node.func.attr == "isoformat" and now.serialised(node) \ and not _offset_written_by_hand(parent_of, node): report("isoformat() of a naive timestamp writes no offset, so whoever " "reads the text cannot tell which zone it is in -- write " "datetime.now(timezone.utc) at the edge") if _speaks_http(tree): for node in ast.walk(tree): if isinstance(node, ast.Call) and (_final_name(node.func) or "")[:1].isupper(): values = [k.value for k in node.keywords] where = "into {0}()".format(_final_name(node.func)) elif isinstance(node, ast.Dict) and id(node) in returned: values = [v for v in node.values if v is not None] where = "in a returned dict" else: continue for value in values: if (now.carries(value) or now.serialised(value)) \ and not _offset_written_by_hand(parent_of, value): report("a naive timestamp leaves {0} with no offset, so whoever " "reads it cannot tell which zone it is in -- write " "datetime.now(timezone.utc) at the edge".format(where)) for node in ast.walk(tree): if isinstance(node, ast.Dict): for key, value in zip(node.keys, node.values): if not _names_a_date(key): continue kind = now.carries(value) if kind == "date" or (kind == "datetime" and key.value == "date"): report("'{0}' is set to the server's UTC date -- for a company " "ahead of UTC that is yesterday in its early hours; " "fields.Date.context_today is the user's day".format(key.value)) elif isinstance(node, ast.Tuple) and len(node.elts) == 3 \ and _names_a_date(node.elts[0]): if now.carries(node.elts[2]) == "date": report("a domain on '{0}' compares against the server's UTC date " "rather than the user's -- fields.Date.context_today is " "the user's day".format(node.elts[0].value)) elif isinstance(node, ast.Call) and _final_name(node.func) == "Date": for kw in node.keywords: body = kw.value.body if isinstance(kw.value, ast.Lambda) else kw.value if kw.arg == "default" and now.carries(body): report("a Date field defaults to the server's UTC date -- " "fields.Date.context_today is the user's day") elif isinstance(node, ast.Assign) and len(node.targets) == 1 \ and isinstance(node.targets[0], ast.Attribute): attr = node.targets[0].attr if "datetime" not in attr and "date" in attr.split("_") \ and now.carries(node.value) == "date": report("'{0}' is set to the server's UTC date -- " "fields.Date.context_today is the user's day".format(attr)) return sorted(findings) def float_coercion_of_external_money(code): """python.decimal-for-money-not-float float() applied to a money value that arrived from outside the file. The regex this replaces matched the NAME either side of the coercion and was 0 true, 3 false, with a reason behind it rather than a sample size: Odoo's Monetary field IS a float -- the ORM defines it that way -- and the framework's answer to the precision problem is float_round and float_compare against the currency's rounding, not Decimal. Inside an Odoo module the rule fired on the idiom and named a fix that cannot be taken. Three findings do not settle a rule either way, so it is narrowed rather than culled, and the narrowing is the line the argument actually draws. `float(self.amount or 0.0)` is the ORM's own type and is not reported. `float(result)`, where result came back from a service in this file, is not reported. `float(payload['price'])`, where the string came from a caller and Decimal(payload['price']) IS available, still is -- which is the rule's own wrong sample and the one shape its remedy fits. """ tree, _ = parse_python(code) if tree is None: return [] owner = _function_of(tree) findings = [] for node in ast.walk(tree): if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "float" and node.args): continue value = node.args[0] name = _final_name(value) or _chain_root(value) or "" target = _MONEY_NAME.search(name) if not target: # The coerced expression may say nothing, while what it is # assigned to does: `price = float(payload['p'])`. holder = owner.get(id(node)) for stmt in ast.walk(holder if holder is not None else tree): if isinstance(stmt, ast.Assign) and stmt.value is node: target = any(_MONEY_NAME.search(t) for t in _targets(stmt) if t) break if not target: continue if not _is_external(value, owner.get(id(node)), tree): continue findings.append(( node.lineno, "a money value from outside this file is routed through float() " "-- the binary float is already wrong before Decimal can be given " "it, so build Decimal from the original string", None)) return findings class _Every(object): """A constraint is present and which fields it covers cannot be read.""" EVERY_FIELD = _Every() def _sql_identifiers(node): """Names inside a constraint's SQL text, or EVERY_FIELD when the text is not a literal. Keywords come along (CHECK, UNIQUE, NOT) and match no field.""" if isinstance(node, ast.Constant) and isinstance(node.value, str): return set(re.findall(r"[A-Za-z_][A-Za-z0-9_]*", node.value)) return EVERY_FIELD def _constrained_fields(tree, legacy_dead): """Field names some constraint in this file covers, or EVERY_FIELD. Three spellings: @api.constrains (its plain names -- a dotted one registers nothing), models.Constraint (the names in its SQL) and _sql_constraints (the names in each entry's SQL, and nothing at all when `legacy_dead`, because from 19 the registry never creates it). A constraint that is present but cannot be read -- @api.constrains(*FIELDS), a _sql_constraints inherited and extended -- covers every field, which is the reading this had before it looked at fields at all. """ covered = set() def add(names): if names is EVERY_FIELD: return True covered.update(names) return False for fn in _functions(tree): declared = _decorator_args(fn, "api", "constrains") if declared is UNREADABLE: return EVERY_FIELD if declared: covered.update(d for d in declared if "." not in d) for node in ast.walk(tree): if isinstance(node, ast.Call): func = node.func name = func.attr if isinstance(func, ast.Attribute) else \ func.id if isinstance(func, ast.Name) else None if name == "Constraint": if add(_sql_identifiers(node.args[0]) if node.args else EVERY_FIELD): return EVERY_FIELD continue if isinstance(node, (ast.Assign, ast.AugAssign, ast.AnnAssign)): targets = node.targets if isinstance(node, ast.Assign) else [node.target] if not any(isinstance(t, ast.Name) and t.id == "_sql_constraints" for t in targets) or node.value is None or legacy_dead: continue if not isinstance(node.value, (ast.List, ast.Tuple)): return EVERY_FIELD for entry in node.value.elts: if not (isinstance(entry, (ast.List, ast.Tuple)) and len(entry.elts) > 1): return EVERY_FIELD if add(_sql_identifiers(entry.elts[1])): return EVERY_FIELD return covered def _clears(value): """False, None, '' or an empty collection: what a form resets a field to.""" if isinstance(value, ast.Constant): return value.value is False or value.value is None or value.value == "" if isinstance(value, ast.Dict): return not value.keys return isinstance(value, (ast.List, ast.Tuple, ast.Set)) and not value.elts def _refuses_in_branch(fn, warning, watched): """Does the branch that returns this warning also take the value back? A warning dict on its own informs: "this reference already exists", "the rate is far from the previous one", "please choose another package type". The user saves anyway, from the form as from an import, so nothing is enforced anywhere and the rule's harm -- a check only the form applies -- is not there. What refuses is the same branch writing back a field the onchange watches (the quantity clamped, the partner cleared, the removed document restored) or clearing any field on the record (a registrant that does not fit the new request type). "The same branch" is the innermost if, for or while around the warning, or the whole body when there is none. """ parents = {} for node in ast.walk(fn): for child in ast.iter_child_nodes(node): parents[id(child)] = node branch = warning while branch is not fn and not isinstance(branch, (ast.If, ast.For, ast.AsyncFor, ast.While)): branch = parents.get(id(branch), fn) roots = set() if watched is UNREADABLE else {w.split(".")[0] for w in watched} aliases = _aliases(fn) def on_record(node): return isinstance(node, ast.Name) and aliases.get(node.id) == () for node in ast.walk(branch): if isinstance(node, (ast.Assign, ast.AugAssign)): targets = node.targets if isinstance(node, ast.Assign) else [node.target] for target in targets: if isinstance(target, ast.Attribute) and on_record(target.value) and ( watched is UNREADABLE or target.attr in roots or _clears(node.value)): return True elif (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr == "update" and on_record(node.func.value) and node.args and isinstance(node.args[0], ast.Dict)): for key, value in zip(node.args[0].keys, node.args[0].values): if isinstance(key, ast.Constant) and ( watched is UNREADABLE or key.value in roots or _clears(value)): return True return False def _raised_messages(scope): """The string literals inside every raise in this scope, long enough to name one check: `_()` and implicit concatenation are already one string.""" messages = set() for node in ast.walk(scope): if isinstance(node, ast.Raise): for inner in ast.walk(node): if isinstance(inner, ast.Constant) and isinstance(inner.value, str) \ and len(inner.value.strip()) >= 20: messages.add(inner.value.strip()) return messages def _check_helpers(scope, fields=None): """Names of the check-shaped methods this scope calls: _check_expiration_date, _validate_dates. A name is all there is to go on, so only those two words. With `fields`, only a call handed one of those fields counts: _check_expiration_date(self.expiration_date) checks the watched value, and a bare self._check_company() in the same onchange checks something else that write() may also happen to check. """ names = set() for node in ast.walk(scope): if not isinstance(node, ast.Call): continue name = _final_name(node.func) if not (name and ("check" in name.lower() or "validat" in name.lower())): continue if fields is not None and not any( isinstance(arg, ast.Attribute) and arg.attr in fields for arg in list(node.args) + [kw.value for kw in node.keywords]): continue names.add(name) return names def _server_methods(cls, fn): """Methods of this class the server runs whatever the client is: create(), write(), an @api.constrains method, and any method a field in the class names as its inverse=. Not action_post and friends: a check at posting lets a draft hold the value, which is the gap the rule is about.""" inverses = set() for node in ast.walk(cls): if isinstance(node, ast.Call): for keyword in node.keywords: if keyword.arg == "inverse" and isinstance(keyword.value, ast.Constant): inverses.add(keyword.value.value) return [m for m in cls.body if isinstance(m, (ast.FunctionDef, ast.AsyncFunctionDef)) and m is not fn and (m.name in ("create", "write") or m.name in inverses or _decorator_args(m, "api", "constrains") is not None)] def _server_repeats_the_check(tree, fn, watched): """Does a server-side method of the same class make the onchange's check? Two signals, and both have to be the SAME check rather than a similar one. The exact message the onchange raises, raised again: mrp.workorder's date_finished onchange refuses to unplan one work order, and _set_dates, the inverse of both date fields, raises the same sentence. Or the same check-shaped helper called from both: the API key wizard's onchange and its create() both call res.users.apikeys._check_expiration_date. Only the onchange's own class is searched, one call deep, because every yes here suppresses a finding. A check made at posting does not count (see _server_methods), and a paraphrased message is not the same check. """ cls = _owner_of(tree, fn) if cls is None or watched is UNREADABLE: return False messages = _raised_messages(fn) helpers = _check_helpers(fn, {w.split(".")[0] for w in watched}) if not messages and not helpers: return False for method in _server_methods(cls, fn): if messages & _raised_messages(method) or helpers & _check_helpers(method): return True return False def onchange_tries_to_enforce(code, facts=None): """odoo.onchange-is-not-validation An @api.onchange method that RAISES, or returns a warning while taking the value back -- it is trying to enforce an invariant the web client is the only thing that will apply. The regex this replaces read 400 characters after the decorator and could not see where the method ended. All three of its false positives are the same failure: a four-line onchange that assigns one field, followed by an action method that raises. `_onchange_variable_id` sets a target model and the UserError 200 characters below it belongs to `action_remap`. The window crossed the function boundary, and a regex has no way not to. Scoped to the decorated function, that shape is silent, and the four true positives -- an onchange that really does raise ValidationError, and ones that return the warning dict -- all stand. A constraint in the file clears it when it covers a field the onchange watches: an onchange beside a real @api.constrains on the same field is the faster feedback the rule explicitly allows. Until 2026-09-27 ANY constraint anywhere in the file cleared it, so a UNIQUE(reference) silenced an onchange that raised on credit_limit. _sql_constraints counts on 16 through 18 and on an unknown series, because it IS a constraint there, and not when the tree pins 19 or later, where the registry never creates it (_constrained_fields). Narrowing the guard alone added 36 findings and 11 read true, because the wide guard had been hiding a second problem: a returned warning dict was read as a refusal whatever it said. Most were notices -- "this reference already exists", "the rate is far from the previous one" -- that the user saves past in the form as easily as in an import, so nothing is enforced anywhere. A warning counts now only when its branch also takes the value back (_refuses_in_branch). Both together: OCA unchanged, OpenSPP +4 (3 true), core +11 (8 true) and -17, all 17 notices. ARCHITECTURE 3.67, 3.68. A check the server repeats in the same class also backs it: the same raised sentence, or the same check helper handed the watched field, in create(), write(), an inverse or an @api.constrains method (_server_repeats_the_check). Core 2,339 -> 2,338, the work order's unplanning check; nothing else moved. ARCHITECTURE 3.69. """ tree, _ = parse_python(code) if tree is None: return [] covered = _constrained_fields( tree, _series_at_least((facts or {}).get("odoo_series"), 19.0)) findings = [] for fn in _functions(tree): watched = _decorator_args(fn, "api", "onchange") if watched is None: continue if covered is EVERY_FIELD or (watched is UNREADABLE and covered): continue if watched is not UNREADABLE and any(w.split(".")[0] in covered for w in watched): continue if _server_repeats_the_check(tree, fn, watched): continue # Anchored on the decorator, not on the def below it. That is where # the rule is, where a reader looking the finding up will look, and # where the marker goes when somebody has already decided about it -- # a FunctionDef's own lineno is the `def`, one line past all three. at = fn.lineno for dec in fn.decorator_list: func = dec.func if isinstance(dec, ast.Call) else dec if isinstance(func, ast.Attribute) and func.attr == "onchange": at = dec.lineno break for node in ast.walk(fn): spot = None if isinstance(node, ast.Raise): exc = node.exc name = _final_name(exc.func) if isinstance(exc, ast.Call) \ else _final_name(exc) if name and ("Error" in name or "Warning" in name): spot = node.lineno elif isinstance(node, ast.Dict) and any( isinstance(key, ast.Constant) and key.value == "warning" for key in node.keys) and _refuses_in_branch(fn, node, watched): spot = node.lineno if spot is None: continue findings.append(( at, "@api.onchange '{0}' refuses a value, and onchange runs in " "the web client form only -- an import, an RPC call, a cron " "or a direct create() never triggers it, so the record enters " "the database unchallenged".format(fn.name), None)) break return findings # A query decides WHICH records you get to see, and dropping ir.rule widens # that set. Every one of the 6 hand-classified true positives is one of these. SUDO_QUERIES = ("search", "search_count", "search_read", "read_group") # These act on a recordset the caller already holds. Dropping ir.rule still # matters -- it governs write and read permission as well as visibility -- but # it widens what you may DO with a record rather than which records exist for # you, and nothing measured here has yet found a real one. SUDO_TOUCHES = ("create", "write", "unlink", "copy", "read") # The grade the write and escape clauses carry, per finding rather than per # rule. `unproven` is the corpus's word for a real defect nothing measured # here has met yet, and that is exactly their position: the read clause has # 6 hand-classified true positives across two trees, and these two have 0 in # 261,000 lines. Every one of the 23 they found was read, and every one was # an administrator by construction -- a demo-data seeder, an ACL-gated # accounting wizard -- or a framework model with no tenant boundary at all. # # Grading them rather than deleting them is the same call ยง3.46 made about # decimal-for-money: a clause the rule's own sentence asks for, measured at # zero, is reported and not trusted to stop a build. It costs the read # clause nothing, because a grade travels with the finding and not the rule. UNPROVEN = "unproven" def _escalates(call): """True when this `.sudo(...)` call turns record rules OFF. `sudo(False)` turns them back ON. It is how a method that must run under the caller's own rights says so, and odoo core does it 60 times -- `self = self.sudo(False)` in mail's `_check_attachments_access`, whose own docstring reads "This method relies on access rules/rights and therefore it should not be called from a sudo env." Reading it as an escalation reports the fix as the defect, which is worse than missing the defect: it is the one kind of finding that teaches a reader the checker cannot be trusted. An argument that is not a literal is unreadable here and counts as escalation, because `sudo(flag)` with a computed flag can be either and the finding should stand. """ if not (isinstance(call, ast.Call) and isinstance(call.func, ast.Attribute) and call.func.attr == "sudo"): return False given = list(call.args) + [k.value for k in call.keywords if k.arg == "flag"] for arg in given: if isinstance(arg, ast.Constant) and not arg.value: return False # `x.sudo(self.env.su)` sets su to whatever the environment already # has, so it cannot raise privilege above ambient -- it is how a # method hands a record back at the caller's own level. odoo core # writes it four times and once with the comment "Unsudo the invoice # after creation if not already sudoed". `sudo(self.env.su or # field.compute_sudo)` is NOT this: the `or` can turn it on, so only # the bare attribute is read as ambient. if isinstance(arg, ast.Attribute) and arg.attr == "su": return False return True def _under_sudo(node): """The receiver of `.sudo().(...)`, or None.""" if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute)): return None inner = node.func.value if isinstance(inner, ast.Call) and _escalates(inner): return inner.func.value return None # Framework models odoo core gives no record rule, so sudo() on them has nothing # to drop. Measured over every ir.rule record in odoo core's XML: the only ir.* # models with one are ir.default, ir.embedded.actions, ir.filters, ir.ui.view and # ir.ui.view.custom. These four are the registry: no company, no owner, no # tenant. They are named rather than derived because a tree under review rarely # contains base, so whether base rules them is otherwise UNKNOWN -- which is how # three sudo() searches on ir.model stayed live on OCA after the AST move. # # ir.attachment is deliberately NOT here. It has no record rule either, but its # own _search filters by the caller's access to res_model unless the env is su, # so sudo() on it drops a real access check under another name. RULELESS_REGISTRY_MODELS = frozenset([ "ir.model", "ir.model.fields", "ir.model.data", "ir.module.module", ]) # A domain leaf ties a search to held records only through these. `!=`, `<` # and `not in` select everything else, which is the opposite of scoping. _HELD_OPERATORS = frozenset(["=", "in", "child_of"]) def _chain_escalates(node): """Does any call along this chain turn record rules off?""" while isinstance(node, (ast.Attribute, ast.Subscript, ast.Call)): if isinstance(node, ast.Call) and _escalates(node): return True node = node.func if isinstance(node, ast.Call) else node.value return False def _chain_calls(node, name): while isinstance(node, (ast.Attribute, ast.Subscript, ast.Call)): if isinstance(node, ast.Call) and _final_name(node.func) == name: return True node = node.func if isinstance(node, ast.Call) else node.value return False # Calls that hand back records the caller could already see, or a subset of # them. Anything else on a `self` chain is a helper whose body is not read here, # and it may sudo inside: a controller's `self._get_search_countries()` rooted at # `self` said nothing about whose rules produced the countries. _RULE_PRESERVING_CALLS = frozenset([ "filtered", "filtered_domain", "mapped", "sorted", "with_context", "with_company", "exists", "search", "grouped", ]) def _only_rule_preserving_calls(node): while isinstance(node, (ast.Attribute, ast.Subscript, ast.Call)): if isinstance(node, ast.Call) and _final_name(node.func) not in _RULE_PRESERVING_CALLS: return False node = node.func if isinstance(node, ast.Call) else node.value return True def _held_recordset(node, fn, depth=0): """Is this a recordset the caller obtained under its own record rules? `self` and anything reached from it without sudo: `self.filtered(...)`, `self.journal_id`, `self.env.user.partner_id`, a plain `search`. A name counts when every assignment to it does, and a loop variable when what it loops over does. `browse(ids)` does not count: browse applies no rule, so it holds whatever ids it was handed, and `env.ref(...)` does not either, for the same reason. Anything unreadable is not held, because every True here suppresses a finding. """ if node is None or depth > 4 or _chain_escalates(node) or _chain_calls(node, "browse") or _chain_calls(node, "ref"): return False root = _chain_root(node) if root == "self": return _only_rule_preserving_calls(node) if root == "request": # The user's own records and company, never an arbitrary lookup. text = ast.unparse(node) return text.startswith(("request.env.user", "request.env.company")) if root is None or fn is None: return False values = _assigned_values(fn, root) if values: return all(_held_recordset(v, fn, depth + 1) for v in values) for loop in ast.walk(fn): if isinstance(loop, (ast.For, ast.comprehension)) \ and isinstance(loop.target, ast.Name) and loop.target.id == root: return _held_recordset(loop.iter, fn, depth + 1) return False def _held_ids(node, fn, depth=0): """Ids of held records: `x.id`, `x.ids`, a list of them, or a name bound to one.""" if depth > 4: return False if isinstance(node, (ast.List, ast.Tuple)): real = [e for e in node.elts if not (isinstance(e, ast.Constant) and e.value in (False, None))] return bool(real) and all(_held_ids(e, fn, depth + 1) for e in real) if isinstance(node, ast.Attribute) and node.attr in ("id", "ids"): return _held_recordset(node.value, fn, depth + 1) if isinstance(node, ast.Name) and fn is not None: values = _assigned_values(fn, node.id) return bool(values) and all(_held_ids(v, fn, depth + 1) for v in values) return False def _tied_to_held_records(call, fn): """Does a domain leaf in this call's arguments pin it to held records? `('employee_id', 'in', self.ids)` returns the contracts of employees the caller can already see, and `('journal_id', '=', self.journal_id.id)` the moves of one journal, which belongs to one company. That is the rule's advice followed in a spelling the rule-field test cannot read: 18 of the 44 findings the global-rule test added on core (ARCHITECTURE 3.60, 3.64). A name passed as the domain is followed to what it was assigned. """ exprs = [] for arg in list(call.args) + [k.value for k in call.keywords]: exprs.append(arg) if isinstance(arg, ast.Name): exprs.extend(_assigned_values(fn, arg.id) or ()) for expr in exprs: for leaf in ast.walk(expr): if isinstance(leaf, (ast.Tuple, ast.List)) and len(leaf.elts) == 3 \ and isinstance(leaf.elts[0], ast.Constant) \ and isinstance(leaf.elts[0].value, str) \ and isinstance(leaf.elts[1], ast.Constant) \ and leaf.elts[1].value in _HELD_OPERATORS \ and _held_ids(leaf.elts[2], fn): return True return False # Prefix operators of an Odoo domain and how many terms each one takes. _DOMAIN_ARITY = {"|": 2, "&": 2, "!": 1} def _anded_leaves(elts): """Leaves ANDed at the top level of a domain list, or None when the list is not a complete domain. A domain is prefix notation: `'|', a, b` is a OR b, and the terms left over at the top are ANDed. Only a leaf reached through `&` alone is guaranteed to hold for every record returned; one under `|` or `!` is not. A list with an operator short of terms, `['|'] + domain`, is a fragment whose meaning depends on what it is glued to, so it answers None. """ def term(i): if i >= len(elts): return None, [] op = elts[i] if isinstance(op, ast.Starred): return None, [] # a spliced list could be a fragment if isinstance(op, ast.Constant) and op.value in _DOMAIN_ARITY: j, kept = i + 1, [] for _ in range(_DOMAIN_ARITY[op.value]): j, inner = term(j) if j is None: return None, [] kept.extend(inner) return j, kept if op.value == "&" else [] return i + 1, [op] i, leaves = 0, [] while i < len(elts): i, inner = term(i) if i is None: return None leaves.extend(inner) return leaves def _is_published_leaf(node): return isinstance(node, (ast.Tuple, ast.List)) and len(node.elts) == 3 \ and all(isinstance(e, ast.Constant) for e in node.elts) \ and [e.value for e in node.elts] == ["website_published", "=", True] def _domain_keeps_published(expr, fn, depth=0): """Does every record this domain can return have website_published = True? Followed through the spellings a controller builds a domain in: a list literal, `a + b`, `list(a)`, `a.copy()`, and a name every plain assignment to which qualifies. `domain += [...]` and `domain.append(leaf)` only AND more terms on, so they keep it, unless what they add is a fragment; an `insert` can put an operator in front of the leaf, so it does not. Anything unreadable answers False, because every True suppresses a finding. """ if depth > 4: return False if isinstance(expr, (ast.List, ast.Tuple)): leaves = _anded_leaves(expr.elts) return leaves is not None and any(_is_published_leaf(x) for x in leaves) if isinstance(expr, ast.BinOp) and isinstance(expr.op, ast.Add): sides = (expr.left, expr.right) if any(isinstance(s, (ast.List, ast.Tuple)) and _anded_leaves(s.elts) is None for s in sides): return False return any(_domain_keeps_published(s, fn, depth + 1) for s in sides) if isinstance(expr, ast.Call): if isinstance(expr.func, ast.Name) and expr.func.id == "list" \ and len(expr.args) == 1: return _domain_keeps_published(expr.args[0], fn, depth + 1) if isinstance(expr.func, ast.Attribute) and expr.func.attr == "copy" \ and not expr.args: return _domain_keeps_published(expr.func.value, fn, depth + 1) return False if not isinstance(expr, ast.Name) or fn is None \ or _assigned_values(fn, expr.id) is None: return False plain = [] for node in ast.walk(fn): if isinstance(node, ast.Assign) and any( isinstance(t, ast.Name) and t.id == expr.id for t in node.targets): plain.append(node.value) elif isinstance(node, ast.AugAssign) and isinstance(node.target, ast.Name) \ and node.target.id == expr.id: added = node.value if not isinstance(node.op, ast.Add) or ( isinstance(added, (ast.List, ast.Tuple)) and _anded_leaves(added.elts) is None): return False elif isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) \ and isinstance(node.func.value, ast.Name) \ and node.func.value.id == expr.id: if node.func.attr == "insert": return False if node.func.attr in ("append", "extend") and any( isinstance(a, ast.Constant) for a in node.args): return False return bool(plain) and all(_domain_keeps_published(v, fn, depth + 1) for v in plain) def _public_website_route(fn): """Is this function an @http.route(..., auth='public', website=True)?""" if not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)): return False for dec in fn.decorator_list: if not isinstance(dec, ast.Call) or _final_name(dec.func) != "route": continue kw = {k.arg: k.value for k in dec.keywords if k.arg} auth, website = kw.get("auth"), kw.get("website") if isinstance(auth, ast.Constant) and auth.value == "public" \ and isinstance(website, ast.Constant) and website.value is True: return True return False def _published_on_a_public_page(call, fn): """Is this a public website page reading only what it publishes? website_customer's /customers and website_crm_partner_assign's /partners list, group and count published partners under sudo(), because the anonymous visitor's own rules would hide most of them. Publishing is the record owner's decision to show it to everybody, and on a model with website.published.multi.mixin, searching website_published = True inside a website request also applies the current website's domain (_search_website_published, website/models/mixins.py). 12 of the 44 false positives the global-rule test added on core were this (ARCHITECTURE 3.60, 3.65). Narrow on purpose: a read, the literal leaf ('website_published', '=', True) ANDed at the top of the domain, inside a route that is both public and a website page. A published flag compared to anything but True, an `is_published` leaf, which applies no website, or the same search in a model method, stays a finding: website_sale's set_sequence_up is that. """ if call.func.attr not in SUDO_QUERIES or not _public_website_route(fn): return False domain = call.args[0] if call.args else next( (k.value for k in call.keywords if k.arg == "domain"), None) return domain is not None and _domain_keeps_published(domain, fn) def sudo_drops_record_rules(code, facts=None): """odoo.sudo-bypasses-record-rules Three clauses, because the rule has three: scope sudo to the narrowest operation, re-apply company filtering by hand, and never pass a sudo recordset onward. 1. a read under sudo -- search, search_read, read_group, read 2. a write under sudo -- create, write, unlink, copy 3. a sudo recordset bound to a name and then handed to something else Only the first was checked. The `--harvest` clustering says what that cost: `.sudo(` is the largest miss cluster in the corpus's recall measurement at 70 missed against 6 caught, and 47 of those 70 are files where a checker fired somewhere else -- a rule aimed wrong rather than a rule that is missing. Counted on the OpenSPP tree, the clause that was checked has 116 call sites and the clause that was not has 136, so the unchecked half was the larger one. One of the misses the README names, *"remove sudo() when importing record"*, is a create the search-only checker could not see at all. ir.rule governs read, write, create and unlink alike, so the same record-rule trigger gates every clause: where the tree declares no rule that filters this model, sudo drops nothing and there is no finding. The trigger only ever SUPPRESSES. A model the walk could not name, a tree that was not walked, a partial walk -- each reads UNKNOWN and the finding stands, because "no rule was found" and "nobody looked" must not be the same answer on a critical rule. `nexa check` on one file therefore behaves exactly as it did before triggers existed. Clause 3 fires at the binding, not at the use. `partners = self.env['res.partner'].sudo()` is a recordset carrying an environment with su=True, and every call made on it downstream inherits that -- which is the whole reason the rule's sentence says not to pass one on. Reported where the escape happens, once, rather than at each later read. What it over-reported, measured: 14 of the 20 surviving read findings constrained the search to records the caller had already identified -- a group id, a partner id, an area -- which is the rule's advice followed in a spelling the rule did not recognise. Three predicates now read that without evaluating the domain: the model's own rule fields (see rescoped), a leaf tying the search to ids of records the caller holds under its own rules (_tied_to_held_records, ARCHITECTURE 3.64), and a public website page reading only published records (_published_on_a_public_page, 3.65). None touches the six OpenSPP true positives, which are unscoped sweeps. """ tree, _ = parse_python(code) if tree is None: return [] classes = [n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)] constants = _module_string_constants(tree) def rescoped(node, model): # Did this call re-apply the scoping the rule it bypasses is about? # # The old test was the literal word `company_id`, from the rule's own # correct sample. That is one spelling of one kind of rule, and the # docstring recorded what it cost: 14 of 20 surviving read findings # were scoped by a group, a partner or an area instead, and read as # unscoped. Every OpenSPP case model is governed by `case_worker_id` # or `case_id.case_worker_id` and carries no company rule at all. # # So the call is now compared against the fields the model's OWN # ir.rule filters on, which is not a guess about the domain -- it is # the domain, read off the tree that declares it. A create naming # `case_id` lands inside the caller's visibility because that is the # link the rule follows. company_id stays as the fallback for a model # whose rules this could not read, so nothing that was exempt before # stops being exempt now. # The ARGUMENTS only. Unparsing the whole call includes the receiver, # and the model name is part of it: `spp.group.membership` contains # the word `group`, which is one of that model's own rule fields, so # reading the whole chain exempted an unscoped sweep of every # membership in the database -- a finding classified true by hand. # # An argument that is a bare name is followed to what it was assigned, # because `create(vals)` otherwise hides every field it sets behind # four letters. The trace is the same local one the other checkers # use, and a name it cannot resolve contributes nothing. pieces = [] for arg in list(node.args) + [k.value for k in node.keywords]: try: pieces.append(ast.unparse(arg)) except Exception: # pragma: no cover continue if isinstance(arg, ast.Name): for value in _assigned_values(owner.get(id(node)), arg.id) or (): try: pieces.append(ast.unparse(value)) except Exception: # pragma: no cover pass call_src = " ".join(pieces) if _tied_to_held_records(node, owner.get(id(node))) \ or _published_on_a_public_page(node, owner.get(id(node))): return True def named(name): return re.search(r"\b" + re.escape(name.split(".", 1)[0]) + r"\b", call_src) is not None # Every GLOBAL rule, not any rule. Odoo ANDs the rules that carry no # groups for everybody and ORs group rules per audience, so naming # one field of one rule re-applies that rule and no other. # website_sale's set_sequence_up names website_published, the field of # product.template's portal and public rule, and was exempt while the # global company rule was dropped. Where the model has no global rule # the old test stands: any rule's field, then company_id. required = [fields for is_global, fields in (context.model_rule_sets(facts, model) if model else ()) if is_global and fields] if required: return all(any(named(name) for name in fields) for fields in required) fields = context.model_rule_fields(facts, model) if model else () for name in fields or ("company_id",): if named(name): return True return "company_id" in call_src owner = _function_of(tree) def model_of(receiver): # `self.env['x'].sudo()` names its model in the chain. `Plan.sudo()` # does not, and every one of the write clause's first findings on # OpenSPP's demo generators was that shape -- `Plan = # self.env['spp.case.plan']` a few lines up, then fifteen writes on # the name. Unresolved, the trigger reads UNKNOWN and cannot suppress; # followed through the assignment, it can answer. Every assignment to # the name must agree, or the answer is still None. model = _env_model_of(receiver) if model is not None or not isinstance(receiver, ast.Name): return model if receiver.id == "self": # `self` is the model the enclosing class defines or extends, when # one name says which: tracking_manager's `self.sudo().search()` # inside `_inherit = "ir.model"` is a search on ir.model. around = [c for c in classes if c.lineno <= receiver.lineno <= getattr(c, "end_lineno", c.lineno)] if around: return _own_model_name(around[-1], constants) values = _assigned_values(owner.get(id(receiver)), receiver.id) if not values: values = [v for n in ast.walk(tree) if isinstance(n, ast.Assign) for t in n.targets if isinstance(t, ast.Name) and t.id == receiver.id for v in (n.value,)] found = {_env_model_of(v) for v in values} return found.pop() if len(found) == 1 and None not in found else None def ruled_out(receiver): model = model_of(receiver) if model in RULELESS_REGISTRY_MODELS: return True, model return context.model_is_record_ruled(facts, model) == context.ABSENT, model findings, seen = [], set() def report(line, text, grade=None): if line not in seen: seen.add(line) findings.append((line, text, grade)) for node in ast.walk(tree): receiver = _under_sudo(node) if receiver is None: continue method = node.func.attr if method not in SUDO_QUERIES and method not in SUDO_TOUCHES: continue absent, model = ruled_out(receiver) if absent: continue if rescoped(node, model): continue where = " on {0}".format(model) if model else "" if method in SUDO_QUERIES: report(node.lineno, "sudo().{0}(){1} runs with record rules off -- ir.rule " "filtering, including multi-company isolation, does not " "apply and the result looks correct in single-company " "testing".format(method, where)) else: report(node.lineno, "sudo().{0}(){1} acts with record rules off -- ir.rule " "governs write, create, unlink and read as well as " "visibility, so this reaches a record the caller's own " "rules would not let it touch".format(method, where), UNPROVEN) # Clause 3: a sudo recordset given a name, then handed on. # # The bare name only, as an argument or as the whole returned value. # `return partners.mapped('name')` hands back a list of strings and the # elevated environment does not travel with it, while # `return partners.filtered(...)` does -- telling those apart means # knowing which recordset methods return recordsets, so neither is # reported. Clause 3 under-reports by construction, which is the right # direction for a new clause on a critical rule: the search clause's # first version fired 106 times for 6 real ones, and a flood is how a # checker gets switched off. for fn in _functions(tree): escapes = set() for node in ast.walk(fn): if isinstance(node, ast.Call): if _under_sudo(node) is None: escapes.update(a.id for a in node.args if isinstance(a, ast.Name)) escapes.update(k.value.id for k in node.keywords if isinstance(k.value, ast.Name)) elif isinstance(node, ast.Return) and isinstance(node.value, ast.Name): escapes.add(node.value.id) for node in ast.walk(fn): if not isinstance(node, ast.Assign) or not isinstance(node.value, ast.Call): continue call = node.value if not _escalates(call): continue bound = [t for t in _targets(node)] # `self = self.sudo()` rebinds the method's own receiver, and # every ORM call below it in the method inherits the elevated # environment without naming sudo anywhere. It is the escape # clause at its widest -- the recordset is not passed on, it # replaces the thing the whole method is written against. # # This is the miss the README names by hand. OCA server-tools # cbefaa511485, "[FIX] excel_import_export: remove sudo() when # importing record", deletes exactly this line and says what it # cost: "finding multiple records across all companies during # import when they have same record name. Previously, importing # with sudo could bypass multi-company record rules." # # Three sites in 260,000 lines, so it is narrow as well as real. # The same commit ADDS `self.sudo().env["ir.model.data"]`, a # deliberate and narrow elevation, which is the evidence for not # also flagging that shape. rebind = isinstance(call.func.value, ast.Name) \ and call.func.value.id in bound if rebind: report(node.lineno, "'{0}' is rebound to its own sudo form, so every ORM " "call below this line in the method runs with record " "rules off -- including ones that name no sudo at all" .format(call.func.value.id)) continue if not any(name in escapes for name in bound): continue absent, model = ruled_out(call.func.value) if absent: continue where = " of {0}".format(model) if model else "" report(node.lineno, "this sudo recordset{0} is passed on -- the elevated " "environment travels with it, so every read and write made " "on it downstream also runs with record rules off" .format(where), UNPROVEN) return sorted(findings) def _env_model_of(node): """The model name at the head of a chained call, or None. Resolves `self.env["res.partner"]` and `env["res.partner"]` through any number of intervening attributes. A head that is a plain name -- the `Partner` in `Partner.sudo().search(...)` -- is not resolvable without following the assignment, so it returns None and the caller treats the record-rule question as unanswered rather than answered no. """ while isinstance(node, (ast.Call, ast.Attribute)): node = node.func if isinstance(node, ast.Call) else node.value if isinstance(node, ast.Subscript) and isinstance(node.slice, ast.Constant) \ and isinstance(node.slice.value, str): target = node.value name = target.attr if isinstance(target, ast.Attribute) else ( target.id if isinstance(target, ast.Name) else None) if name == "env": return node.slice.value return None CHECKS = { "handler_order_shadows_a_subclass": handler_order_shadows_a_subclass, "sudo_drops_record_rules": sudo_drops_record_rules, "float_equality_against_zero": float_equality_against_zero, "path_join_takes_an_external_name": path_join_takes_an_external_name, "float_coercion_of_external_money": float_coercion_of_external_money, "onchange_tries_to_enforce": onchange_tries_to_enforce, "check_company_missing_on_company_owned_relation": check_company_missing_on_company_owned_relation, "commit_inside_a_loop": commit_inside_a_loop, "depends_names_unread_fields": depends_names_unread_fields, "sql_built_by_string_formatting": sql_built_by_string_formatting, "constrains_lists_every_field_read": constrains_lists_every_field_read, "constrains_declares_a_dotted_name": constrains_declares_a_dotted_name, "depends_covers_compute_reads": depends_covers_compute_reads, "ondelete_leaves_a_dangling_read": ondelete_leaves_a_dangling_read, "prefer_sql_constraint_applies": prefer_sql_constraint_applies, "parent_can_form_a_cycle": parent_can_form_a_cycle, "editable_record_without_noupdate": editable_record_without_noupdate, "naive_time_leaves_or_dates_a_day": naive_time_leaves_or_dates_a_day, "legacy_sql_constraints_on_19": legacy_sql_constraints_on_19, } # Checkers that read whole-tree facts as well as the file in front of them. A # set rather than signature introspection: which checkers are conditional on the # tree is a design fact worth being able to read off one line, and a checker that # grew a parameter by accident would otherwise start receiving one silently. CONTEXT_CHECKS = frozenset([ "ondelete_leaves_a_dangling_read", "check_company_missing_on_company_owned_relation", # Reads models_record_ruled: whether sudo drops anything depends on whether # the tree declares a rule that filters. "sudo_drops_record_rules", # Reads odoo_series: whether a _sql_constraints block is a constraint at # all depends on which Odoo is going to load it. "prefer_sql_constraint_applies", # Reads odoo_series, and whether a tree was walked at all: whether a # _sql_constraints block exists in the database depends on the series. "legacy_sql_constraints_on_19", # Reads odoo_series: a _sql_constraints block backs an onchange only on a # series that creates it. "onchange_tries_to_enforce", # Reads the ORM facts: whether a compute's field is stored, which # @api.depends Odoo merges into the method, and what a declared computed # field depends on in turn. A gap checker as well. "depends_covers_compute_reads", ]) # Checkers that can meet input they cannot decide about -- a decorator whose # arguments are computed -- and say so instead of guessing. Same reasoning as # CONTEXT_CHECKS: which checkers have a second channel is a design fact, and one # that grew a parameter by accident should not start receiving one silently. GAP_CHECKS = frozenset([ "constrains_lists_every_field_read", "constrains_declares_a_dotted_name", "depends_covers_compute_reads", ]) def run_check(name, code, facts=None, gaps=None): """Findings from one named checker. Unknown name raises, never passes. A renamed checker that silently returned 'clean' would turn a critical rule into a permanent false pass, which is the exact failure the corpus exists to prevent. corpus.py validate calls this for every rule carrying a semantic_check, so a typo fails the build. `gaps` collects what a checker could not decide -- a method whose decorator is computed rather than written out. Not findings: nothing is wrong with @api.constrains(*FIELDS), it is simply outside a static pass. None means the caller is not collecting them. `facts` is context.tree_facts for the tree this file came from, or None. The checkers in CONTEXT_CHECKS read a trigger off it; None means no tree was walked and every trigger reads UNKNOWN, which leaves those checkers behaving exactly as they did before triggers existed. Returns (line, message, grade) throughout. `grade` is a likelihood the finding earned on its own evidence -- a trigger that fired -- or None to keep the grade its rule already carries. Normalised here so the six checkers with no opinion about it do not each have to return a third element saying so. """ if name not in CHECKS: raise KeyError("no semantic checker named '{0}'".format(name)) if name in CONTEXT_CHECKS and name in GAP_CHECKS: found = CHECKS[name](code, facts, gaps=gaps) elif name in CONTEXT_CHECKS: found = CHECKS[name](code, facts) elif name in GAP_CHECKS: found = CHECKS[name](code, gaps=gaps) else: found = CHECKS[name](code) return [hit if len(hit) == 3 else (hit[0], hit[1], None) for hit in found] # ------------------------------------------------- imports and symbol checks # Symbols a generated Odoo or Python snippet uses constantly and cannot use # without an import. Curated rather than derived: a general undefined-name pass # over a method-shaped fragment flags the surrounding class it was lifted from # and buries the real finding under it. NEEDS_IMPORT = { "ValidationError": "odoo.exceptions", "UserError": "odoo.exceptions", "AccessError": "odoo.exceptions", "MissingError": "odoo.exceptions", "api": "odoo", "fields": "odoo", "models": "odoo", "float_compare": "odoo.tools", "float_is_zero": "odoo.tools", "float_round": "odoo.tools", "timedelta": "datetime", "Decimal": "decimal", "relativedelta": "dateutil.relativedelta", } def imported_names(tree): """Every name the module binds by importing it, aliases resolved.""" names = set() for node in ast.walk(tree): if isinstance(node, ast.Import): for alias in node.names: names.add(alias.asname or alias.name.split(".")[0]) elif isinstance(node, ast.ImportFrom): for alias in node.names: names.add(alias.asname or alias.name) return names def imported_modules(tree): """Every module a tree reaches for, by its real name, aliases seen through. Deliberately not imported_names(). That one answers "what does this module BIND", which is what resolving an undefined name needs. An allowlist needs "what does this REACH FOR", and the two disagree in both directions: from decimal import Decimal binds Decimal, reaches decimal import os as math binds math, reaches os A relative import resolves to a package this snippet is not part of, so it is reported as "." and can never match an allowlist entry. """ modules = set() for node in ast.walk(tree): if isinstance(node, ast.Import): for alias in node.names: modules.add(alias.name.split(".")[0]) elif isinstance(node, ast.ImportFrom): if node.level: modules.add(".") elif node.module: modules.add(node.module.split(".")[0]) return modules def bound_names(tree): """Names bound by assignment, def, class, except, or a for/with target. A snippet that defines its own 'fields' helper is not missing an import. """ names = set() for node in ast.walk(tree): if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): names.add(node.name) elif isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store): names.add(node.id) elif isinstance(node, ast.ExceptHandler) and node.name: names.add(node.name) elif isinstance(node, ast.arg): names.add(node.arg) return names def is_lifted_fragment(tree, notes): """True when the block is a method shown without the class it belongs to. WHY THIS GATES THE IMPORT CHECK: a fragment like that was never going to carry an import header. The imports live in the module the answer quoted from, which was not part of the answer. Reporting them as missing is the same category of noise that the undefined-name analysis already declines to report -- and it is worse than useless, because a medium finding is enough to turn a correct answer's verdict into WARN and bury whatever else the run found. Two signals, either one sufficient: parse_fragment had to repair the block to make it parse at all (notes), or a function takes self or cls, which no module-level function does. """ if notes: return True for node in ast.walk(tree): if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): args = list(node.args.posonlyargs) + list(node.args.args) if args and args[0].arg in ("self", "cls"): return True return False def missing_imports(tree): """Curated symbols used without an import, as (line, message). Catches the failure the prompt explicitly asks the model to avoid -- 'Code first: runnable, imports included' -- and which a 7B drops routinely once the answer is long enough for that instruction to fall out of attention. """ have = imported_names(tree) | bound_names(tree) findings, seen = [], set() for node in ast.walk(tree): if not isinstance(node, ast.Name) or not isinstance(node.ctx, ast.Load): continue if node.id in have or node.id in seen or node.id not in NEEDS_IMPORT: continue seen.add(node.id) findings.append((node.lineno, "'{0}' is used but never imported (expected: from {1} import {0})" .format(node.id, NEEDS_IMPORT[node.id]))) return findings def unresolvable_imports(tree): """Imported modules that are not installed here, as (line, message). Implementation verification rather than plausibility: find_spec asks the interpreter whether the module actually exists on this machine, which is a different and stronger question than whether the import line looks reasonable. odoo is skipped. It is absent from this environment by design -- Nexa runs beside Odoo projects, not inside one -- so reporting it would be noise on every single Odoo answer. """ findings = [] for node in ast.walk(tree): if isinstance(node, ast.Import): modules = [a.name for a in node.names] elif isinstance(node, ast.ImportFrom): modules = [node.module] if node.module and not node.level else [] else: continue for module in modules: root = module.split(".")[0] if root in ("odoo", "openerp"): continue try: found = importlib.util.find_spec(root) is not None except (ImportError, ValueError, ModuleNotFoundError): found = False if not found: findings.append((node.lineno, "module '{0}' is not installed here, so the import " "cannot be verified".format(root))) return findings # ------------------------------------------------------- comment stripping def _docstring_starts(code): """Character offsets where a docstring literal opens. A docstring is prose, and a regex cannot tell prose from code -- which is the whole reason comments are stripped before matching. String literals were left standing, and classifying 50 findings by hand found the cost: two of them were `eval()` named in a docstring in a sentence arguing AGAINST using it, and a `datetime.now()` inside a doctest example. Both read as live code. Only docstrings, not every string. A triple-quoted SQL query is data the checkers have a legitimate interest in, and `_SQL_COLUMNS` reads one. The statement-level bare string an AST calls a docstring is the one place the file is guaranteed to be talking ABOUT code rather than in it. Offsets, not spans: the scanner already knows how to find a literal's end, and asking it to do that is one way of measuring the string rather than two. `col_offset` is a byte offset, which parts company with a character offset on a line holding non-ASCII -- so the result is checked against the quote it claims to point at and dropped when it does not, leaving that docstring intact rather than blanking the wrong run of characters. """ tree, _ = parse_python(code) if tree is None: return frozenset() line_start, offset = [], 0 for row in code.splitlines(True): line_start.append(offset) offset += len(row) starts = set() holders = (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef) for node in ast.walk(tree): if not isinstance(node, holders): continue body = getattr(node, "body", None) if not body: continue first = body[0] if not (isinstance(first, ast.Expr) and isinstance(first.value, ast.Constant) and isinstance(first.value.value, str)): continue lineno = getattr(first.value, "lineno", 0) if not 0 < lineno <= len(line_start): continue at = line_start[lineno - 1] + first.value.col_offset # The guard the docstring above describes. A prefix of anything but # indentation is possible -- `def f(): "doc"` -- but it is ASCII in # every spelling that reaches here, so a miss means the offset is # wrong and the safe answer is to leave the literal alone. if at < len(code) and code[at] in ("'", '"'): starts.add(at) return frozenset(starts) def _blanked(text): """A string literal with its contents emptied, quotes and shape intact. Same length and the same newlines, so the line AND column a finding reports are the ones a person reading the file will see, and so a docstring that is the whole body of a function still leaves that function syntactically intact for anything that parses the stripped view. """ if len(text) < 2: return text quote = 3 if text[:3] in ("'''", '"""') else 1 head, body, tail = text[:quote], text[quote:-quote or None], text[-quote:] if len(text) < quote * 2: return text return head + "".join("\n" if c == "\n" else " " for c in body) + tail def strip_comments(code, kind="python"): """Code with its comments and docstrings removed, strings left intact. WHY: a violation_pattern and a satisfied_pattern are regexes, and a regex cannot tell code from prose. The adversarial counterexample probe put a number on what that costs -- all 12 satisfied_patterns in the corpus could be cleared by a COMMENT quoting the fix rather than applying it. Four of those were critical rules, including odoo.onchange-is-not-validation: write the onchange, add '# _sql_constraints = [', and the checker reported clean while the bug shipped. It cuts the other way too. A commented-out @api.onchange was being reported as a live violation, which is the false positive that teaches people to ignore a checker. A docstring is the same argument wearing a different quote. It was left standing here until classifying 50 findings by hand turned up two that were prose read as code, so docstrings are now emptied and left in place -- see _docstring_starts for why only docstrings and not every literal. Newlines inside removed comments are preserved so that multi-line patterns still see the line structure they were written against. """ out = [] i, n = 0, len(code) triple = ("'''", '"""') docstrings = _docstring_starts(code) if kind == "python" else frozenset() while i < n: ch = code[i] if kind == "python" and code[i:i + 3] in triple: marker = code[i:i + 3] end = code.find(marker, i + 3) end = n if end < 0 else end + 3 out.append(_blanked(code[i:end]) if i in docstrings else code[i:end]) i = end continue if ch in ("'", '"') or (kind == "js" and ch == "`"): j = i + 1 while j < n: if code[j] == "\\": j += 2 continue if code[j] == ch: j += 1 break if code[j] == "\n" and kind == "python": break # unterminated single-quoted string j += 1 out.append(_blanked(code[i:j]) if i in docstrings else code[i:j]) i = j continue # XML comments are stripped for both kinds: an Odoo answer mixes Python # and view XML in the same block often enough to matter. if code[i:i + 4] == "", i + 4) end = n if end < 0 else end + 3 out.append("\n" * code[i:end].count("\n")) i = end continue if kind == "python" and ch == "#": end = code.find("\n", i) i = n if end < 0 else end continue if kind == "js" and code[i:i + 2] == "//": end = code.find("\n", i) i = n if end < 0 else end continue if kind == "js" and code[i:i + 2] == "/*": end = code.find("*/", i + 2) end = n if end < 0 else end + 2 out.append("\n" * code[i:end].count("\n")) i = end continue out.append(ch) i += 1 return "".join(out) def looks_like_markup(code): """Is this a view file rather than source? Decides the comment syntax a mutation may safely insert.""" stripped = code.lstrip() return stripped.startswith("<") and not stripped.startswith("<<") # ------------------------------------------------ JavaScript / TypeScript _JS_OPEN = {"(": ")", "[": "]", "{": "}"} _JS_CLOSE = {")": "(", "]": "[", "}": "{"} # A slash starts a regex literal only where a value cannot already be complete. # The standard heuristic; wrong for a few legal programs, and documented as a # heuristic rather than sold as a parser. _REGEX_OK_AFTER = set("(,=:[!&|?{};+-*%~^<>") | {"\n", "\r", ""} # The other half of the same heuristic, and the half this was missing. A slash # after one of these words is a regex too, because none of them can END a value: # `return /re/`, `case /re/`, `typeof /re/`. Read as division instead, the regex # BODY was scanned as code, so a character class holding one bracket -- /[(]/, # /]/, /{/ -- was reported as an unbalanced delimiter in a file that is fine. # Only bites when the class is unbalanced, which is why /^[^@]+@[^@]+$/ survived # it: its brackets happen to pair. _REGEX_OK_AFTER_WORD = frozenset([ "return", "typeof", "instanceof", "in", "of", "new", "delete", "void", "throw", "case", "do", "else", "yield", "await", ]) def balance_js(code): """Delimiter balance for JS/TS, aware of strings, comments and templates. NOT a parser. There is no JavaScript grammar in the standard library, so this answers one question only: does every bracket that opens also close, in the right order, outside a string or comment? That catches the truncated answer and the run-together answer -- the two ways generated code most often fails to be code at all -- and says nothing about anything else. A caller wanting a real parse has to shell out to a toolchain, which this project deliberately does not require. The layer above records that gap as UNKNOWN instead of letting a balanced file read as a clean parse. """ stack, findings = [], [] i, line, n = 0, 1, len(code) while i < n: ch = code[i] nxt = code[i + 1] if i + 1 < n else "" if ch == "\n": line += 1 i += 1 continue if ch == "/" and nxt == "/": while i < n and code[i] != "\n": i += 1 continue if ch == "/" and nxt == "*": i += 2 while i < n - 1 and not (code[i] == "*" and code[i + 1] == "/"): if code[i] == "\n": line += 1 i += 1 i += 2 continue if ch in ("'", '"'): i, line = _skip_quoted(code, i, line, ch) continue if ch == "`": i, line = _skip_template(code, i, line) continue if ch == "/" and _starts_a_regex(code, i): i, line = _skip_regex(code, i, line) continue if ch in _JS_OPEN: stack.append((ch, line)) elif ch in _JS_CLOSE: if not stack: findings.append((line, "closing '{0}' with nothing open".format(ch))) elif stack[-1][0] != _JS_CLOSE[ch]: opener, opened_at = stack.pop() findings.append((line, "'{0}' closes a '{1}' opened on line {2}".format( ch, opener, opened_at))) else: stack.pop() i += 1 for opener, opened_at in stack: findings.append((opened_at, "'{0}' opened here is never closed".format(opener))) return findings def _starts_a_regex(code, i): """Is the slash at i opening a regex literal rather than dividing?""" token = _prev_significant(code, i) return token in _REGEX_OK_AFTER or token in _REGEX_OK_AFTER_WORD def _prev_significant(code, i): """The previous significant character -- or the whole WORD it ends. One character cannot tell `return` from an identifier ending in n, and that is exactly the difference between a regex and a division. Returning the word lets the caller ask the question it actually has. """ j = i - 1 while j >= 0 and code[j] in " \t": j -= 1 if j < 0: return "" if not (code[j].isalnum() or code[j] in "_$"): return code[j] end = j + 1 while j >= 0 and (code[j].isalnum() or code[j] in "_$"): j -= 1 return code[j + 1:end] def _skip_quoted(code, i, line, quote): i += 1 while i < len(code): if code[i] == "\\": i += 2 continue if code[i] == quote: return i + 1, line if code[i] == "\n": line += 1 # unterminated; balance reports the damage after i += 1 return i, line def _skip_template(code, i, line): """Template literals nest. Backtick-dollar-brace bodies are scanned as code, because a generated validation message routinely interpolates another template inside itself and the braces have to balance there too.""" i += 1 while i < len(code): if code[i] == "\\": i += 2 continue if code[i] == "`": return i + 1, line if code[i] == "\n": line += 1 if code[i] == "$" and i + 1 < len(code) and code[i + 1] == "{": depth = 1 i += 2 while i < len(code) and depth: if code[i] == "{": depth += 1 elif code[i] == "}": depth -= 1 elif code[i] == "\n": line += 1 elif code[i] == "`": i, line = _skip_template(code, i, line) continue i += 1 continue i += 1 return i, line def _skip_regex(code, i, line): i += 1 in_class = False while i < len(code): if code[i] == "\\": i += 2 continue if code[i] == "[": in_class = True elif code[i] == "]": in_class = False elif code[i] == "/" and not in_class: return i + 1, line elif code[i] == "\n": return i, line + 1 # a regex cannot span lines: it was division i += 1 return i, line if __name__ == "__main__": import json import sys if len(sys.argv) > 2 and sys.argv[1] == "check": print(json.dumps(run_check(sys.argv[2], sys.stdin.read()), indent=2)) else: print(__doc__) print("checkers: " + ", ".join(sorted(CHECKS)))