feat(#13): missing-rho-reference lint
#450
Conversation
|
@maxonfjvipon the changes are very experimental, I'm not sure about the way of navigating over parent objects from |
|
@maxonfjvipon WDYT? |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
|
|
@maxonfjvipon WDYT? |
|
@h1alexbel what's up with this one? |
|
@yegor256 plan to finish it soon |
WalkthroughA new XSLT stylesheet for detecting missing rho ( Changes
Sequence Diagram(s)sequenceDiagram
participant EO_XML as EO XML Input
participant XSLT as missing-rho-reference.xsl
participant Output as Defects XML
EO_XML->>XSLT: Provide EO source as XML
XSLT->>XSLT: For each object with @base starting with ^.
XSLT->>XSLT: Resolve owner and attribute
XSLT->>XSLT: Traverse ancestors to find attribute
alt Attribute missing
XSLT->>Output: Emit <defect> for missing rho reference
else Attribute found
XSLT->>Output: No defect emitted
end
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/main/resources/org/eolang/lints/errors/missing-rho-reference.xsl (1)
15-18: Address TODO comments before merging.There are two significant TODO comments indicating:
- The
report-missingtemplate needs restructuring (lines 15-18)- Auto-named objects are not fully supported (lines 37-40)
These should be addressed as they represent core functionality gaps.
Would you like me to help implement these TODO items or create issues to track them?
Also applies to: 37-40
src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/allows-rho-ref-to-present-object-from-deeply-nested.yaml (1)
1-21: Good coverage for deeply nested references.This test appropriately covers the
^.^.messagepattern.Consider adding more edge cases like:
- Triple nesting (
^.^.^.attr)- References to non-existent deeply nested attributes
- Mix of existing and non-existing references in the same object
src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/catches-missing-rho-ref.yaml (2)
4-8: Add message assertion & align style with sibling testYou only assert the count and the line number. Including a
normalize-space()assertion for the exact message – as you already do in the “deeply-nested” sibling – would make the test stricter and prevent silent regressions in the wording.- /defects[count(defect[@severity='error'])=1] - /defects/defect[@line='4'] + - /defects/defect[1][normalize-space()= + 'The rho reference "hello" is missing in "foo" object']
1-8: Keep yamllint happy – disable long-line ruleThe XPath in the previous suggestion (and even the current second assertion) breaches the default 80-char limit enforced by
yamllint.
Add the same directive you already placed in the second test file to avoid CI noise.---- +# yamllint disable rule:line-length +---src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/catch-missing-rho-ref-from-deeply-nested.yaml (2)
10-10: Quote the whole XPath to avoid YAML edge-casesThe scalar starts with
/but contains an unbalanced single quote right afternormalize-space()=.
While most YAML parsers swallow it, it is technically a plain-scalar gotcha and trips up stricter linters.
Wrap the whole XPath in single quotes (and escape the internal ones) or keep the current disable-rule comment but be explicit.- - /defects/defect[1][normalize-space()='The rho reference "messageeee" is missing in "foo" object'] + - '/defects/defect[1][normalize-space()=''The rho reference "messageeee" is missing in "foo" object'']'
1-3: File-name consistencyOther resources follow the
catches-…prefix.
Renaming this file tocatches-missing-rho-ref-from-deeply-nested.yamlkeeps naming uniform and eases grepping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/resources/org/eolang/lints/errors/missing-rho-reference.xsl(1 hunks)src/main/resources/org/eolang/motives/errors/missing-rho-reference.md(1 hunks)src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/allows-rho-ref-to-present-object-from-deeply-nested.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/allows-rho-ref-to-present-object.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/allows-rho-ref-when-parent-has-multiple-objects.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/catch-missing-rho-ref-from-deeply-nested.yaml(1 hunks)src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/catches-missing-rho-ref.yaml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
src/main/resources/org/eolang/motives/errors/missing-rho-reference.md
[grammar] ~3-~3: The plural noun “objects” cannot be used with the article “an”. Did you mean “an existing object” or “existing objects”?
Context: ...] reference (^.) should point only to an existing objects. Incorrect: ```eo # Foo. [] > foo [...
(A_NNS)
🔇 Additional comments (2)
src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/allows-rho-ref-to-present-object.yaml (1)
1-14: Well-structured test case.This test correctly verifies that valid rho references to sibling objects don't trigger false positives.
src/test/resources/org/eolang/lints/packs/single/missing-rho-reference/allows-rho-ref-when-parent-has-multiple-objects.yaml (1)
1-15: Good test coverage for multiple siblings.This test appropriately covers the case where the parent has multiple objects.
| For now we ignore them, but we need to check them as well, as they are abstract | ||
| objects. Don't forget to add test packs with auto named objects. | ||
| --> | ||
| <xsl:if test="not($owner/o[@name=$attribute]) and not(contains($owner/@name, 'a🌵'))"> |
There was a problem hiding this comment.
Inconsistent pattern matching for auto-named objects.
Line 41 uses contains() while line 25 uses starts-with() for checking the auto-named pattern. This inconsistency could lead to different behavior.
Apply consistent pattern matching:
-<xsl:if test="not($owner/o[@name=$attribute]) and not(contains($owner/@name, 'a🌵'))">
+<xsl:if test="not($owner/o[@name=$attribute]) and not(starts-with($owner/@name, 'a🌵'))">🤖 Prompt for AI Agents
In src/main/resources/org/eolang/lints/errors/missing-rho-reference.xsl at line
41, replace the contains() function with starts-with() to match the pattern used
at line 25 for checking auto-named objects. This ensures consistent pattern
matching by using starts-with($owner/@name, 'a🌵') instead of contains(),
aligning the logic across the file.
| <xsl:param name="attribute"/> | ||
| <xsl:param name="owner"/> | ||
| <xsl:choose> | ||
| <xsl:when test="$owner[not(eo:abstract(.))] or not(empty($owner/o[starts-with(@name, 'a🌵')]))"> |
There was a problem hiding this comment.
Logic appears inverted in the recursion condition.
The condition on line 25 seems to continue recursion when the owner is NOT abstract or HAS auto-named children. Based on the comment at line 13-14, shouldn't this skip auto-named objects instead? This logic appears inverted.
Consider reversing the logic:
-<xsl:when test="$owner[not(eo:abstract(.))] or not(empty($owner/o[starts-with(@name, 'a🌵')]))">
+<xsl:when test="$owner[eo:abstract(.)] and empty($owner/o[starts-with(@name, 'a🌵')])">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <xsl:when test="$owner[not(eo:abstract(.))] or not(empty($owner/o[starts-with(@name, 'a🌵')]))"> | |
| <xsl:when test="$owner[eo:abstract(.)] and empty($owner/o[starts-with(@name, 'a🌵')])"> |
🤖 Prompt for AI Agents
In src/main/resources/org/eolang/lints/errors/missing-rho-reference.xsl at line
25, the recursion condition incorrectly continues when the owner is not abstract
or has auto-named children, which is inverted. Modify the condition to skip
recursion for owners that are abstract or have auto-named children by reversing
the logic in the test expression accordingly.
| <xsl:variable name="position" select="(count(tokenize(@base, '\^')) - 1) * 2"/> | ||
| <xsl:call-template name="report-missing"> | ||
| <xsl:with-param name="position" select="$position"/> | ||
| <xsl:with-param name="attribute" select="translate(translate(@base, '.^', ''), '$', '')"/> | ||
| <xsl:with-param name="owner" select="ancestor::node()[$position]"/> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unclear position calculation logic.
The position calculation (count(tokenize(@base, '\^')) - 1) * 2 lacks documentation. Why multiply by 2? This makes the code hard to understand and maintain.
Add documentation or verify the calculation:
🏁 Script executed:
#!/bin/bash
# Search for examples of rho references to understand the pattern
rg -A 5 -B 5 '\$\.\^' --type yamlLength of output: 35
Here’s a deeper inspection across all XSL error templates to see if this “* 2” pattern recurs and how other templates document it:
🏁 Script executed:
#!/bin/bash
echo "1) Listing all missing-*-reference.xsl files:"
ls src/main/resources/org/eolang/lints/errors/missing-*-reference.xsl
echo
echo "2) Looking for all count(tokenize occurrences in these XSLs:"
rg -n "count(tokenize" src/main/resources/org/eolang/lints/errors/*.xsl
echo
echo "3) Checking how ancestor::node() is used elsewhere:"
rg -n "ancestor::node" src/main/resources/org/eolang/lints/errors/*.xsl
echo
echo "4) Finding the report-missing template definition for context:"
rg -n "<xsl:template name=\"report-missing\"" src/main/resources/org/eolang/lints/errors/*.xslLength of output: 2011
Document the position‐calculation logic
The (count(tokenize(@base,'\^')) - 1) * 2 expression is only used in missing-rho-reference.xsl, and nothing else in the repo uses that same pattern—so it really needs an explanatory comment. Without it, future maintainers will struggle to understand why each caret maps to “two levels” in the XSLT tree.
Please update src/main/resources/org/eolang/lints/errors/missing-rho-reference.xsl around line 68 to include a clear XSL comment, for example:
<!--
Each '^' in @base represents a missing ρ-reference.
We step up two ancestor nodes per caret, so:
(count of '^') - 1 → number of jumps
× 2 → levels per jump
-->
<xsl:variable name="position"
select="(count(tokenize(@base, '\^')) - 1) * 2"/>- File: src/main/resources/org/eolang/lints/errors/missing-rho-reference.xsl
- Lines: ~68
🤖 Prompt for AI Agents
In src/main/resources/org/eolang/lints/errors/missing-rho-reference.xsl around
line 68, add an explanatory XSL comment above the position variable declaration
to clarify the logic behind the expression (count(tokenize(@base, '\^')) - 1) *
2. The comment should explain that each '^' in @base represents a missing
ρ-reference and that the calculation steps up two ancestor nodes per caret,
detailing the meaning of each part of the expression for future maintainers.
| <xsl:variable name="up" select="ancestor::node()[$position + 1]"/> | ||
| <xsl:if test="name($up) = 'o'"> | ||
| <xsl:call-template name="report-missing"> | ||
| <xsl:with-param name="position" select="$position + 1"/> | ||
| <xsl:with-param name="attribute" select="$attribute"/> | ||
| <xsl:with-param name="owner" select="$up"/> | ||
| </xsl:call-template> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fragile ancestor navigation logic.
Using ancestor::node()[$position + 1] is fragile as it depends on the exact node structure. Consider using a more robust approach like ancestor::o to specifically target object elements.
Verify that the position calculation correctly navigates to parent objects in all cases by examining the XML structure:
🏁 Script executed:
#!/bin/bash
# Search for test cases that might reveal the expected XML structure
fd -e yaml -e xml | xargs rg -l "missing-rho-reference|\\$\\.\\^" | head -10 | xargs catLength of output: 2048
Use ancestor::o for reliable object lookup
The current lookup
<xsl:variable name="up" select="ancestor::node()[$position + 1]"/>depends on every ancestor (including text or comment nodes) and is brittle. Instead, pick the Nth object ancestor directly:
Replace
- <xsl:variable name="up" select="ancestor::node()[$position + 1]"/>
+ <xsl:variable name="up" select="(ancestor::o)[$position + 1]"/>This way you only count <o> elements (object scopes), so the $position+1 lookup always lands on the correct object.
🤖 Prompt for AI Agents
In src/main/resources/org/eolang/lints/errors/missing-rho-reference.xsl around
lines 26 to 32, the variable 'up' is assigned using ancestor::node() which
includes all ancestor nodes and can lead to incorrect selection. Change the
XPath to use ancestor::o to select only object elements, ensuring the
$position+1 index correctly targets the intended object ancestor. Replace
ancestor::node()[$position + 1] with ancestor::o[$position + 1] to fix this.
| Each [rho][EO-specials] reference (`^.`) should point only to an existing | ||
| objects. |
There was a problem hiding this comment.
Fix grammar error in documentation.
-Each [rho][EO-specials] reference (`^.`) should point only to an existing
-objects.
+Each [rho][EO-specials] reference (`^.`) should point only to existing
+objects.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Each [rho][EO-specials] reference (`^.`) should point only to an existing | |
| objects. | |
| Each [rho][EO-specials] reference (`^.`) should point only to existing | |
| objects. |
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: The plural noun “objects” cannot be used with the article “an”. Did you mean “an existing object” or “existing objects”?
Context: ...] reference (^.) should point only to an existing objects. Incorrect: ```eo # Foo. [] > foo [...
(A_NNS)
🤖 Prompt for AI Agents
In src/main/resources/org/eolang/motives/errors/missing-rho-reference.md around
lines 3 to 4, fix the grammar error by changing "objects" to "object" so the
sentence reads "Each [rho][EO-specials] reference (`^.`) should point only to an
existing object."
@maxonfjvipon why we should not complain in this case? Seems that |
3 participants
In this PR I've introduced new lint:
missing-rho-reference, that issues defect with error severity, in case of\rhoreference (.^) is missing inside abstract object, reference refers to. I also left several puzzles to be implemented later.closes #13
History:
^references #13): missing-caret-references^references #13): allows present^references #13): check deeply nested^references #13): correct calculation^references #13): if auto move up^references #13): rho^references #13): allows to present from deeply nested^references #13): recursion^references #13): cleaned up^references #13): skip non-abstracts^references #13): eo:abstract^references #13): clean^references #13): escape^references #13): puzzleSummary by CodeRabbit
New Features
Documentation
Tests