Fix text() XPath NodeType failing to lex - #38
Open
steiler wants to merge 3 commits into
Open
Conversation
text() is a standard XPath 1.0 NodeType test (§2.3) that is fully implemented by ProgBuilder.Text() in the "expr" grammar via the path/context stack, but the lexer special-cased it to require a function-table lookup instead of going through the generic nameIsNodeType() path used by comment()/processing-instruction()/node(). Since "text" was never added to xpathFunctionTable, that lookup always failed, causing any expression using text() (e.g. YANG must-statements like sonic-port.yang's "count(adv_speeds[text()='all']) = 0 or count(adv_speeds) = 1") to fail with "Unknown function or node type: 'text'". Also fix NewCtxFromMach/newCtx to initialize actualPathStack and predicatePathElemStack like NewCtxFromCurrent does, avoiding nil pointer panics for must/when statements using absolute or parent-relative paths.
count(adv_speeds[text()='all']) crashed with "Fn 'count' takes NODESET, not BOOL as arg 0" because Eq()/Ne()/relational operators collapsed a leaf-list's DatumSlice operand straight to a Bool before count() ever saw it. Leaf-lists have no per-item node identity in the Entry-based adapter used by data-server, so they can't be modeled as a real Nodeset. Generalize DatumSlice comparisons to behave like Nodeset ones: filter the DatumSlice down to matching items and push the result as a new DatumSliceDatum, composable with count() and existentially truthy in boolean contexts, instead of collapsing to Bool. This applies uniformly across Eq, Ne, and the relational operators via a shared compareDatumSlicesAndPush() helper. Adds an Entry-based test double (entryfake) that mirrors data-server's single-DatumSliceDatum-per-leaf-list representation, since the existing xpathtest fixtures model leaf-list items as per-item nodes and can't reproduce this bug. Records the decision in an ADR.
count(leaflist[text()='x']) = 0 or count(leaflist) = 1 -- the exact shape of SONiC's PORT_LIST must-statements -- still crashed with "Fn 'count' takes NODESET, not BOOL as arg 0" even after the previous DatumSlice fix, but only when the leaf-list predicate and a second, unrelated bare reference to the same leaf-list appeared in the same expression. EvalLocPath's fast path for "the predicate already produced this path's value, don't re-resolve it" (guarded by previousPredicateRequiresELP) only reset that flag on the *next* predicate start, and never popped the actualPathStack frame it was abandoning. That frame (e.g. a lone "flag" element) was left sitting on the stack, so the next unrelated bare path reference to the same leaf-list appended onto it instead of starting fresh, producing a bogus multi-element path that failed to navigate -- count() then silently fell back to whatever Bool was left over from the prior comparison. Reset both previousPredicateRequiresELP and the actualPathStack frame at the point the fast path is consumed, mirroring what EvalLocPathInternal itself does when it actually resolves a path. Also fixes entryfake: NewLeafList's variadic values collapse to a nil slice for a zero-value call, which was indistinguishable from "not a leaf-list at all" and made it impossible to reproduce this bug's unset-leaf-list case in tests.
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
1 participant
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
text()is a standard XPath 1.0 NodeType test (§2.3), fully implemented byProgBuilder.Text()in theexprgrammar via the path/context stack. The lexer special-cased"text"to require a function-table lookup instead of using the genericnameIsNodeType()path used bycomment()/processing-instruction()/node(). Since"text"was never inxpathFunctionTable, that lookup always failed withUnknown function or node type: 'text', breaking any expression usingtext()— including real-world YANG must-statements such assonic-port.yang'scount(adv_speeds[text()='all']) = 0 or count(adv_speeds) = 1.NewCtxFromMach/newCtxnever initializedactualPathStack/predicatePathElemStack(unlikeNewCtxFromCurrent), causing nil pointer panics for must/when statements using absolute (/) or parent-relative (..) paths in the code paths that use those constructors (schema/validate.go,compile/compile.go).Test plan
go test ./xpath/grammars/expr/... -run 'TestLexNodeType|TestLexTextFunc' -vpasses.(count(adv_speeds[text()='all']) = 0) or (count(adv_speeds) = 1)now compiles successfully (previously failed withUnknown function or node type: 'text').go test ./schema/...still passes fully after thecontext.gochange.