Reject self-referential predicate definitions#61
Merged
Conversation
`define p: is p` on a first-ever definition of `p` silently parses to string-equality (BareWord), not a PredicateApplicationNode, so PR #60's cycle check never sees it and the line succeeds as "equals the text 'p'" instead of erroring — even though it reads as self-referential. Adds _check_self_referential_define, run unconditionally in _check_define before the existing checks, catching a bare-word/name value identical to the name being defined in field, value, or value2 position, including inside compound and/or branches.
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
Closes the one case PR #60 (
fix/cyclic-predicate-rejection, mergeddd0d711) doesn't reach:define p: is pon a first-ever definition ofpcurrently returnsSUCCESS(canonicaldefine p: each is p), silently meaning "equals the textp" instead of erroring — even though the line reads as self-referential to a human.Mechanism: when
pis not yet a known predicate at parse time,is pparses to aConditionNode(field=EachPronoun(), op='is', value=BareWord(word='p'))— string equality, not aPredicateApplicationNode. PR #60's_find_predicate_cyclewalks predicate references only, so it never sees this case; there's no predicate reference in the AST to walk.Fix:
_check_self_referential_define(name, cond)inanalyzer.py, called from_check_definebefore the existing forward-declaration/cycle checks, unconditionally (regardless of whethernameis already in the symbol table). WalksCompoundConditionNodeon both branches; for aConditionNode, checksfield,value, andvalue2for aBareWord.wordorNameRef.nameequal to the name being defined.Message wording: chose
"Definition '{name}' can't refer to itself."— distinct from the existing"can't depend on itself"(one-hop redefinition) and"refers back to itself through..."(multi-hop chain), so all three stay distinguishable in tests and terminal output.Analyzer-only, no parser or interpreter change:
execute()gates onanalyze()before running, so the analyzer rejects before any interpreter code touches a self-referential define.Acceptance cases (12 total, verified against this branch)
define p: is p(no prior def)ERROR_SEMANTIC—Definition 'p' can't refer to itself.define p: is not pERROR_PARSE(see note below — pre-existing, unrelated)define p: p is above 1ERROR_SEMANTIC— same new messagedefine p: is above 1 and is pERROR_SEMANTIC— same new messagedefine p: is p or is above 1ERROR_SEMANTIC— same new messageERROR_SEMANTIC—Definition 'p' can't depend on itself.— byte-identical to mainERROR_SEMANTIC—...through 'q'. A definition can't depend on itself.— byte-identical to mainERROR_SEMANTIC—...through 'r', 'q'. A definition can't depend on itself.— byte-identical to mainSUCCESSSUCCESSSUCCESSSUCCESSCase 2 note:
define p: is not pnever reaches the analyzer — it'sERROR_PARSE("After 'not' I expected 'above', 'below', or 'equal to'"), for reasons unrelated to self-reference. Confirmed pre-existing and general:define p: is not grownupanddefine zzz: is not qfail identically on unmodified main. The grammar only acceptsis not <word>when<word>is already a known predicate name — an unknown bareword afteris notis invalid syntax regardless of what it says. Fixing that is a parser change, explicitly out of scope for this fix. Documented with a test (test_is_not_self_reference_is_a_pre_existing_parse_error_not_semantic) rather than silently left unhandled.PR #60's messages (cases 6–8) are unchanged, confirmed byte-identical above.
Test plan
pytest tests/test_define.py -q— 47 passed (40 existing + 7 new)pytest tests/ -q— 1668 passed (1661 baseline + 7 new), zero regressions, zero skippedlen(ALL_RESERVED) - len(TOMBSTONES) == 61— vocabulary unchanged (pre-existing assertion attests/test_define.py:342)🤖 Generated with Claude Code