Collector: dispatch on parsed statement type, not first token#67
Merged
Conversation
_collect_checkable_statements decided whether a line was checkable by testing whether its first token was a verb. starting "<date>" and until "<date>" are TokenType.CONNECTIVE, and inherited is TokenType.OPERATOR -- both consumed statement-initial, before the verb, by _parse_one_operation (parser.py). A first-token-VERB guard therefore silently dropped every temporally-prefixed or inherited-prefixed statement: no error, no warning, just absent from the collected list, with check_source still reporting encodable=True -- the headline result concealed the under-reporting. Found by the Halverson re-run (liminate-dev PR #92): four statements across three documents excluded from every check, combined checked=17 where it should have been 21. inherited shares the identical defect and was never exercised by any corpus -- fixed here too, not left for a second finding. Inverts the collector: tokenize -> reorder -> parse every line (as before), then dispatch on the resulting AST node's type -- the same pattern check_agreement already uses downstream via _VERB_NAMES and _iter_statements. DefineNode registers into predicate_names before statements.append, preserving the forward-declaration order exactly. run._collect_deontic_statements, check_agreement, the encoder, and the seven checks are untouched -- this changes collection only. Adds a hand-written coverage table (test_collector_coverage_matches_ parser_accepted_shapes) asserting the collector checks exactly one statement of the expected type for every deontic verb, define, and value-form remember, under every prefix combination the parser accepts. Deliberately not derived from parser.py internals -- a specification of intended coverage, not a mirror of the implementation, so a future grammar addition without a matching row here is a gap the test can still expose once someone notices. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_collect_checkable_statementsdecided whether a line was checkable by testing whether its first token was a verb.starting "<date>"anduntil "<date>"areTokenType.CONNECTIVE, andinheritedisTokenType.OPERATOR— both consumed statement-initial, before the verb, by_parse_one_operation(parser.py). A first-token-VERB guard therefore silently dropped every temporally-prefixed orinherited-prefixed statement: no error, no warning, just absent from the collected list — withcheck_sourcestill reportingencodable=True, concealing the under-reporting.Found by the Halverson re-run (liminate-dev PR #92): four statements across three documents excluded from every check,
combined checked=17where it should have been 21.inheritedshares the identical defect and was never exercised by any corpus — fixed here too, not left for a second finding.Changes
check_agreementalready uses downstream via_VERB_NAMESand_iter_statements.DefineNoderegisters intopredicate_namesbeforestatements.append, preserving the forward-declaration order exactly.run._collect_deontic_statements,check_agreement, the encoder, and the seven checks are untouched — this changes collection only._CHECK_DEONTIC_VERBSconstant andTokenTypeimport.D3 — a guard against the class
Added a hand-written coverage table (
test_collector_coverage_matches_parser_accepted_shapes, 42 parametrized cases) asserting the collector checks exactly one statement of the expected type for every deontic verb,define, and value-formremember, under every prefix combination the parser accepts (bare,starting,until,starting+until,inherited,starting+inherited, full stack). Deliberately not derived fromparser.pyinternals — a specification of intended coverage, not a mirror of the implementation, so a future grammar addition without a matching row here is a gap the test can still expose once someone notices, not one it silently absorbs.Test plan
starting-prefixedforbidreturnschecked=1, not0until,starting+until,inherited, and the full canonical stackunless_swallows_rulefires correctly)defineon a temporally-prefixed line still registers inpredicate_namesfor later linescheckedcount unchanged for a source with no prefixes — no double-counting, no newly-swept-in non-deontic linescombined checked=21(was 17) with the fix appliedpytest tests/ -q→ 1837 passed (1787 prior + 50 new)🤖 Generated with Claude Code