bug(#4529): locals-to-aliases optimization implemented in MjParse
#4588
Conversation
…ts to aliases for better package management
…cals-to-aliases optimization
WalkthroughAdds an XSLT-based optimization step to MjParse that rewrites local EO object references into aliases during parsing. Integrates an Xsline pipeline with reserved-name handling, applies it to parsed XMIR before caching, introduces a new XSL stylesheet, removes two legacy shake XSLs, and adds a test verifying alias conversion. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Maven Goal
participant P as MjParse
participant X as XMLDocument
participant L as Xsline (OPTIMIZATION_LINE)
participant T as XSL: locals-to-aliases
participant C as Cache
U->>P: parse(source)
P->>X: build XMIR
X-->>P: XMIR Document
P->>L: pass XMIR
rect rgba(210,235,255,0.4)
note right of L: Optimization step (new)
L->>T: transform with reserved names
T-->>L: rewritten XMIR (aliases)
end
L-->>P: optimized XMIR
P->>C: store optimized XMIR
C-->>P: ack
P-->>U: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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
|
|
potentially blocked by objectionary/lints#759 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eo-maven-plugin/src/test/java/org/eolang/maven/MjParseTest.java (1)
36-38: Suppress PMD TooManyMethods for this big test class (or split it).Pipeline fails with PMD TooManyMethods. Easiest fix is to extend the existing suppression; better fix is splitting into focused test classes.
Apply this diff to unblock CI:
-@SuppressWarnings("PMD.AvoidDuplicateLiterals") +@SuppressWarnings({"PMD.AvoidDuplicateLiterals", "PMD.TooManyMethods"})
🧹 Nitpick comments (2)
eo-maven-plugin/src/main/resources/org/eolang/maven/parse/locals-to-aliases.xsl (2)
6-9: Remove unused param.
nameparam isn’t used anywhere.Apply this diff:
- <xsl:param name="name"/>
10-12: Edge case: missing package.If package meta is absent, this would produce “.name” bases. Guarded above with string-length($pkg) > 0; consider also short‑circuiting alias creation when pkg is empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eo-maven-plugin/src/main/java/org/eolang/maven/MjParse.java(4 hunks)eo-maven-plugin/src/main/resources/org/eolang/maven/parse/locals-to-aliases.xsl(1 hunks)eo-maven-plugin/src/main/resources/org/eolang/maven/shake/clean-up.xsl(0 hunks)eo-maven-plugin/src/main/resources/org/eolang/maven/shake/remove-noise.xsl(0 hunks)eo-maven-plugin/src/test/java/org/eolang/maven/MjParseTest.java(2 hunks)
💤 Files with no reviewable changes (2)
- eo-maven-plugin/src/main/resources/org/eolang/maven/shake/clean-up.xsl
- eo-maven-plugin/src/main/resources/org/eolang/maven/shake/remove-noise.xsl
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: h1alexbel
PR: objectionary/eo#4431
File: eo-parser/src/main/resources/XMIR.xsd:80-81
Timestamp: 2025-08-12T15:24:05.011Z
Learning: In the EO parser architecture, Φ̇ is an alias for Φ.org.eolang that gets expanded during XMIR decoration by expand-qqs.xsl. The XSD schema for FQN patterns should only validate canonical forms (Φ and ξ as roots) since alias expansion happens before XSD validation.
📚 Learning: 2025-08-12T15:24:05.011Z
Learnt from: h1alexbel
PR: objectionary/eo#4431
File: eo-parser/src/main/resources/XMIR.xsd:80-81
Timestamp: 2025-08-12T15:24:05.011Z
Learning: In the EO parser architecture, Φ̇ is an alias for Φ.org.eolang that gets expanded during XMIR decoration by expand-qqs.xsl. The XSD schema for FQN patterns should only validate canonical forms (Φ and ξ as roots) since alias expansion happens before XSD validation.
Applied to files:
eo-maven-plugin/src/main/resources/org/eolang/maven/parse/locals-to-aliases.xsl
📚 Learning: 2025-08-22T12:19:41.098Z
Learnt from: h1alexbel
PR: objectionary/eo#4468
File: eo-integration-tests/src/test/java/integration/XmirIT.java:42-44
Timestamp: 2025-08-22T12:19:41.098Z
Learning: The jcabi-xml library's XMLDocument class has a constructor that accepts java.nio.file.Path: `public XMLDocument(final Path file) throws FileNotFoundException`. This constructor internally converts the Path to File using path.toFile().
Applied to files:
eo-maven-plugin/src/main/java/org/eolang/maven/MjParse.javaeo-maven-plugin/src/test/java/org/eolang/maven/MjParseTest.java
🪛 GitHub Actions: qulice
eo-maven-plugin/src/main/java/org/eolang/maven/MjParse.java
[error] 78-78: Qulice Checkstyle violation: Variable access definition in wrong order. (DeclarationOrderCheck)
[error] 83-83: Qulice Checkstyle violation: Variable access definition in wrong order. (DeclarationOrderCheck)
[error] 88-88: Qulice Checkstyle violation: Variable access definition in wrong order. (DeclarationOrderCheck)
eo-maven-plugin/src/test/java/org/eolang/maven/MjParseTest.java
[error] 38-330: PMD: This class has too many methods. (TooManyMethods)
[error] 8-8: PMD: Unused import 'com.jcabi.xml.XML' (UnnecessaryImport)
⏰ 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: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: ebnf
- GitHub Check: benchmark
- GitHub Check: trufflehog
- GitHub Check: integration
- GitHub Check: ort
🔇 Additional comments (3)
eo-maven-plugin/src/test/java/org/eolang/maven/MjParseTest.java (1)
287-312: Test intent LGTM, but currently depends on a broken XSL.The XPath assertions are correct for “locals-to-aliases”. However, the stylesheet (locals-to-aliases.xsl) rewrites only Φ.* bases, not local names, so this test will fail until the XSL is fixed (see stylesheet review).
eo-maven-plugin/src/main/java/org/eolang/maven/MjParse.java (1)
135-138: Apply transform is correct; keep logging before/after as is.Wrapping parsed XMIR through Xsline here is the right place (before caching). No concerns once XSL is fixed.
eo-maven-plugin/src/main/resources/org/eolang/maven/parse/locals-to-aliases.xsl (1)
29-34: LGTM on alias meta structure.Head/tail/part layout matches existing XMIR conventions and the test assertion.
| private static final String QQS = String.join( | ||
| ",", | ||
| new ListOf<>( | ||
| "Φ.org.eolang.number", | ||
| "Φ.org.eolang.bytes" | ||
| ) | ||
| ); | ||
|
|
There was a problem hiding this comment.
Fix Checkstyle DeclarationOrder by aligning access modifiers.
The new fields precede package-private constants (ZERO, DIR, CACHE), causing “Variable access definition in wrong order.” Make these new constants package‑private to match, or move them below. Quick unblocking change:
Apply this diff:
- private static final String QQS = String.join(
+ static final String QQS = String.join(
",",
new ListOf<>(
"Φ.org.eolang.number",
"Φ.org.eolang.bytes"
)
);🤖 Prompt for AI Agents
In eo-maven-plugin/src/main/java/org/eolang/maven/MjParse.java around lines 51
to 58, the newly added constants are declared private which violates Checkstyle
DeclarationOrder because they appear before package-private constants (ZERO,
DIR, CACHE); change these new constants to package-private by removing the
'private' modifier (or alternatively move their declarations below the existing
package-private constants) so their access modifiers align with the existing
fields and resolve the "Variable access definition in wrong order" error.
| private static final Xsline OPTIMIZATION_LINE = new Xsline( | ||
| new TrDefault<>( | ||
| new StClasspath( | ||
| "/org/eolang/maven/parse/locals-to-aliases.xsl", | ||
| "name reserved", | ||
| String.format( | ||
| "reserved %s", | ||
| MjParse.QQS | ||
| ) | ||
| ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Same Checkstyle issue for OPTIMIZATION_LINE; also drop unused XSL param “name”.
- Align access to package‑private to satisfy DeclarationOrder.
- The XSL doesn’t use the “name” param; remove it to avoid confusion.
Apply this diff:
- private static final Xsline OPTIMIZATION_LINE = new Xsline(
+ static final Xsline OPTIMIZATION_LINE = new Xsline(
new TrDefault<>(
new StClasspath(
"/org/eolang/maven/parse/locals-to-aliases.xsl",
- "name reserved",
String.format(
"reserved %s",
MjParse.QQS
)
)
)
);📝 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.
| private static final Xsline OPTIMIZATION_LINE = new Xsline( | |
| new TrDefault<>( | |
| new StClasspath( | |
| "/org/eolang/maven/parse/locals-to-aliases.xsl", | |
| "name reserved", | |
| String.format( | |
| "reserved %s", | |
| MjParse.QQS | |
| ) | |
| ) | |
| ) | |
| ); | |
| static final Xsline OPTIMIZATION_LINE = new Xsline( | |
| new TrDefault<>( | |
| new StClasspath( | |
| "/org/eolang/maven/parse/locals-to-aliases.xsl", | |
| String.format( | |
| "reserved %s", | |
| MjParse.QQS | |
| ) | |
| ) | |
| ) | |
| ); |
🤖 Prompt for AI Agents
In eo-maven-plugin/src/main/java/org/eolang/maven/MjParse.java around lines 62
to 73, make OPTIMIZATION_LINE package-private (remove the "private" modifier) to
align with DeclarationOrder and remove the unused XSL parameter "name" (drop the
second argument "name reserved" from the StClasspath constructor call), leaving
only the resource path and the actual "reserved ..." parameter; keep the rest of
the Xsline/TrDefault wrapping unchanged.
| <xsl:template match="@base[starts-with(., 'Φ.') and not(. = $qqs)]"> | ||
| <xsl:variable name="seg" select="tokenize(., '\.')[last()]"/> | ||
| <xsl:attribute name="base" select="concat($pkg, '.', $seg)"/> | ||
| </xsl:template> |
There was a problem hiding this comment.
Critical: Wrong rewrite target (affects Φ. instead of locals).*
You currently rewrite bases starting with Φ., which are standard library objects, not locals. This will incorrectly turn many QQ references into package ones and won’t touch local names like “bar”. Rewrite only simple, local bases (no dot, not Φ/ξ).
Apply this diff:
- <xsl:template match="@base[starts-with(., 'Φ.') and not(. = $qqs)]">
- <xsl:variable name="seg" select="tokenize(., '\.')[last()]"/>
- <xsl:attribute name="base" select="concat($pkg, '.', $seg)"/>
- </xsl:template>
+ <!-- Rewrite only local (simple) bases to fully-qualified ones -->
+ <xsl:template match="@base[not(contains(., '.')) and not(starts-with(., 'Φ')) and not(starts-with(., 'ξ')) and string-length($pkg) > 0]">
+ <xsl:attribute name="base" select="concat($pkg, '.', .)"/>
+ </xsl:template>🤖 Prompt for AI Agents
In
eo-maven-plugin/src/main/resources/org/eolang/maven/parse/locals-to-aliases.xsl
around lines 17-20, the template currently matches bases starting with "Φ."
(affecting standard library names) and tokenizes by dots; instead, change the
match to target only simple local bases (no dot, and not the special names Φ or
ξ) and exclude $qqs. Replace the condition with something like
@base[not(contains(., '.')) and not(. = 'Φ') and not(. = 'ξ') and not(. = $qqs)]
and set the new base to concat($pkg, '.', .) (remove the tokenize/last() logic).
Ensure the attribute creation uses the simple local name rather than extracting
the last dotted segment.
| <xsl:template match="metas"> | ||
| <xsl:copy> | ||
| <xsl:apply-templates select="@*|node()"/> | ||
| <xsl:for-each-group select="//@base[starts-with(., 'Φ.') and not(. = $qqs)]" group-by="tokenize(., '\.')[last()]"> | ||
| <xsl:variable name="seg" select="current-grouping-key()"/> | ||
| <xsl:variable name="new" select="concat($pkg, '.', $seg)"/> | ||
| <xsl:variable name="tail" select="concat($seg, ' ', $new)"/> | ||
| <xsl:if test="not(/object/metas/meta[head='alias' and tail=$new])"> | ||
| <meta> | ||
| <head>alias</head> | ||
| <tail><xsl:value-of select="$tail"/></tail> | ||
| <part><xsl:value-of select="$seg"/></part> | ||
| <part><xsl:value-of select="$new"/></part> | ||
| </meta> | ||
| </xsl:if> | ||
| </xsl:for-each-group> | ||
| </xsl:copy> |
There was a problem hiding this comment.
Critical: Alias generation targets the wrong set and has a bug in existence check.
- It groups Φ.* bases, so it will never create an alias for local “bar”.
- It checks existing aliases by
tail=$newinstead of the full “bar foo.bar”.
Apply this diff:
<xsl:template match="metas">
<xsl:copy>
<xsl:apply-templates select="@*|node()"/>
- <xsl:for-each-group select="//@base[starts-with(., 'Φ.') and not(. = $qqs)]" group-by="tokenize(., '\.')[last()]">
- <xsl:variable name="seg" select="current-grouping-key()"/>
- <xsl:variable name="new" select="concat($pkg, '.', $seg)"/>
- <xsl:variable name="tail" select="concat($seg, ' ', $new)"/>
- <xsl:if test="not(/object/metas/meta[head='alias' and tail=$new])">
+ <!-- Create aliases for local names only -->
+ <xsl:for-each-group
+ select="//@base[not(contains(., '.')) and not(starts-with(., 'Φ')) and not(starts-with(., 'ξ'))]"
+ group-by=".">
+ <xsl:variable name="seg" select="current-grouping-key()"/>
+ <xsl:variable name="new" select="concat($pkg, '.', $seg)"/>
+ <xsl:variable name="tail" select="concat($seg, ' ', $new)"/>
+ <xsl:if test="string-length($pkg) > 0 and not(/object/metas/meta[head='alias' and tail=$tail])">
<meta>
<head>alias</head>
- <tail><xsl:value-of select="$tail"/></tail>
- <part><xsl:value-of select="$seg"/></part>
- <part><xsl:value-of select="$new"/></part>
+ <tail><xsl:value-of select="$tail"/></tail>
+ <part><xsl:value-of select="$seg"/></part>
+ <part><xsl:value-of select="$new"/></part>
</meta>
</xsl:if>
</xsl:for-each-group>
</xsl:copy>
</xsl:template>📝 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:template match="metas"> | |
| <xsl:copy> | |
| <xsl:apply-templates select="@*|node()"/> | |
| <xsl:for-each-group select="//@base[starts-with(., 'Φ.') and not(. = $qqs)]" group-by="tokenize(., '\.')[last()]"> | |
| <xsl:variable name="seg" select="current-grouping-key()"/> | |
| <xsl:variable name="new" select="concat($pkg, '.', $seg)"/> | |
| <xsl:variable name="tail" select="concat($seg, ' ', $new)"/> | |
| <xsl:if test="not(/object/metas/meta[head='alias' and tail=$new])"> | |
| <meta> | |
| <head>alias</head> | |
| <tail><xsl:value-of select="$tail"/></tail> | |
| <part><xsl:value-of select="$seg"/></part> | |
| <part><xsl:value-of select="$new"/></part> | |
| </meta> | |
| </xsl:if> | |
| </xsl:for-each-group> | |
| </xsl:copy> | |
| <xsl:template match="metas"> | |
| <xsl:copy> | |
| <xsl:apply-templates select="@*|node()"/> | |
| <!-- Create aliases for local names only --> | |
| <xsl:for-each-group | |
| select="//@base[not(contains(., '.')) | |
| and not(starts-with(., 'Φ')) | |
| and not(starts-with(., 'ξ'))]" | |
| group-by="."> | |
| <xsl:variable name="seg" select="current-grouping-key()"/> | |
| <xsl:variable name="new" select="concat($pkg, '.', $seg)"/> | |
| <xsl:variable name="tail" select="concat($seg, ' ', $new)"/> | |
| <xsl:if test="string-length($pkg) > 0 | |
| and not(/object/metas/meta[head='alias' and tail=$tail])"> | |
| <meta> | |
| <head>alias</head> | |
| <tail><xsl:value-of select="$tail"/></tail> | |
| <part><xsl:value-of select="$seg"/></part> | |
| <part><xsl:value-of select="$new"/></part> | |
| </meta> | |
| </xsl:if> | |
| </xsl:for-each-group> | |
| </xsl:copy> | |
| </xsl:template> |
🤖 Prompt for AI Agents
In
eo-maven-plugin/src/main/resources/org/eolang/maven/parse/locals-to-aliases.xsl
around lines 21 to 37, the for-each-group currently targets bases starting with
"Φ." (so it never creates aliases for plain local names like "bar") and the
existence check compares tail against $new instead of the full "seg new" string;
change the group selection to operate on bases that do NOT start with "Φ." (e.g.
select="//@base[not(starts-with(., 'Φ.')) and not(. = $qqs)]") so locals are
processed, and update the existence test to compare tail against the computed
$tail (not $new) so the meta-alias presence check is correct.
| package org.eolang.maven; | ||
|
|
||
| import com.jcabi.matchers.XhtmlMatchers; | ||
| import com.jcabi.xml.XML; |
There was a problem hiding this comment.
Remove unused import to fix qulice failure.
com.jcabi.xml.XML is not used. This triggers the reported PMD/Checkstyle error.
Apply this diff:
-import com.jcabi.xml.XML;📝 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.
| import com.jcabi.xml.XML; |
🧰 Tools
🪛 GitHub Actions: qulice
[error] 8-8: PMD: Unused import 'com.jcabi.xml.XML' (UnnecessaryImport)
🤖 Prompt for AI Agents
In eo-maven-plugin/src/test/java/org/eolang/maven/MjParseTest.java around line
8, remove the unused import statement "com.jcabi.xml.XML" which is causing the
qulice/PMD/Checkstyle failure; delete that import line and re-run the
build/tests to confirm the linter error is resolved.
1 participant
In this pull I've implemented
locals-to-aliasesoptimization for local objects inMjParse.see #4529
Summary by CodeRabbit