Skip to content

Fix PythonRstNode.__contains__ returning a match index instead of a bool - #248

Merged
pjljvandelaar merged 2 commits into
mainfrom
fix_contains_match_result
Oct 5, 2026
Merged

pjljvandelaar merged 2 commits into
mainfrom
fix_contains_match_result

Conversation

@pjljvandelaar

Copy link
Copy Markdown
Collaborator

Problem

__contains__ returned the raw result of find_in_list, which is an index, not a boolean:

return find_in_list(self.children, item)

find_in_list returns the end index of the match, or a negative sentinel when there is none. Python coerces whatever __contains__ returns with bool(), so the in operator was inverted at both ends of the range:

situation find_in_list item in node correct
no match -2 True False
match ending at index 0 0 False True
match ending at index >= 1 n True True

So x in node answered "yes" for everything that is not there, and "no" for a match on the first child.

How it surfaced

Adding the missing -> bool return annotation (ANN204) made pyright report:

src/renaissance/integrations/python/ast/rst_node.py:292
  Type "Unknown | Literal[-2]" is not assignable to return type "bool"  (reportReturnType)

The magic -2

find_in_list had a bare -2 as its "not found" value, while the module already defines sentinels on lines 14-15:

MIS_MATCH = -12
INCOMPLETE_MATCH = -11

-2 is not one of them and carried no meaning — every caller only tests the sign (found_position >= 0 in match_pattern, less_than(0) in test_match_tree). It is replaced by MIS_MATCH, which is what the function means and is already the sentinel find_variants uses internally for a rejected variant. No caller or test depends on the literal -2 (verified by search).

Test correction

test_is_match_all_stmt passed only because of the bug: find_in_list returned -2, and -2 is truthy.

match_all = self.pattern_factory.create("$$pa")
assert_that(match_all.node, is_in(atu))

Two things were wrong. create() wraps the pattern in a module, so .node was the Module translation unit (pattern.py), not a statement — it could never match a statement in atu. And pattern_kind is resolved on the PythonPattern wrapper, not on the inner .node, so even create_statement(...).node is not recognised as MATCH_ALL:

expression parser_kind pattern_kind find_in_list
create("$$pa") Module None -2
create("$$pa").node Module None -2
create_statement("$$pa").node Expr None -2
create_statement("$$pa") Expr match_all 3

The test now uses the form that actually expresses its intent, and passes for the right reason — a $$pa wildcard matches the whole 4-statement module, end index 3.

Changes

  • src/renaissance/integrations/python/ast/rst_node.py — return find_in_list(self.children, item) >= 0
  • src/renaissance/syntax_tree/match_finder.py — return -2 → return MIS_MATCH, docstring updated
  • test/python/ast/test_patternic_style.py — assert on the pattern, not on its .node

Verification

  • Full suite before and after: 143 failed / 2209 passed / 91 skipped / 31 xfailed — identical failing set (the failures are pre-existing clang issues)
  • ruff check clean, ruff format --check clean
  • lint_budget --check green; removes the reportReturnType regression

@pjljvandelaar
pjljvandelaar merged commit a634bed into main Oct 5, 2026
10 checks passed
@pjljvandelaar
pjljvandelaar deleted the fix_contains_match_result branch October 5, 2026 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant