Skip to content

Conversation

@h1alexbel
Copy link
Member

@h1alexbel h1alexbel commented Oct 17, 2025

In this PR I've implemented correct FQN resolution for object @base of compact arrays. New XSL resolve-compacts resolves all @compact:oname left during the parsing in CompactArrayFqn.java for precise FQN construction, taking into account both: objects in scope, and present +aliase-s.

see #4504

Summary by CodeRabbit

Release Notes

  • New Features

    • Added parsing support for compact arrays, including arrays with defined first parts.
    • Enhanced support for aliased sprintf formatting operations.
    • Improved composite name resolution in array contexts.
  • Tests

    • Added comprehensive test coverage for compact array operations, array formatting, and composite name handling.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 17, 2025

Walkthrough

This PR adds comprehensive support for compact array parsing in the EO language parser. It includes new test resources for integration and parser tests, modifies the CompactArrayFqn.java logic to handle HOME and XI prefix conditions, adds XSLT templates for FQN resolution with alias lookup and default Φ prefix application, and updates existing test assertions to reference fully-qualified names.

Changes

Cohort / File(s) Change Summary
Integration & Maven Plugin Tests
eo-integration-tests/src/test/resources/org/eolang/snippets/compact-array.yaml, eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/compact-array.yaml
New test resource files defining compact array test cases with regex expectations and sprintf formatting examples
CompactArrayFqn Logic Update
eo-parser/src/main/java/org/eolang/parser/CompactArrayFqn.java
Modified FQN construction: when HOME() and XI() are both null, keep fqn as name; when HOME() exists and XI() is null, prefix with Φ.org.eolang; when XI() is not null, use ξ prefix; eliminated intermediate base variable
CompactArrayFqn Test Data
eo-parser/src/test/java/org/eolang/parser/CompactArrayFqnTest.java
Added new CSV test case: "tt.x *55,tt.x"
XSLT FQN Resolution
eo-parser/src/main/resources/org/eolang/parser/parse/build-fqns.xsl
Added XSLT template for FQN resolution with pattern matching; resolution order: local scope → alias definition → default Φ.org.eolang prefix (duplicate template present)
Parser Syntax Test Resources
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array.yaml, eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array-with-defined-first-part.yaml, eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-sprintf-aliased.yaml, eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name-aliased.yaml
New test resource files for compact array and aliased sprintf syntax validation
Parser Test Assertions Updates
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name.yaml, eo-parser/src/test/resources/org/eolang/parser/eo-syntax/validate-resolve-before-stars.yaml
Updated AST path assertions to reference fully-qualified names (Φ.org.eolang.tt.sprintf, Φ.org.eolang.foo.bar) and replaced 'sprintf' tokens with 'QQ.tt.sprintf'

Sequence Diagram

sequenceDiagram
    participant Parser
    participant CompactArrayFqn
    participant FQN Resolution

    Parser->>CompactArrayFqn: process base attribute
    
    alt XI() is not null
        CompactArrayFqn->>FQN Resolution: set fqn = "ξ.<name>"
    else HOME() is present and XI() is null
        CompactArrayFqn->>FQN Resolution: set fqn = "Φ.org.eolang.<name>"
    else HOME() and XI() both null
        CompactArrayFqn->>FQN Resolution: keep fqn = "<name>"
    end
    
    FQN Resolution->>Parser: return resolved fqn
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The changes include conditional logic refactoring in CompactArrayFqn.java, XSLT template additions with a duplicate pattern, and multiple test resource files requiring pattern consistency verification. While many files are added, they follow consistent structures; however, the logic density in the Java and XSLT components and the presence of duplicate XSLT templates warrant careful review.

Possibly related PRs

Suggested labels

core

Suggested reviewers

  • maxonfjvipon
  • yegor256

Poem

