bug(#4559): auto phi formation correctly parsed with application precedences #4560
Conversation
…th application precedence
… for brevity and consistency
WalkthroughNormalizes EO application strings into XMIR bases in XeEoListener via a new helper and regex extraction of the last application segment, updates auto-phi formation to use the normalized base, tightens an XSL removal predicate to require not(@pos), and adds/updates parser, transpiler, and integration tests for various application forms. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor EO as EO source
participant Listener as XeEoListener
participant Helper as eoApplicationToXmir()
participant Regex as LAST_APPLICATION
participant XMIR as XMIR builder
EO->>Listener: encounter application string
Listener->>Helper: eoApplicationToXmir(application)
Helper-->>Listener: normalized base (handles Q/Φ/ρ prefixes or defaults to ξ.ρ.<base>)
Listener->>Regex: match last dotted identifier
Regex-->>Listener: optional ".<last>"
Listener->>XMIR: startAutoPhiFormation(ctx, computed abase)
XMIR-->>Listener: create/link φ node
note right of Listener: flow now uses normalization + last-segment extraction
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
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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). (1)
🔇 Additional comments (1)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-application-precedence.yaml (1)
1-18: Good targeted test for precedence; add a guard to avoid regressions toward the outer finisherAdd a negative check so we don’t accidentally switch back to using the outer “.ttt”.
Apply:
- /object[not(errors)] - //o[not(@base) and @name='ap🌵34' and o[@name='φ' and @base='.what']] + - //o[not(@base) and @name='ap🌵34' and not(o[@name='φ' and @base='.ttt'])] - //o[not(@base) and @name='ap🌵66' and o[@name='φ' and @base='.contains']]eo-parser/src/main/java/org/eolang/parser/XeEoListener.java (2)
40-44: Tighten the regex and document intentCurrent pattern limits names to ASCII
\wand lacks intent docs. Prefer a Unicode/EO‑friendly char class and clarify “first match wins”.Apply:
- /** - * Application precedence. - */ - private static final Pattern A_PRECEDENCE = Pattern.compile("\\([^()]*\\)\\.(\\w+)"); + /** + * Application precedence: extract the first method that immediately follows + * a parenthesized expression. E.g. "((text orgn).what b).ttt" -> ".what". + * Intentionally uses the first leftmost match. + */ + private static final Pattern A_PRECEDENCE = + Pattern.compile("\\([^()]*\\)\\.([\\p{L}\\p{N}_-]+)");If EO allows additional symbols in method names, adjust the class accordingly and add a test.
1219-1231: Behavioral change is correct; minor nitsLogic matches tests. Two nits:
- Use concatenation over String.format here.
- Consider asserting the “first match” behavior with an extra test case like "(((a).b).c).d".
Apply:
- final Matcher matcher = XeEoListener.A_PRECEDENCE.matcher(application); + final Matcher matcher = XeEoListener.A_PRECEDENCE.matcher(application); final String abase; if (matcher.find()) { - abase = String.format(".%s", matcher.group(1)); + abase = "." + matcher.group(1); } else { - abase = String.format("ξ.ρ.%s", application); + abase = "ξ.ρ." + application; }Add a unit/integration test that contains multiple candidate matches to lock in the intended precedence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java(3 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-application-precedence.yaml(1 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-nested-vapplication.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-nested-vapplication.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). (11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: integration
- GitHub Check: ebnf
- GitHub Check: trufflehog
- GitHub Check: qulice
- GitHub Check: snippets
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: ort
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: mvn (windows-2022, 23)
🔇 Additional comments (2)
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-nested-vapplication.yaml (1)
8-9: ap🌵 rename and base path update look correctMatches the new precedence-based φ derivation and keeps the nested reference consistent.
Are there any remaining tests still expecting a🌵… names? If yes, update them to ap🌵….
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java (1)
13-14: Regex imports are fineRequired for the new precedence extraction.
|
@h1alexbel some nitpick comments from Rabbit make sense, maybe you can use them |
… with application precedence
… with application precedence
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/transpiles-auto-phi-formation-with-application-precedence.yaml (1)
14-15: Reduce brittleness: avoid hard-coding cactus index in asserts.The numeric part of ap🌵NN may change; assert more robustly using combined contains() and also fold to keep lines short.
- - //java[contains(text(), 'Φ.custom-map.ap🌵46.φ')] - - //java[contains(text(), 'Φ.custom-map.ap🌵46.φ.ρ.ρ.α0')] + - > + //java[contains(text(), 'Φ.custom-map.ap') + and contains(text(), '.φ')] + - > + //java[contains(text(), 'Φ.custom-map.ap') + and contains(text(), '.φ.ρ.ρ.α0')]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/transpiles-auto-phi-formation-with-application-precedence.yaml(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: yamllint
eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/transpiles-auto-phi-formation-with-application-precedence.yaml
[error] 16-16: yamllint: line-length violation in YAML file. line too long (90 > 80 characters) (line-length).
[error] 17-17: yamllint: line-length violation in YAML file. line too long (87 > 80 characters) (line-length).
⏰ 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: qulice
- GitHub Check: ort
- GitHub Check: integration
- GitHub Check: snippets
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: ebnf
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: trufflehog
🔇 Additional comments (2)
eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/transpiles-auto-phi-formation-with-application-precedence.yaml (2)
17-19: Name consistency sanity-check (rrbb1 vs rrbbb1).You assert both “Phi rrbb1 …” and later “rrbbb1 = new PhMethod(rrbbb1, "orgn");”. That may be correct, but please confirm this isn’t a typo and that both identifiers exist in the generated Java.
4-11: LGTM: sheets ordering and EO input look appropriate for exercising precedence.Nothing else blocking from this file after addressing lint and optional assert hardening.
Also applies to: 21-26
.../eolang/maven/transpile-packs/transpiles-auto-phi-formation-with-application-precedence.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java (2)
13-14: Regex scope and resiliency: align with NAME token, no need for multiple matches
- The pattern is anchored with
$, so there can be at most one match; the later loop (handled below) isn’t needed.- Verify allowed identifier chars in EO’s
NAMEtoken. If hyphens or non-ASCII letters are valid, the current[a-zA-Z_]\\w*will miss them and fall back toξ.ρ.<application>incorrectly.Proposed broader, grammar-friendly pattern (if NAME allows Unicode letters/digits and
-):- private static final Pattern LAST_APPLICATION = Pattern.compile("\\)\\.([a-zA-Z_]\\w*)$"); + // Matches: ...).<name> at end; name starts with letter/_ then letters/digits/_/- + private static final Pattern LAST_APPLICATION = + Pattern.compile("\\)\\.([\\p{L}_][\\p{L}\\p{N}_-]*)$");If hyphen or Unicode aren’t allowed by grammar, keep the current class but add a brief Javadoc with an example input to document intent.
Also applies to: 40-44
1220-1228: Simplify matcher usage and avoid String.format in hot pathSingle anchored match suffices; drop the do/while and use straightforward concatenation.
Apply:
- final Matcher matcher = XeEoListener.LAST_APPLICATION.matcher(application); - String abase; - if (matcher.find()) { - do { - abase = String.format(".%s", matcher.group(1)); - } while (matcher.find()); - } else { - abase = String.format("ξ.ρ.%s", application); - } + final Matcher m = XeEoListener.LAST_APPLICATION.matcher(application); + final String abase = m.find() ? ("." + m.group(1)) : ("ξ.ρ." + application);eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-application-precedence.yaml (1)
6-10: Good coverage of “dot-after-parens” path; add a fallback-case assertThese asserts validate the new regex path well. Please add one case that exercises the fallback branch (no closing paren before the trailing segment), to guard against regressions in
ξ.ρ.<application>behavior.Minimal example to consider adding (pseudo‑diff to this YAML):
# Extra assert idea: # - Ensure when head has no ')', phi base falls back to ξ.ρ.<application> # e.g., for "text orgn" case (or another simple head), expect no ".<name>" base. # - //o[not(@base) and @name='ap🌵NN' and o[@name='φ' and starts-with(@base,'ξ.ρ.')]]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/transpiles-auto-phi-formation-with-application-precedence.yaml(1 hunks)eo-parser/src/main/java/org/eolang/parser/XeEoListener.java(3 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-application-precedence.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/transpiles-auto-phi-formation-with-application-precedence.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). (12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: qulice
- GitHub Check: snippets
- GitHub Check: integration
- GitHub Check: ort
- GitHub Check: trufflehog
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: benchmark
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: ebnf
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: mvn (ubuntu-24.04, 11)
🔇 Additional comments (1)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java (1)
1232-1232: LGTM: using computed base (abase) in Auto Phi formationThis correctly swaps the previously hardcoded base with the derived one.
|
@maxonfjvipon please, take a look |
|
@maxonfjvipon @yegor256 this PR is currently in progress. Need to polish a few things |
…l test resource for EO snippets
…ects from auto-phi cleanup
|
@maxonfjvipon ready, please check |
|
@maxonfjvipon reminder |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-application.yaml (1)
9-9: Make XPath assertions resilient: avoid hardcoded auto-name index.Replace exact checks like @name='ap🌵34' with contains(@name,'ap🌵') to avoid brittle tests.
Affected files to update:
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-application.yaml
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-application-precedence.yaml
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-nested-vapplication.yaml
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/simple-auto-phi-formation.yaml
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation.yaml
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-simple-application.yaml
- eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/transpiles-auto-phi-formations.yaml
- eo-maven-plugin/src/test/resources/org/eolang/maven/transpile-packs/transpiles-auto-phi-formation-with-application-precedence.yaml
Note: eo-parser/src/main/resources/org/eolang/parser/parse/auto-phi-formation-restruct.xsl already uses contains(@name,'ap🌵').
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eo-integration-tests/src/test/resources/org/eolang/snippets/auto-phi-with-application-precedence.yaml(1 hunks)eo-parser/src/main/java/org/eolang/parser/XeEoListener.java(3 hunks)eo-parser/src/main/resources/org/eolang/parser/parse/auto-phi-formation-restruct.xsl(1 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-application.yaml(1 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-application-precedence.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-with-application-precedence.yaml
- eo-parser/src/main/java/org/eolang/parser/XeEoListener.java
🧰 Additional context used
🧠 Learnings (2)
📚 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/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-application.yamleo-parser/src/main/resources/org/eolang/parser/parse/auto-phi-formation-restruct.xsl
📚 Learning: 2025-08-22T08:25:17.517Z
Learnt from: h1alexbel
PR: objectionary/eo#4463
File: eo-maven-plugin/src/main/resources/org/eolang/maven/transpile/to-java.xsl:346-373
Timestamp: 2025-08-22T08:25:17.517Z
Learning: In EO, the idempotency attribute `ξ.xi🌵` is a special meta-attribute that appears inside formations and can be anonymous objects. When an attribute's bound value contains this sentinel object (checked via `bound/o[base='ξ.xi🌵']`), the entire attribute should be skipped from Java code generation at the attr template level, as these are idempotency markers rather than actual program logic.
Applied to files:
eo-parser/src/main/resources/org/eolang/parser/parse/auto-phi-formation-restruct.xsl
🪛 GitHub Actions: yamllint
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-application.yaml
[error] 1-1: yamllint: too many blank lines (1 > 0) (empty-lines)
⏰ 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). (13)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ebnf
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: qulice
- GitHub Check: trufflehog
- GitHub Check: snippets
- GitHub Check: ort
- GitHub Check: integration
- GitHub Check: benchmark
- GitHub Check: infer
🔇 Additional comments (2)
eo-integration-tests/src/test/resources/org/eolang/snippets/auto-phi-with-application-precedence.yaml (1)
14-26: LGTM: integration covers application-precedence path.The snippet exercises (text orgn).contains x under custom-map; good coverage for precedence in auto-phi formation.
Ensure this test fails on the pre-fix parser to guard against regressions.
eo-parser/src/main/resources/org/eolang/parser/parse/auto-phi-formation-restruct.xsl (1)
66-66: Scope cleanup to synthetic ξ. nodes; add rationale comment and a test*Keep not(@pos) to avoid pruning source-positioned ξ.* nodes; add an inline XSL comment documenting this intent and a unit test that asserts ξ.* elements with @pos inside an ap🌵 formation are preserved.
File: eo-parser/src/main/resources/org/eolang/parser/parse/auto-phi-formation-restruct.xsl (around line 66)
- <!-- Remove redundant ξ.X for void attributes, located inside the auto-phi formation --> + <!-- Remove redundant ξ.X for void attributes inside auto-phi formation. + Keep nodes that have @pos: they originate from the source and must not be pruned. --> <xsl:template match="o[starts-with(@base,'ξ.') and not(@pos) and ancestor::o[contains(@name,'ap🌵')][descendant::o[@base='∅' and @name=substring-after(current()/@base,'ξ.')]]]"/>No tests found under eo-parser/src/test/resources that assert preservation of ξ.* with @pos; add one that verifies ξ.* with @pos inside ap🌵 is kept.
.../src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-application.yaml
Outdated
Show resolved
Hide resolved
|
@maxonfjvipon updated. Take a look, please |
|
@maxonfjvipon reminder |
… precedence application
…eplacing Q with Φ directly
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-precedence-application.yaml (3)
8-8: Avoid brittle assert on exact auto‑phi nameAsserting the exact generated name
ap🌵34makes the test fragile against benign changes in naming. Use a predicate that checks the structural shape and a stable prefix instead.Apply this diff:
- - //o[not(@base) and @name='ap🌵34' and o[@name='φ' and @base='.j']/o[@base='.i']/o[@base='Φ.org.eolang.txt.text']/o[@base='ξ.ρ.args']] + - //o[not(@base) and starts-with(@name,'ap') and o[@name='φ' and @base='.j']/o[@base='.i']/o[@base='Φ.org.eolang.txt.text']/o[@base='ξ.ρ.args']]
9-9: Scope the second assert under the same auto‑phi to reduce false positivesAnchor the check for the second operand (
Φ.org.foo.b) within the detected auto‑phi subtree.Apply this diff:
- - //o[@base='.i' and not(@name)]/o[2][@base='Φ.org.foo.b'] + - //o[not(@base) and starts-with(@name,'ap')]//o[@base='.i' and not(@name)]/o[2][@base='Φ.org.foo.b']
6-9: Add an assert to guarantee full FQN alias expansion (no residual Q. bases)*This protects against regressions where
Q.aliases slip through.You can append this assert:
asserts: - /object[not(errors)] - //o[not(@base) and starts-with(@name,'ap') and o[@name='φ' and @base='.j']/o[@base='.i']/o[@base='Φ.org.eolang.txt.text']/o[@base='ξ.ρ.args']] - //o[not(@base) and starts-with(@name,'ap')]//o[@base='.i' and not(@name)]/o[2][@base='Φ.org.foo.b'] + - count(//o[starts-with(@base,'Q.')])=0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java(3 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-application.yaml(1 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-precedence-application.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eo-parser/src/main/java/org/eolang/parser/XeEoListener.java
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-application.yaml
🧰 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/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-precedence-application.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). (13)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ebnf
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: trufflehog
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: ort
- GitHub Check: benchmark
- GitHub Check: snippets
- GitHub Check: infer
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: qulice
- GitHub Check: integration
🔇 Additional comments (2)
eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-fqn-precedence-application.yaml (2)
1-6: LGTM: SPDX headers and YAML harness structure are correctNo issues with headers, yamllint directive, or the basic test harness keys.
8-8: Confirm emoji in generated name is stable across environmentsIf other tests also rely on emoji in auto‑phi names, fine. Otherwise, even with the relaxed predicate, ensure your CI environments uniformly handle UTF‑8 and don’t normalize names unexpectedly.
|
@maxonfjvipon updated. Take a look, please |
| String abase; | ||
| if (matcher.find()) { | ||
| do { | ||
| abase = String.format(".%s", matcher.group(1)); |
There was a problem hiding this comment.
@h1alexbel what if the base would be something like this: ^.x.^.y.^.z? You'll reduce it to z and append $.^. in front of it?
There was a problem hiding this comment.
@maxonfjvipon we won't reduce it, but will append the base "as is" to the ξ.ρ., so it looks like this: ξ.ρ.ρ.x.ρ.y.ρ.z. Should we remove first ρ. part from the start in this case, so the final one will look like ξ.ρ.x.ρ.y.ρ.z?
There was a problem hiding this comment.
@h1alexbel all this stuff with appending $.^ is not correct by default, we have a corresponding XSL transformation which does all of this if it's necessary. The main part of parsing this block is to properly create formation with auto name and with @ attribute and save everything else unchanged and leave it to the further processing
There was a problem hiding this comment.
@maxonfjvipon sometimes we need add redundant ξ.ρ in auto Phi formations to correctly processed by auto-phi-formation-restruct.xsl, especially on this line:
<!-- Remove redundant ξ.X for void attributes, located inside the auto-phi formation -->
<xsl:template match="o[starts-with(@base,'ξ.') and not(@pos) and ancestor::o[contains(@name,'ap🌵')][descendant::o[@base='∅' and @name=substring-after(current()/@base,'ξ.')]]]"/>Once we solve this puzzle, we can get rid of this sheet and appending ξ.ρ to the application base. WDYT? Am I missing something?
There was a problem hiding this comment.
@h1alexbel I think all of this stuff is over engineered and we're shooting to our leg by introducing such crutches. I strongly believe all of this auto naming processing can be done in XeListener without any regexp and further XSL processing, because you have two entry points for each AST element: enter and exit. It should be implemented much simpler in general
There was a problem hiding this comment.
@h1alexbel Let's resolve that puzzle first so we create right XML in XeListener for auto named formation with phi without further XSL processing and ap prefixes
There was a problem hiding this comment.
Let's resolve that puzzle first so we create right XML in XeListener for auto named formation with phi without further XSL processing and
apprefixes
@maxonfjvipon in this PR, right? Or we can merge this one, and resolve #4557 in parallel?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java (1)
717-719: Same helper caveats apply here.For
vapplicationHead()beginning withΦ̇/QQ.or a standalone root, current helper can mis-prefix. See helper fix below.eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-application-with-rho.yaml (1)
8-12: Good coverage; consider adding HOME and Q/QQ variants.Add asserts for:
Φ̇.org.eolang.* >> [...]to ensure HOME isn’t prefixed withξ.ρ.Q.org.* >> [...]andQQ.org.* >> [...]to verify alias normalization toΦ.andΦ̇.respectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java(7 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-application-with-rho.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/java/org/eolang/parser/XeEoListener.java
📚 Learning: 2025-08-22T08:25:17.517Z
Learnt from: h1alexbel
PR: objectionary/eo#4463
File: eo-maven-plugin/src/main/resources/org/eolang/maven/transpile/to-java.xsl:346-373
Timestamp: 2025-08-22T08:25:17.517Z
Learning: In EO, the idempotency attribute `ξ.xi🌵` is a special meta-attribute that appears inside formations and can be anonymous objects. When an attribute's bound value contains this sentinel object (checked via `bound/o[base='ξ.xi🌵']`), the entire attribute should be skipped from Java code generation at the attr template level, as these are idempotency markers rather than actual program logic.
Applied to files:
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java (3)
13-14: LGTM on imports.Needed for the new regex logic.
671-681: Vertical application base transformation is fine; ensure helper handles method-only cases.Good extraction via
verticalApplicationBase. Just relies oneoApplicationToXmirto correctly keepρ.*andΦ.*unchanged and to not prefix plainρ/Φ. Addressed by helper fix.
377-382: Handle QQ and standalone-root prefixes in eoApplicationToXmir
eoApplicationToXmir only covers^.→ρ.,Q.→Φ.and defaults everything else toξ.ρ.*, so inputs likeQQ.*,Φ̇*,Φorρ(no dot) get misnormalized. InvokeqqToGlobalPhifirst and add explicit checks forΦ̇*,Φ, andρ(with or without trailing.) before the default branch.⛔ Skipped due to 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.
…letters and hyphens in method names
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java (1)
1229-1237: SimplifystartAutoPhiFormationby removing redundant loop
LAST_APPLICATIONis anchored at end, somatcher.find()can succeed at most once—replace thedo/whilewith a singleif.- Fallback is safe: every caller passes
eoApplicationToXmir(…), ensuringapplicationis already a canonical XMIR base.--- a/eo-parser/src/main/java/org/eolang/parser/XeEoListener.java +++ b/eo-parser/src/main/java/org/eolang/parser/XeEoListener.java @@ -1229,8 +1229,7 @@ - String abase; - if (matcher.find()) { - do { - abase = String.format(".%s", matcher.group(1)); - } while (matcher.find()); - } else { - abase = application; - } + final String abase; + if (matcher.find()) { + abase = "." + matcher.group(1); + } else { + abase = application; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java(7 hunks)eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-application-with-hypens.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eo-parser/src/test/resources/org/eolang/parser/eo-syntax/auto-phi-formation-from-application-with-hypens.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 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/java/org/eolang/parser/XeEoListener.java
📚 Learning: 2025-08-22T08:25:17.517Z
Learnt from: h1alexbel
PR: objectionary/eo#4463
File: eo-maven-plugin/src/main/resources/org/eolang/maven/transpile/to-java.xsl:346-373
Timestamp: 2025-08-22T08:25:17.517Z
Learning: In EO, the idempotency attribute `ξ.xi🌵` is a special meta-attribute that appears inside formations and can be anonymous objects. When an attribute's bound value contains this sentinel object (checked via `bound/o[base='ξ.xi🌵']`), the entire attribute should be skipped from Java code generation at the attr template level, as these are idempotency markers rather than actual program logic.
Applied to files:
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java
⏰ 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). (12)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: trufflehog
- GitHub Check: ort
- GitHub Check: snippets
- GitHub Check: qulice
- GitHub Check: integration
- GitHub Check: ebnf
- GitHub Check: benchmark
🔇 Additional comments (7)
eo-parser/src/main/java/org/eolang/parser/XeEoListener.java (7)
13-14: LGTM: regex imports are appropriate.
168-170: Good: use canonicalization before auto-phi.Passing finisher text through eoApplicationToXmir is correct.
378-381: Good: canonicalize the head before starting auto-phi.This will keep Q.* as Φ.* and avoid double-prefixing.
675-681: Good: vertical application base is canonicalized before auto-phi.Keeps precedence handling consistent across V-application.
718-719: Good: canonicalize vapplication head before auto-phi.
1245-1266: Fix eoApplicationToXmir: preserve standalone ρ/Φ/Φ̇, handle QQ., and short‑circuit ξ.*Current logic:
- Wraps bare ρ/Φ (and Φ̇) into ξ.ρ.ρ / ξ.ρ.Φ / ξ.ρ.Φ̇ — invalid.
- Ignores QQ. alias (HOME → Φ̇.).
- May double‑prefix when input already starts with ξ.
Apply this diff:
- private static String eoApplicationToXmir(final String application) { - final String transformed = application.replace("^.", "ρ."); - final String base; - if (transformed.startsWith("Q.")) { - base = transformed.replace("Q.", "Φ."); - } else { - base = transformed; - } - final String result; - if (base.startsWith("ρ.") || base.startsWith("Φ.")) { - result = base; - } else { - result = String.format("ξ.ρ.%s", base); - } - return result; - } + private static String eoApplicationToXmir(final String application) { + // Normalize ^. to ρ. only at the start + String base = application.replaceFirst("^\\^\\.", "ρ."); + // Map Q/QQ aliases to global/home Phi + base = XeEoListener.qqToGlobalPhi(base); + // If already canonical, keep as is + if ("ρ".equals(base) || "Φ".equals(base) || "Φ̇".equals(base) + || base.startsWith("ρ.") || base.startsWith("Φ.") || base.startsWith("Φ̇.") + || base.startsWith("ξ.") + ) { + return base; + } + // Otherwise qualify local names + return "ξ.ρ." + base; + }This aligns with prior guidance and the aliasing rules (Φ̇ is the QQ/“home” alias; see earlier learnings).
40-44: Regex misses underscores in identifiers; broaden character class.Identifiers like foo_bar won't match and will skip tail extraction, changing semantics (falls back to whole application). Allow underscores in both first and subsequent chars.
Apply this diff:
- private static final Pattern LAST_APPLICATION = Pattern.compile("\\)\\.(\\p{L}[\\p{L}\\p{N}-]*)$"); + // Matches trailing ").<name>" where <name> starts with a unicode letter or underscore, + // then letters/digits/underscore/hyphen, anchored at end. + private static final Pattern LAST_APPLICATION = + Pattern.compile("\\)\\.([\\p{L}_][\\p{L}\\p{N}_-]*)$");
|
@maxonfjvipon updated. Take a look, please |
| if (base.startsWith("ρ.") || base.startsWith("Φ.")) { | ||
| result = base; | ||
| } else { | ||
| result = String.format("ξ.ρ.%s", base); |
There was a problem hiding this comment.
@h1alexbel again, I strongly believe there's no need to append ξ.ρ. before base, imagine such code:
[] > foo
5 > x
[] > bar
[] > baz
x.some >> []
You'll translate it to
[] > foo
5 > x
[] > bar
[] > baz
[] > some-auto-name
$.^.x.some > @
But this is wrong, should be $.^.^.^.x.some, and it'll be as such, but later, on XSL transformations step
There was a problem hiding this comment.
@maxonfjvipon please check this. How about we add this test case as disabled with puzzle?
…ssues with nested abstracts
|
Waiting for #4557 |
3 participants
In this PR I've updated parsing of applications with precedences in auto Phi formations.
see #4559
Summary by CodeRabbit
New Features
Bug Fixes
Tests