bug(#4533): remove $ from syntax#4604
Conversation
…ject references and add typo tests
WalkthroughRemoves XI ("$") token support from the parser grammar and listener, updates CompactArrayFqn to stop prefixing ξ, adjusts many parser test resources and tests, disables several integration tests, tweaks several eo-runtime test export signatures, and updates maven-plugin test resource mappings and transpile expectations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Source (EO)
participant Lxr as Lexer
participant Psr as Parser (Eo.g4)
participant Lsn as XeEoListener
participant CFA as CompactArrayFqn
Dev->>Lxr: EO input (no XI/$ cases)
Lxr->>Psr: Tokens (STAR, ROOT, HOME, NAME, ...)
Psr->>Lsn: parse events (beginner, compactArray)
Note over Psr,Lsn: XI alternative removed from rules
Lsn->>Lsn: Resolve base (STAR/ROOT/HOME/"" — no ξ)
Lsn->>CFA: Request FQN string
CFA-->>Lsn: fqn (no ξ prefix)
Lsn-->>Dev: AST / parse tree without XI semantics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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. Comment |
🚀 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
✅ Performance gain: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
eo-integration-tests/src/test/java/integration/JarIT.java(1 hunks)eo-integration-tests/src/test/java/integration/ReadmeSnippetsIT.java(2 hunks)eo-integration-tests/src/test/java/integration/SnippetIT.java(2 hunks)eo-maven-plugin/src/test/resources/org/eolang/maven/probe-packs/add-probes.yaml(1 hunks)eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/embedded-class.yaml(1 hunks)eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/fetching.yaml(2 hunks)eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/locators/add-locators.yaml(1 hunks)eo-parser/src/main/antlr4/org/eolang/parser/Eo.g4(1 hunks)eo-parser/src/main/java/org/eolang/parser/CompactArrayFqn.java(1 hunks)eo-parser/src/main/java/org/eolang/parser/XeEoListener.java(1 hunks)eo-parser/src/test/java/org/eolang/parser/CompactArrayFqnTest.java(1 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-packs/parse/add-default-package.yaml(1 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-packs/parse/expand-aliases.yaml(1 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-packs/parse/resolve-aliases.yaml(1 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/bool-tests-straight.yaml(3 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/full-syntax.yaml(2 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-this-ref-in-name.yaml(0 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-typos/xi-attr.yaml(1 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-typos/xi.yaml(1 hunks)eo-runtime/src/main/eo/org/eolang/fs/path.eo(1 hunks)eo-runtime/src/main/eo/org/eolang/txt/regex.eo(4 hunks)eo-runtime/src/main/eo/org/eolang/txt/text.eo(4 hunks)
💤 Files with no reviewable changes (1)
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-this-ref-in-name.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-10T14:52:11.813Z
Learnt from: h1alexbel
PR: objectionary/eo#4431
File: eo-parser/src/test/java/org/eolang/parser/CompactArrayFqnTest.java:21-27
Timestamp: 2025-08-10T14:52:11.813Z
Learning: In the EO language grammar (Eo.g4), the HOME token is defined as the literal string 'QQ', and the ROOT token is defined as 'Q'. These are input tokens that the parser expects, not the internal representation symbols Φ and Φ̇ that are used in the output/transformation.
Applied to files:
eo-parser/src/main/antlr4/org/eolang/parser/Eo.g4
🧬 Code graph analysis (3)
eo-integration-tests/src/test/java/integration/SnippetIT.java (1)
eo-maven-plugin/src/test/java/org/eolang/maven/MjLintIT.java (1)
Disabled(37-101)
eo-integration-tests/src/test/java/integration/ReadmeSnippetsIT.java (1)
eo-maven-plugin/src/test/java/org/eolang/maven/MjLintIT.java (1)
Disabled(37-101)
eo-integration-tests/src/test/java/integration/JarIT.java (3)
eo-maven-plugin/src/test/java/org/eolang/maven/MjLintIT.java (1)
Disabled(37-101)eo-integration-tests/src/test/java/integration/ReadmeSnippetsIT.java (1)
SuppressWarnings(34-98)eo-integration-tests/src/test/java/integration/SnippetIT.java (1)
SuppressWarnings(33-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: trufflehog
- GitHub Check: ort
- GitHub Check: benchmark
- GitHub Check: snippets
- GitHub Check: qulice
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: integration
🔇 Additional comments (29)
eo-parser/src/test/resources/org/eolang/parser/eo-packs/parse/add-default-package.yaml (1)
39-39: LGTM! Test data updated to reflect XI removal.The replacement of "$" with "and.@" correctly updates the test input to align with the removal of the XI symbol from the language syntax.
eo-parser/src/test/resources/org/eolang/parser/eo-packs/parse/expand-aliases.yaml (1)
26-26: LGTM! Test input correctly updated.The substitution of "$" with "x.@" appropriately reflects the syntax change while maintaining the test's semantic intent.
eo-parser/src/test/java/org/eolang/parser/CompactArrayFqnTest.java (1)
20-25: LGTM! Test case appropriately reduced.The removal of the XI-specific test case ("$.seq *1,ξ.seq") correctly aligns with the elimination of XI from the syntax. The remaining test cases provide adequate coverage for simple and qualified name scenarios.
eo-parser/src/main/java/org/eolang/parser/CompactArrayFqn.java (1)
43-55: LGTM! Method correctly simplified.The removal of XI-related conditional logic streamlines the method while maintaining correct behavior for HOME and NAME token handling. The change aligns perfectly with the test case updates in CompactArrayFqnTest.java.
eo-parser/src/test/resources/org/eolang/parser/eo-packs/parse/resolve-aliases.yaml (1)
42-42: LGTM! Test resource correctly updated.The replacement of "$" with "and.@" is consistent with the XI removal and maintains proper test coverage for the resolve-aliases functionality.
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java (1)
859-877: LGTM! Clean token handling after XI removal.The
enterBeginnermethod now correctly handles STAR, ROOT, and HOME tokens with appropriate fallback logic for empty base cases. The structure is clear and the removal of XI token handling is properly reflected.eo-parser/src/main/antlr4/org/eolang/parser/Eo.g4 (2)
246-248: LGTM! Correct removal of XI from compactArray rule.The
compactArrayrule now correctly accepts(HOME | NAME)as the starting alternatives, consistent with the XI token removal. The structure aligns with the broader parser changes.
407-412: LGTM! Clean beginner rule after XI removal.The
beginnerrule correctly lists STAR, ROOT, HOME, and data as alternatives. The absence of XI token is consistent with the grammar-wide removal of the$symbol.eo-parser/src/test/resources/org/eolang/parser/eo-syntax/bool-tests-straight.yaml (1)
110-177: LGTM! Test resource correctly updated for XI removal.The path references have been properly updated to use direct attribute names (
x.write,x.as-int.lt) and parent references (^.x.write,^.x.as-int.plus) instead of XI-prefixed paths. This maintains the test semantics while aligning with the grammar changes.eo-parser/src/test/resources/org/eolang/parser/eo-syntax/full-syntax.yaml (1)
45-91: LGTM! Test paths correctly simplified after XI removal.The path references have been properly updated:
- Line 51:
plus.@ 5 > i(removed leading$)- Line 53:
x.@(explicit attribute reference instead of$)- Line 88:
b(simplified from XI-based reference)These changes maintain test semantics while aligning with the grammar changes that removed the XI token.
eo-parser/src/test/resources/org/eolang/parser/eo-typos/xi.yaml (1)
5-14: Expectation update matches$token removal.Thanks for pinning the lexer and follow-up parser diagnostics for the stray
$; this keeps the regression test aligned with the new grammar.eo-parser/src/test/resources/org/eolang/parser/eo-typos/xi-attr.yaml (1)
5-14: Attribute typo case covered.Good to see the attribute-form fixture capturing the same
$rejection; it keeps coverage consistent with the syntax change.eo-runtime/src/main/eo/org/eolang/fs/path.eo (1)
603-606: Test visibility reduced to disable export.This test is one of seven identified in the TODO comment at
eo-runtime/src/main/eo/org/eolang/txt/regex.eo(lines 88-96) that depend on regex functionality requiring the XI attribute. The mechanical change from+>to>is consistent with the PR objective to remove$(XI) from the syntax.eo-runtime/src/main/eo/org/eolang/txt/regex.eo (5)
63-63: Simplified internal matcher initialization.The
.matchedwrapper has been removed, streamlining the initialization of thenextblock. This is consistent with removing XI-related constructs from the codebase.
88-96: Clear explanation of disabled regex tests.The TODO comment provides a comprehensive explanation: seven tests are disabled because the idempotency attribute
xi🌵is not available in runtime after removing the$(XI) symbol. This matches the PR objective and explains the test visibility changes across multiple files.
147-157: Test visibility reduced per TODO.The
tests-regex-returns-valid-second-matched-blocktest is one of the seven listed in the TODO comment above. Changing from+>to>disables its export, consistent with the XI removal.
256-269: Test visibility reduced per TODO.The
tests-regex-contains-valid-groups-on-each-matched-blocktest is one of the seven listed in the TODO comment. The visibility change aligns with the PR objective.
100-105: Ensure Intentional Prime Suffix onnext'
Only this prime-suffixed identifier appears in the codebase; confirm that renamingnexttonext'is intentional, aligns with EO conventions for variant naming or collision avoidance, and is documented—or consider reverting to a conventional name if not.eo-runtime/src/main/eo/org/eolang/txt/text.eo (1)
1015-1023: Four regex-dependent tests disabled per XI removal.All four test visibility changes (
tests-replaces-digits-with-string,tests-replaces-slashes-to-windows-separator,tests-replaces-windows-path-with-slash,tests-sanitizes-windows-path-with-regex) are listed in the TODO comment ateo-runtime/src/main/eo/org/eolang/txt/regex.eolines 88-96. These tests depend on the.replacedmethod with regex patterns, which requires the XI attribute being removed in this PR. The mechanical changes from+>to>are consistent with the PR objective.Also applies to: 1026-1034, 1037-1045, 1048-1056
eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/locators/add-locators.yaml (1)
43-43: Locator update aligns with$removal.Switching to the caret chain keeps the locator tied to the same ancestor path after dropping
$. Looks consistent with the new grammar.eo-maven-plugin/src/test/resources/org/eolang/maven/probe-packs/add-probes.yaml (1)
52-52: Probe path mirrors the new syntax rule.Removing the
$prefix cleanly rebindsotherwithin the current scope, matching the updated language expectations.eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/embedded-class.yaml (1)
28-28: Nested attribute access still resolves correctly.Directly targeting
xhere matches the revised scoping rules post-$removal and keeps the Java expectations intact.eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/fetching.yaml (1)
15-15: Expected Java and EO inputs stay in lockstep.The chained
Phi.Φ.take(...)assertion and the simplifiedxreference match the transpiler output after$was dropped, so the resource stays authoritative.Also applies to: 27-27
eo-integration-tests/src/test/java/integration/SnippetIT.java (2)
25-25: LGTM!The import is correctly added to support the
@Disabledannotation.
36-36: Integration test correctly disabled pending EO version release.The
@Disabledannotation properly disables this test class until a new EO version without$syntax is released, as documented in the TODO comment inJarIT.java(lines 29-33).eo-integration-tests/src/test/java/integration/ReadmeSnippetsIT.java (2)
23-23: LGTM!The import is correctly added to support the
@Disabledannotation.
37-37: Integration test correctly disabled pending EO version release.The
@Disabledannotation properly disables this test until a new EO version without$syntax is released, as documented in the TODO comment inJarIT.java(lines 29-33).eo-integration-tests/src/test/java/integration/JarIT.java (2)
22-22: LGTM!The import is correctly added to support the
@Disabledannotation.
29-33: TODO comment clearly documents the reason for disabling tests.The comment explains that tests are disabled because they depend on released EO objects with the old
$syntax, and identifies the affected tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
eo-runtime/src/main/eo/org/eolang/txt/regex.eo (1)
88-96: Fix disabled tests count in TODO: The TODO says “7 tests” are disabled but actually lists 8—update the number to match.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eo-runtime/src/main/eo/org/eolang/fs/dir.eo(1 hunks)eo-runtime/src/main/eo/org/eolang/txt/regex.eo(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ort
- GitHub Check: trufflehog
- GitHub Check: integration
- GitHub Check: snippets
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: benchmark
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: qulice
- GitHub Check: mvn (ubuntu-24.04, 17)
🔇 Additional comments (3)
eo-runtime/src/main/eo/org/eolang/txt/regex.eo (3)
147-147: Test declaration change: Confirm the test runner still recognizestests-regex-returns-valid-second-matched-blocknow declared with>(eo-runtime/src/main/eo/org/eolang/txt/regex.eo:147).
63-63: Approve removal of.matched
No remaining call sites reference the old tuple-like API; removal of.matchedis correct.
256-256: Test harness discovery confirmed: the[] > tests-regex-contains-valid-groups-on-each-matched-blockentry matches the harness pattern and will be executed.
|
@maxonfjvipon please check |
| # Creates a `matcher` that will match the given input against the pattern. | ||
| [txt] > match | ||
| (matched-from-index 1 0).matched > next | ||
| matched-from-index 1 0 > next |
There was a problem hiding this comment.
@maxonfjvipon it does not work here:
Ex org.eolang.Data$ToPhi@10d23e54(Δ = [0x454F6F72-672E454F-656F6C61-6E672E45-4F747874-2E454F72-65676578-24454F70-61747465-726E2445-4F6D6174-63682445-4F6D6174-63686564-5F66726F-6D5F696E-64657840-37363031-30383063-20686173-206A7573-74203220-61747472-69627574-65287329-2C206361-6E277420-72656164-20746865-20322D74-68206F6E-65] = "EOorg.EOeolang.EOtxt.EOregex$EOpattern$EOmatch$EOmatched_from_index@7601080c has just 2 attribute(s), can't read the 2-th one")
Seems that it happens due to idempotency attribute xi🌵 is not available in runtime for Phi.take("xi🌵")
There was a problem hiding this comment.
@h1alexbel it must be available because this attribute is added before converting to Java
There was a problem hiding this comment.
@maxonfjvipon the attribute xi🌵 available in XMIR, but in the translation phase, we are doing this:
<xsl:if test="not(bound/o[eo:idempotent(.)])">
<xsl:variable name="name" select="eo:attr-name(@name, false())"/>
<xsl:if test="not(@name)">
<xsl:message terminate="yes">
<xsl:text>Unnamed attribute found in </xsl:text>
<xsl:value-of select="parent::*/@loc"/>
</xsl:message>
</xsl:if>
<xsl:if test="not(contains($name, '+'))">
<xsl:value-of select="eo:eol($indent)"/>
<xsl:if test="$context!='this'">
<xsl:text>((PhDefault) </xsl:text>
</xsl:if>
<xsl:value-of select="$context"/>
<xsl:if test="$context!='this'">
<xsl:text>)</xsl:text>
</xsl:if>
<xsl:text>.add("</xsl:text>
<xsl:value-of select="$name"/>
<xsl:text>", </xsl:text>
<xsl:apply-templates select="void|bound|atom|abstract">
<xsl:with-param name="indent" select="$indent"/>
<xsl:with-param name="name" select="$name"/>
<xsl:with-param name="parent" select="$parent"/>
<xsl:with-param name="context" select="$context"/>
</xsl:apply-templates>
<xsl:text>);</xsl:text>
</xsl:if>
</xsl:if>If we remove this if check, then in runtime, we will get troubles with put() attributes into xi🌵. Is it the right direction?
There was a problem hiding this comment.
@h1alexbel why do we get troubles? We put only void attributes which must be upper than any other attributes. This xi is already bound so I don't see any troubles
|
|
||
| # Tests that Windows path normalization replaces forward slashes with backslashes. | ||
| [] +> tests-normalizes-win32-path-with-replacing-slashes | ||
| [] > tests-normalizes-win32-path-with-replacing-slashes |
There was a problem hiding this comment.
@h1alexbel I think it's not a good way to disable test.
There was a problem hiding this comment.
@maxonfjvipon I've created todo that mentions all disabled tests.
There was a problem hiding this comment.
@h1alexbel I saw, but disabling tests by replacing +> with > is not a good idea in general
There was a problem hiding this comment.
@maxonfjvipon should I comment them then?
P.S. #4228 to be implemented.
| base = String.format("ξ.%s", fqn); | ||
| } | ||
| return base; | ||
| return fqn; |
There was a problem hiding this comment.
@h1alexbel It's better to leave xi in XMIR since it's IR for PHI expression also so it must be as full as possible
There was a problem hiding this comment.
@maxonfjvipon to me, it does not look correct to append \xi to base in CompactArrayFqn. This snippet:
[] > foo
bar *1 > x
y
z
Will be parsed as such:
[] > foo
ξ.bar *1 > x
y
z
However, bar object also can be global one, so we can't say it should be either Q.org..bar or ξ.bar. If we keep just bar, later, special XSL transformation will resolve that. WDYT?
2 participants
In this PR I've removed
$(\xi) from the syntax.see #4533
Summary by CodeRabbit
Refactor
Runtime
Tests