Migrate the Scope generator off classic Xtend onto Xbase + JvmModelInferrer#1405
Migrate the Scope generator off classic Xtend onto Xbase + JvmModelInferrer#1405rubenporras wants to merge 1 commit into
Conversation
61fa966 to
6b0bcb2
Compare
6b0bcb2 to
633c4aa
Compare
joaodinissf
left a comment
There was a problem hiding this comment.
Thanks for tackling the classic-Xtend retirement here — the Xbase/JvmModelInferrer direction looks right and the structure is clean. I went deep on the hand-written translation layer (the src-gen is noise) and found one blocker plus a semantics change worth confirming. Flagging the key risk up front: dsl-devkit has no .scope/.export inputs, so the inferrers/compilers never run in CI — only compilation is checked. Everything below is therefore invisible to the green build and would only surface as wrong generated Java in ASMD.
- Blocker: arithmetic
-/*//emits non-compilable Java (-(a, 1)instead ofa - 1) — details inline, affects both scope and export. This is a regression vs the legacy engine, which handled it correctly and had a test for it (CodeGenerationXTest.testArithmetics, removed in this PR). - Confirm intended:
==/!=now compile toObjects.equals(...)(value equality) rather than Java==(identity).
Could you attach the before/after diff of regenerating the ASMD .scope/.export sources? That's the only artifact that actually exercises these paths, and it would let me confirm the fixes land. One concrete question: do any real ASMD sources use -/*// arithmetic, or ==/!= on non-null object/String operands?
🤖 Surfaced via an adversarial multi-agent review and manually verified against the diff. — Claude Code
| null | ||
| } | ||
| } | ||
| default: |
There was a problem hiding this comment.
Blocker (regression): this resolveType switch has no IntegerLiteral / RealLiteral case, so a numeric literal resolves to null. That makes isNumber (ScopeExpressionCompiler.xtend:343) return false for literals, so isArithmeticOperatorCall (:372) fails and - / * / / fall through to the final else (ScopeExpressionCompiler.xtend:276), emitting the operator as a method call: a - 1 → -(a, 1), count * 2 → *(count, 2) — non-compilable Java. It triggers unconditionally for -/*// (even 4 - 2), since arithmetic is never routed through translate() (see the doc-comment at :416).
This is a regression, not a faithful carryover: legacy isNumber (ExpressionExtensionsX.xtend:44, findType('Real').isAssignableFrom(analyze(it))) typed literals as numeric, so the arithmetic branch fired and produced a - 1. The removed CodeGenerationXTest.testArithmetics asserted exactly this.
Fix: add IntegerLiteral / RealLiteral cases to resolveType returning the numeric builtin (this also fixes the +-parenthesization comment below).
🤖 Adversarial review + manual verification. — Claude Code
| null | ||
| } | ||
| } | ||
| default: |
There was a problem hiding this comment.
Same blocker as in the scope translator: no IntegerLiteral / RealLiteral case here either, so ExportExpressionCompiler emits -(a, 1) for -/*//. Same fix applies. (Confirmed the scope and export engines are otherwise consistent.)
🤖 Adversarial review + manual verification. — Claude Code
| case '==': toBinaryOperation(xLeft, xRight, ObjectExtensions, 'operator_equals', sourceElement) | ||
| case '!=': toBinaryOperation(xLeft, xRight, ObjectExtensions, 'operator_notEquals', sourceElement) |
There was a problem hiding this comment.
Confirm this is intended: linking ==/!= to ObjectExtensions.operator_equals makes Xbase inline them to java.util.Objects.equals(a, b) (null-safe value equality), whereas the legacy generator emitted raw Java == (identity for objects, primitive compare for numbers). For non-null object/String operands this is a behavioral flip; numeric operands also pick up boxing (NaN/-0.0 edge cases). Compilable, so CI won't catch it — just want to make sure the semantics change is deliberate.
🤖 Adversarial review + manual verification. — Claude Code
| (if (target !== null) target.javaExpression(ctx) + '.' else '') + name + '(' + ', '.join(params.map[javaExpression(ctx)]) + ')' | ||
| } else if (isArithmeticOperatorCall(ctx)) { | ||
| autoBracket((' ' + name + ' ').join(params.map(e|e.javaExpression(ctx))), ctx) | ||
| } else if (isSimpleConcatCall()) { |
There was a problem hiding this comment.
Minor, and fixed by the same resolveType change as the arithmetic blocker: because numeric literals miss the arithmetic branch, + is handled here as concatenation without autoBracket. Top-level output matches legacy, but a + nested under a prefix operator loses its grouping, e.g. -(1 + 2) → -1 + 2. Adding the literal cases to resolveType restores the bracketed arithmetic branch.
🤖 Adversarial review + manual verification. — Claude Code
| def dispatch factoryArgument(Expression it) { | ||
| error('unsupported scope factory argument ' + it.toString()) |
There was a problem hiding this comment.
Intended narrowing? The legacy generator emitted full javaExpression(param) for factory arguments; this version supports only literals / type-refs / context vars and error(...)s on anything else. It fails loud at generation time rather than miscompiling, so low-risk — just flagging in case ASMD factory calls pass arbitrary expressions.
🤖 Adversarial review + manual verification. — Claude Code
| } | ||
| String firstSegment = segments.getFirst(); | ||
| if (segments.size() == 1) { | ||
| return findClassifier(firstSegment); |
There was a problem hiding this comment.
The new resolver is a bit more permissive than the legacy EmfRegistryMetaModel path: an unqualified single segment now resolves to a classifier (:69), and an unknown first segment falls back to a global last-segment search (:86), where legacy returned the raw name. Only diverges on names that were already unqualified/unknown, so low-risk — worth a glance in the ASMD regen diff. (Same applies to the export twin.)
🤖 Adversarial review + manual verification. — Claude Code
| // org.eclipse.osgi needed for NLS | ||
| // org.apache.logging.log4j needed for logging in generated StandaloneSetup | ||
| private static final List<String> REQUIRED_BUNDLES = newArrayList("org.eclipse.xtext.xbase.lib", "org.eclipse.xtend.lib", "org.eclipse.emf.ecore", "com.avaloq.tools.ddk.check.core", "com.avaloq.tools.ddk.check.runtime.core", "com.avaloq.tools.ddk.check.lib", "com.avaloq.tools.ddk.xtext", "org.eclipse.xtext", "org.eclipse.osgi", "org.eclipse.xtend", "org.eclipse.core.runtime", "org.eclipse.xtext.xbase", "org.apache.logging.log4j.api"); | ||
| private static final List<String> REQUIRED_BUNDLES = newArrayList("org.eclipse.xtext.xbase.lib", "org.eclipse.xtend.lib", "org.eclipse.emf.ecore", "com.avaloq.tools.ddk.check.core", "com.avaloq.tools.ddk.check.runtime.core", "com.avaloq.tools.ddk.check.lib", "com.avaloq.tools.ddk.xtext", "org.eclipse.xtext", "org.eclipse.osgi", "org.eclipse.xtend", "org.eclipse.core.runtime", "org.eclipse.xtext.xbase"); |
There was a problem hiding this comment.
Looks unrelated to the scope/export migration: this drops "org.apache.logging.log4j.api" from REQUIRED_BUNDLES, while the comment right above still says log4j is needed for the generated StandaloneSetup. Could you justify or revert — and if it's intended, update that comment so it's not self-contradicting?
🤖 Adversarial review + manual verification. — Claude Code
There was a problem hiding this comment.
I can revert part of this PR once this once is merged.
633c4aa to
343963b
Compare
The com.avaloq.tools.ddk.xtext.scope plugin compiled the embedded Expression DSL to Java through expression.generator.CodeGenerationX / CompilationContext, whose type system is the legacy classic Xtend runtime (org.eclipse.xtend.expression.*, org.eclipse.xtend.typesystem.emf.*). That was the only reason the plugin required those bundles. This PR replaces the IGenerator2-based ScopeGenerator with an Xbase JvmModelInferrer, introduces a self-contained (Xtend-free) expression-translation layer, deletes the classic-Xtend execution context, and removes org.eclipse.xtend and org.eclipse.xtend.typesystem.emf from the scope plugin. Key decisions Scope generation moves to a JvmModelInferrer + Xbase JvmModelGenerator Same pattern as Format/Check; lets the standard XbaseCompiler emit the provider classes. The *ScopeProvider/*ScopeNameProvider are now inferred JVM types rather than hand-templated .java. Hybrid expression backend: an Xbase translator + a string compiler fallback String + concatenation (very common in .scope) has no operator_plus in xbase.lib — Xbase special-cases it in its compiler — so it can't be pre-linked as an XExpression tree. We translate to Xbase what maps cleanly and fall back to a faithful port of the legacy string output for +, arithmetic and relational operators. Model-type names resolve via imported EPackages, not the classpath In real sources, (Cast)x / typeSelect(T) / T.isInstance(x) name EMF model types (FormDef, ILogicalTable, intfdef::AlternatingGroup), not Java FQNs. javaType resolves them through the model's imported EPackages → GenModelUtilX.instanceClassName, exactly as the legacy EmfRegistryMetaModel did. findDeclaredType would fail on these. .ext (JAVA extension) support dropped Scope sources no longer reference Xtend extension files; removed the .ext-reading validation and the isJavaExtensionCall/isExtension generator branches. factory becomes a direct Xbase static call Must be written Type.method(args); the generator resolves the type to a JvmDeclaredType.qualifiedName and emits the static call. Bare factory foo() is no longer supported. Downstream projects need to adapt their .scope/.ext sources.
343963b to
252d2f2
Compare
The com.avaloq.tools.ddk.xtext.scope plugin compiled the embedded
Expression DSL to Java through expression.generator.CodeGenerationX /
CompilationContext, whose type system is the legacy classic Xtend
runtime (org.eclipse.xtend.expression.,
org.eclipse.xtend.typesystem.emf.). That was the only reason the plugin
required those bundles.
This PR replaces the IGenerator2-based ScopeGenerator with an Xbase
JvmModelInferrer, introduces a self-contained (Xtend-free)
expression-translation layer, deletes the classic-Xtend execution
context, and removes org.eclipse.xtend and
org.eclipse.xtend.typesystem.emf from the scope plugin.
Key decisions
Scope generation moves to a JvmModelInferrer + Xbase
JvmModelGenerator Same pattern as Format/Check; lets the standard
XbaseCompiler emit the provider classes. The
*ScopeProvider/*ScopeNameProvider are now inferred JVM types rather than
hand-templated .java.
Hybrid expression backend: an Xbase translator + a string compiler
fallback String + concatenation (very common in .scope) has no
operator_plus in xbase.lib — Xbase special-cases it in its compiler — so
it can't be pre-linked as an XExpression tree. We translate to Xbase
what maps cleanly and fall back to a faithful port of the legacy string
output for +, arithmetic and relational operators.
Model-type names resolve via imported EPackages, not the classpath In
real sources, (Cast)x / typeSelect(T) / T.isInstance(x) name EMF model
types (FormDef, ILogicalTable, intfdef::AlternatingGroup), not Java
FQNs. javaType resolves them through the model's imported EPackages →
GenModelUtilX.instanceClassName, exactly as the legacy
EmfRegistryMetaModel did. findDeclaredType would fail on these.
.ext (JAVA extension) support dropped Scope sources no longer reference
Xtend extension files; removed the .ext-reading validation and the
isJavaExtensionCall/isExtension generator branches.
factory becomes a direct Xbase static call Must be written
Type.method(args); the generator resolves the type to a
JvmDeclaredType.qualifiedName and emits the static call. Bare factory
foo() is no longer supported.
Downstream projects need to adapt their .scope/.ext sources.