🐰 Arrays bundled tight and neat,
With FQN logic so sweet,
Φ and ξ dance in harmony,
Compact parsing, wild and free,
The parser hops to victory! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "bug(#4504): FQN resolution for compact array base" accurately reflects the core changes in this changeset. The primary modifications include updates to CompactArrayFqn.java which implements FQN resolution logic for compact arrays, additions to build-fqns.xsl with new XSLT templates for resolving FQNs, and multiple test resources and test cases that validate this FQN resolution behavior. The title is concise, specific, and clearly communicates that this is a bug fix addressing issue #4504 related to how fully qualified names are resolved for the base of compact arrays. This gives reviewers a clear understanding of the PR's primary purpose when scanning commit history.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d845d29 and 1d8a837.

📒 Files selected for processing (4)
  • eo-parser/src/main/java/org/eolang/parser/CompactArrayFqn.java (1 hunks)
  • eo-parser/src/main/resources/org/eolang/parser/parse/build-fqns.xsl (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/validate-resolve-before-stars.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • eo-parser/src/test/resources/org/eolang/parser/eo-packs/parse/validate-resolve-before-stars.yaml
  • eo-parser/src/main/java/org/eolang/parser/CompactArrayFqn.java
🧰 Additional context used
🧠 Learnings (1)
📚 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-parser/src/main/resources/org/eolang/parser/parse/build-fqns.xsl
⏰ 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: ort
  • GitHub Check: mvn (ubuntu-24.04, 23)
  • GitHub Check: mvn (windows-2022, 23)
  • GitHub Check: mvn (ubuntu-24.04, 17)
  • GitHub Check: mvn (macos-15, 23)
  • GitHub Check: benchmark
  • GitHub Check: qulice
  • GitHub Check: integration
  • GitHub Check: snippets
  • GitHub Check: trufflehog
🔇 Additional comments (6)
eo-parser/src/test/java/org/eolang/parser/CompactArrayFqnTest.java (1)

23-23: LGTM! Good test coverage for in-scope dotted names.

The new test case verifies that dotted names like tt.x remain unchanged when the first component is in scope, complementing the existing coverage for QQ-prefixed and $-prefixed compact arrays.

eo-parser/src/main/resources/org/eolang/parser/parse/build-fqns.xsl (5)

194-194: Pattern correctly excludes simple names and already-qualified FQNs.

The regex pattern ^\w+(?:\.\w+)+$ properly matches multi-part dotted names (e.g., tt.x, foo.bar.baz) while excluding:

  • Simple names without dots (handled by existing templates)
  • Already-qualified names starting with Φ. or ξ

196-197: Alias resolution logic looks correct.

The XPath correctly extracts the head of the dotted name and looks up the corresponding alias from document metadata. Using part[last()] to retrieve the FQN is appropriate for the alias structure.

Based on learnings: Aliases expand to canonical forms (Φ and ξ roots) during XMIR decoration.


209-213: Default FQN construction assumes org.eolang namespace.

The fallback case prefixes unresolved names with Φ.org.eolang., which is the canonical form for the EO standard library. Verify this assumption is correct for all compact array base references that don't match an alias or in-scope object.

Based on learnings: Φ.org.eolang is the canonical form that gets validated after alias expansion.


194-215: No duplicate template found—concern is resolved.

Verification confirms the template appears exactly once in the file at line 194. The distinctive match pattern with Greek letter checks (Φ. and ξ) is unique, and no duplicates exist. The AI summary claiming two identical templates were added was inaccurate.


194-215: The scope check is intentional and correctly designed for its purpose.

The @base template (lines 194-215) is explicitly documented to perform LOCAL scope resolution for compact array references, not recursive ancestor search. The XPath parent::o/parent::o/o[@name = $head] correctly checks siblings in the immediate scope. When an object isn't found locally, the template properly falls back to the Φ.org.eolang prefix. This is distinct from the fqn template (line 141), which handles deep recursive ancestor search using explicit apply-templates recursion for different use cases. Test cases like sugared-array-with-composite-name.yaml confirm this behavior works as designed.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/compact-array.yaml (1)

13-16: Strengthen the assertion to catch unresolved compacts.

Add an assertion that no @compact:* bases leak into this pack stage.

Example:

 asserts:
   - /object[not(errors)]
   - //java[contains(text(), 'base="Φ.org.eolang.tt.sprintf"')]
+  - /object[count(//o[starts-with(@base,'@compact:')])=0]
eo-parser/src/main/java/org/eolang/parser/CompactArrayFqn.java (1)

49-57: Good sentinel approach; add a brief contract and a defensive guard.

The @compact:%s marker is clear and plays well with resolve-compacts.xsl. Please:

  • Document in class Javadoc that @compact:* is a transient marker resolved later; unresolved markers should be an error.
  • Optionally guard against empty NAME() to avoid emitting "@compact:" (assert or precondition).

Example:

 final class CompactArrayFqn implements Text {
+    /**
+     * Produces a transient FQN:
+     * - "@compact:<name>" when unresolved; resolved later by resolve-compacts.xsl.
+     * - "Φ.org.eolang.<name>" when HOME present.
+     * - "ξ.<name>" when XI present.
+     */
     @Override
     public String asString() {
         final String name = this.context.NAME().stream()
             .map(ParseTree::getText)
             .collect(Collectors.joining("."));
+        if (name.isEmpty()) {
+            throw new IllegalStateException("Compact array name is empty");
+        }
         final String fqn;
         ...
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name.yaml (1)

8-9: Also assert that no @compact: remains after canonicalization.*

Prevents regressions where compacts aren’t fully resolved.

 asserts:
   - /object[not(errors)]
   - //o[@base='Φ.org.eolang.foo.bar']/o[@base='Φ.org.eolang.x']
   - //o[@base='Φ.org.eolang.foo.bar']//o[@base='Φ.org.eolang.y']
+  - /object[count(//o[starts-with(@base,'@compact:')])=0]
eo-parser/src/test/resources/org/eolang/parser/eo-packs/parse/validate-resolve-before-stars.yaml (1)

5-9: Sheet order looks right here; add an explicit safety check.

Nice that resolve-compacts.xsl runs before validate/resolve-before-stars in this test. Add a check that no @compact:* nodes remain.

 asserts:
   - /object/errors/error[@line='3' and @severity='error']
   ...
   - /object[count(//o[@before-star])=0]
+  - /object[count(//o[starts-with(@base,'@compact:')])=0]
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0134ada and ce56e1d.

📒 Files selected for processing (12)
  • eo-integration-tests/src/test/resources/org/eolang/snippets/compact-array.yaml (1 hunks)
  • eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/compact-array.yaml (1 hunks)
  • eo-parser/src/main/java/org/eolang/parser/CompactArrayFqn.java (1 hunks)
  • eo-parser/src/main/java/org/eolang/parser/EoSyntax.java (1 hunks)
  • eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (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/validate-resolve-before-stars.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array-with-defined-first-part.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-sprintf-aliased.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name-aliased.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name.yaml (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: copyrights
eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl

[warning] 1-1: Missed: resolve-compacts.xsl

⏰ 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). (5)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: trufflehog
  • GitHub Check: benchmark
  • GitHub Check: mvn (windows-2022, 23)
  • GitHub Check: mvn (macos-15, 23)
🔇 Additional comments (9)
eo-parser/src/test/java/org/eolang/parser/CompactArrayFqnTest.java (1)

22-22: Test expectation updated correctly for new compact array behavior.

The change from "foo *1,foo" to "foo *1,@compact:foo" correctly reflects the new behavior where simple names in compact arrays are prefixed with @compact: during parsing before being resolved by the XSLT transformation.

eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array.yaml (1)

1-17: LGTM! Test correctly verifies compact array base resolution.

The test properly validates that tt.sprintf (without an alias) resolves to Φ.org.eolang.tt.sprintf in the compact array context, which is the expected default namespace behavior.

eo-integration-tests/src/test/resources/org/eolang/snippets/compact-array.yaml (1)

1-18: LGTM! Good internationalization test coverage.

The integration test appropriately validates compact array functionality with non-ASCII arguments (上海), ensuring the feature works correctly with internationalized content.

eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name-aliased.yaml (1)

1-16: LGTM! Test correctly verifies alias-based resolution.

The test properly validates that bar resolves to Φ.org.eolang.foo.bar via the declared alias, and that child elements x and y are also fully qualified with the EO namespace.

eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-sprintf-aliased.yaml (1)

1-18: LGTM! Test correctly verifies aliased sprintf resolution.

The test appropriately validates that sprintf resolves to Φ.org.eolang.tt.sprintf via the declared alias, and that the string argument is properly typed as Φ.org.eolang.string.

eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (3)

14-16: LGTM! Variable extraction logic is correct.

The string manipulation correctly handles both simple names (e.g., "foo") and composite names (e.g., "foo.bar.baz") by extracting the head before the first dot and the tail after it.


19-23: Verify whether single-level scope lookup is intended design or a limitation.

The XPath parent::o/parent::o/o[@name = $head] limits scope resolution to checking if the name is available as a child of the grandparent (i.e., at the same level as the parent element). This implements single-level local scope rather than searching the entire ancestor chain. The only test file referencing this stylesheet (validate-resolve-before-stars.yaml) tests star expansion validation, not nested scope scenarios. Confirm whether this single-level lookup matches the EoLang language semantics or if ancestor-chain resolution should be supported.


24-28: Verify alias resolution with composite names.

When an alias is found (line 26), the code sets base to $alias directly. However, if $q contains dots (e.g., "foo.bar"), and an alias exists for "foo" (e.g., "org.eolang.baz"), this would resolve to just "org.eolang.baz" instead of "org.eolang.baz.bar".

Confirm whether the tail portion should be appended to the alias when present:

 <xsl:when test="string-length($alias) &gt; 0">
   <xsl:attribute name="base">
-    <xsl:value-of select="$alias"/>
+    <xsl:value-of select="if (string-length($tail) &gt; 0) then concat($alias, '.', $tail) else $alias"/>
   </xsl:attribute>
 </xsl:when>

Check existing test cases to verify the expected behavior:

eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array-with-defined-first-part.yaml (1)

8-9: Mixed resolution behavior is intentional and correctly implemented.

The resolve-compacts.xsl logic confirms the expected behavior:

  • Line 19: Checks if the first part of a composite name (e.g., foo in foo.bar) is defined in parent scope
  • If defined locally (line 21): The composite name is kept as-is (foo.bar)
  • If not defined (line 31): Simple references are resolved to the default namespace (Φ.org.eolang.x, Φ.org.eolang.y)

In the test case, foo is defined in scope, so foo.bar remains unchanged. However, x and y are not defined references and correctly resolve to the EO namespace.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name-aliased.yaml (1)

6-10: Add a guard assert to ensure all compacts are fully resolved

To catch regressions, assert that no "@compact:" bases remain after the pipeline.

Proposed addition:

 asserts:
   - /object[not(errors)]
   - //o[@base='Φ.org.eolang.foo.bar']/o[@base='Φ.org.eolang.x']
   - //o[@base='Φ.org.eolang.foo.bar']//o[@base='Φ.org.eolang.y']
+  - count(//o[starts-with(@base, '@compact:')])=0
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-sprintf-aliased.yaml (1)

6-9: Strengthen assertions to cover stdout alias and no residual compacts

Currently only sprintf is asserted. Also assert stdout’s FQN and absence of any unresolved compacts.

Suggested additions:

 asserts:
   - /object[not(errors)]
   - //o[@base='Φ.org.eolang.tt.sprintf']/o[@base='Φ.org.eolang.string']
+  - //o[@base='Φ.org.eolang.io.stdout']
+  - count(//o[starts-with(@base, '@compact:')])=0
eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (1)

22-22: Scope probe uses fixed depth; prefer nearest lexical owner

parent::o/parent::o may miss valid bindings or overreach depending on nesting. Use the nearest owner “o”.

Replace:

-      <xsl:when test="parent::o/parent::o/o[@name = $head]">
+      <xsl:when test="ancestor::o[1]/o[@name = $head]">

This checks the immediate lexical scope reliably in XMIR hierarchies.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0134ada and 06d878a.

📒 Files selected for processing (12)
  • eo-integration-tests/src/test/resources/org/eolang/snippets/compact-array.yaml (1 hunks)
  • eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/compact-array.yaml (1 hunks)
  • eo-parser/src/main/java/org/eolang/parser/CompactArrayFqn.java (1 hunks)
  • eo-parser/src/main/java/org/eolang/parser/EoSyntax.java (1 hunks)
  • eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (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/validate-resolve-before-stars.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array-with-defined-first-part.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-sprintf-aliased.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name-aliased.yaml (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name.yaml (1 hunks)
🔇 Additional comments (9)
eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/compact-array.yaml (1)

1-24: LGTM! Well-structured test resource for compact array transpilation.

The test resource correctly validates the new FQN resolution behavior, ensuring that tt.sprintf is resolved to Φ.org.eolang.tt.sprintf after transpilation. The use of the +alias directive and the assertion structure are appropriate.

eo-integration-tests/src/test/resources/org/eolang/snippets/compact-array.yaml (1)

1-18: LGTM! Good integration test with internationalization coverage.

The integration test appropriately validates end-to-end execution of compact array resolution with internationalized input (上海), ensuring the full pipeline works correctly from parsing through execution.

eo-parser/src/test/resources/org/eolang/parser/eo-syntax/sugared-array-with-composite-name.yaml (1)

8-9: LGTM! Assertions correctly reflect the new FQN resolution.

The updated assertions appropriately expect Φ.org.eolang.foo.bar since foo is not defined in the local scope. This aligns with the new compact array FQN resolution behavior.

eo-parser/src/test/java/org/eolang/parser/CompactArrayFqnTest.java (1)

22-22: LGTM! Test expectation correctly updated.

The updated expectation @compact:foo correctly reflects the new CompactArrayFqn.asString() behavior when neither HOME nor XI contexts are present.

eo-parser/src/test/resources/org/eolang/parser/eo-packs/parse/validate-resolve-before-stars.yaml (2)

6-6: LGTM! Resolve-compacts transform correctly integrated.

The addition of resolve-compacts.xsl appropriately extends the transformation pipeline to handle compact reference resolution.


14-14: LGTM! Assertion updated to reflect resolved base name.

The assertion correctly expects Φ.org.eolang.sprintf after the compact reference resolution, validating the new transformation behavior.

eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array.yaml (1)

1-17: LGTM! Parser test resource correctly validates compact array resolution.

The test appropriately validates that tt.sprintf is resolved to Φ.org.eolang.tt.sprintf during parsing, complementing the transpilation tests.

eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array-with-defined-first-part.yaml (1)

1-16: LGTM! Test correctly validates scope-aware FQN resolution.

This test appropriately verifies that when foo is defined in the local scope (line 13), the compact array reference foo.bar remains as foo.bar rather than being resolved to Φ.org.eolang.foo.bar. However, the unresolved child references (x and y) are correctly expected to be resolved to Φ.org.eolang.x and Φ.org.eolang.y. This demonstrates proper scope-aware resolution behavior.

eo-parser/src/main/java/org/eolang/parser/EoSyntax.java (1)

89-90: Duplication is intentional—confirmed two-pass transformation approach.

The transforms validate-before-stars.xsl and resolve-before-stars.xsl appear at both lines 89-90 and lines 102-103 after resolve-compacts.xsl (line 101). This is a deliberate two-pass validation/resolution pattern:

  1. First pass (lines 89-90): Validate and resolve initial compact references
  2. Compact resolution (line 101): Transform @compact:oname markers
  3. Second pass (lines 102-103): Re-validate and resolve newly created references

The transformation pipeline comment ("order of transformations matters"; "strongly coupled") confirms this multi-phase approach is intentional. The pattern is logically sound: the first pass processes initial references, then resolve-compacts creates new references that require a second validation/resolution pass.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2025

🚀 Performance Analysis

All 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
Test Base Score PR Score Change % Change Unit Mode
benchmarks.XmirBench.xmirToEO 150.517 172.109 21.592 14.35% ms/op Average Time

⚠️ Performance loss: benchmarks.XmirBench.xmirToEO is slower by 21.592 ms/op (14.35%)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (1)

2-5: Copyright header is now present.

The copyright warning from the previous review has been addressed. The file now includes the proper SPDX header consistent with the repository's standards.

🧹 Nitpick comments (1)
eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (1)

18-18: Consider validating non-empty $q.

If the input is exactly @compact: (with no suffix), $q will be empty, leading to a malformed FQN like Φ.org.eolang. in the fallback case. Consider adding validation or documenting the assumption that @compact: is always followed by a non-empty name.

Example validation approach:

<xsl:variable name="q" select="substring-after(., '@compact:')"/>
<xsl:if test="string-length($q) = 0">
  <xsl:message terminate="yes">
    <xsl:text>Error: empty name after @compact: in </xsl:text>
    <xsl:value-of select="."/>
  </xsl:message>
</xsl:if>
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce56e1d and a2c2ece.

📒 Files selected for processing (4)
  • eo-integration-tests/src/test/resources/org/eolang/snippets/compact-array.yaml (1 hunks)
  • eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/compact-array.yaml (1 hunks)
  • eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (1 hunks)
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • eo-integration-tests/src/test/resources/org/eolang/snippets/compact-array.yaml
  • eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/compact-array.yaml
  • eo-parser/src/test/resources/org/eolang/parser/eo-syntax/compact-array.yaml
⏰ 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). (4)
  • GitHub Check: mvn (macos-15, 23)
  • GitHub Check: actionlint
  • GitHub Check: mvn (windows-2022, 23)
  • GitHub Check: benchmark
🔇 Additional comments (2)
eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (2)

18-19: LGTM: Clean variable extraction logic.

The use of concat($q, '.') with substring-before is an elegant way to extract the first component while handling both simple names (e.g., "sprintf") and dotted names (e.g., "sprintf.format") correctly.


22-26: Scope resolution logic appears sound.

The XPath parent::o/parent::o/o[@name = $head] correctly checks for objects in the enclosing scope (siblings of the compact array's parent object), which aligns with the expected semantics for local name resolution.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (1)

8-8: Fix grammatical error in comment.

Change "it's FQN" to "its FQN" (possessive form, not contraction).

Apply this diff:

-    Here we go through all `@compact:oname` bases and resolve it's FQN:
+    Here we go through all `@compact:oname` bases and resolve its FQN:
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f654a36 and d845d29.

📒 Files selected for processing (1)
  • eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T14:21:45.031Z
Learnt from: h1alexbel
PR: objectionary/eo#4637
File: eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl:27-31
Timestamp: 2025-10-17T14:21:45.031Z
Learning: In the EO language aliasing system, only concrete objects (complete paths to objects, e.g., `org.eolang.io.stdout`) can be aliased, not intermediate packages or partial paths (e.g., `org.eolang.io`). This means aliases always point to final objects, not package prefixes.

Applied to files:

  • eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl
⏰ 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: benchmark
  • GitHub Check: integration
  • GitHub Check: qulice
  • GitHub Check: snippets
  • GitHub Check: mvn (windows-2022, 23)
  • GitHub Check: trufflehog
  • GitHub Check: mvn (ubuntu-24.04, 17)
  • GitHub Check: mvn (ubuntu-24.04, 23)
  • GitHub Check: mvn (macos-15, 23)
  • GitHub Check: ort
🔇 Additional comments (4)
eo-parser/src/main/resources/org/eolang/parser/parse/resolve-compacts.xsl (4)

1-6: LGTM! Copyright header properly added.

The SPDX copyright header has been correctly added, addressing the previous pipeline warning. The file structure and namespace declarations are appropriate.


13-17: LGTM! Standard identity template correctly implemented.

The identity template follows XSLT best practices for preserving document structure while allowing selective transformations.


18-39: Resolution logic correctly implements FQN resolution per EO aliasing constraints.

The three-case resolution logic is sound:

  1. Local scope check: Returns the qualified name if the head exists in parent scope
  2. Alias resolution: Returns the aliased FQN using part[last()] to correctly handle composite alias definitions
  3. Default namespace: Falls back to Φ.org.eolang.* for standard library objects

The alias resolution correctly uses only the alias value without appending any tail from the composite name. This is correct given the EO language constraint that only concrete objects (not packages) can be aliased, which prevents scenarios where a composite name would have an aliased head requiring tail concatenation.

Based on learnings.


23-27: Single-level scope check is correct by design.

The XPath parent::o/parent::o/o[@name = $head] intentionally checks only the immediate parent scope, which aligns with EO's documented scoping semantics for compact notation resolution. The file's comment clarifies that step 1 resolves names "in the current scope"—meaning the immediate parent scope—not the full ancestor chain.

This design choice is consistent with the codebase: other XSLT transformations use ancestor::o patterns when full ancestor chain traversal is needed (e.g., line 26 in validate-abstracts.xsl). No test cases or bug reports indicate that nested scope resolution is required for compact notation, confirming this is working as intended.

@h1alexbel
Copy link
Member Author

@maxonfjvipon please check

Copy link
Member

@maxonfjvipon maxonfjvipon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h1alexbel can we solve this problem without adding new @compact attribute and new XSL, but somehow just in XeListener?

@h1alexbel
Copy link
Member Author

@h1alexbel can we solve this problem without adding new @compact attribute and new XSL, but somehow just in XeListener?

@maxonfjvipon seems that we need full XMIR(with metas and top object, to check the scope). WDYT?

@h1alexbel
Copy link
Member Author

h1alexbel commented Oct 20, 2025

@maxonfjvipon ^ WDYT?

@maxonfjvipon
Copy link
Member

@h1alexbel can't we just map EO with such compact arrays to valid XMIR and leave this scope resolving to build-fqns?

@h1alexbel
Copy link
Member Author

h1alexbel commented Oct 20, 2025

@maxonfjvipon seems that no, without compact FQN resolution, in this snippet:

+alias org.eolang.io.stdout
+package org.eolang.snippets

[args] > program
  io.stdout > @
    tt.sprintf *1
      "Hello, %s\n"
      args.at 0

The tt.sprintf will be translated as such:

<o base="tt.sprintf"...>

Later, in to-java.xsl this will trigger an error here:

<xsl:choose>
  <xsl:when test="$first='Φ'">
    <xsl:text>Phi.Φ</xsl:text>
  </xsl:when>
  <xsl:when test="$first='ξ'">
    <xsl:value-of select="$rho"/>
  </xsl:when>
  <xsl:otherwise>
    <xsl:message terminate="yes">
      <xsl:text>FQN must start with either with Φ or ξ, but </xsl:text>
      <xsl:value-of select="$first"/>
      <xsl:text> found</xsl:text>
    </xsl:message>
  </xsl:otherwise>
</xsl:choose>

@maxonfjvipon
Copy link
Member

@h1alexbel maybe it's better to fix the original XSL(s) which append corresponding prefixes and resolve FQNs

@h1alexbel
Copy link
Member Author

@maxonfjvipon updated. Take a look, please

@h1alexbel
Copy link
Member Author

@maxonfjvipon reminder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants