diff --git a/.agents/skills/xtend-to-java/rules/05-control-flow.md b/.agents/skills/xtend-to-java/rules/05-control-flow.md index 2789f207d..cbaeb8d5b 100644 --- a/.agents/skills/xtend-to-java/rules/05-control-flow.md +++ b/.agents/skills/xtend-to-java/rules/05-control-flow.md @@ -54,3 +54,39 @@ final String label = x != null ? x.getName() : ""; ``` For multi-line bodies, factor to a helper method or write `if`/`else` with an assignment in each branch. + +## 5.5 Exception handling — how Xtend lowers `try`/`catch` + +Xtend hides checked exceptions. Reading `xtend-gen/` shows the real lowering, and the faithful Java must +reproduce the **observed exception behaviour** — not a convenient re-wrap. + +- **`catch (SpecificException e)`** compiles to: + ```java + catch (Throwable _t) { + if (_t instanceof SpecificException) { /* the catch body */ } + else { throw Exceptions.sneakyThrow(_t); } + } + ``` + So the original catches its declared type and **sneaky-rethrows everything else unchanged**. A plain + Java `catch (SpecificException e)` is **behaviourally identical at runtime** — a non-matching throwable + propagates unchanged either way — so it is the preferred translation. The only real difference is + compile-time: if the `try` body throws a checked exception that the method does not declare, plain Java + won't compile, and only then is the literal lowering (or a `throws` clause) needed. Reproducing the + literal `catch (Throwable)` scaffold also trips Checkstyle's `IllegalCatch` + (`ddk-configuration/checkstyle/avaloq.xml`; PMD's `AvoidCatchingThrowable` is excluded in its favour), + so it needs a justified suppression — the fidelity-vs-lint call of §9.11 applies. What is NEVER + acceptable: *narrowing* `catch (Throwable)` to `catch (Exception)`, which changes behaviour + (drops `Error`). +- **A body that throws a checked exception with no `catch`** compiles to a whole-body + `try { ... } catch (Throwable _e) { throw Exceptions.sneakyThrow(_e); }` and the method declares **no** + checked exceptions — the **original throwable propagates UNWRAPPED**. +- **The faithful reproduction** is `org.eclipse.xtext.xbase.lib.Exceptions.sneakyThrow(e)` (the same utility + the compiler uses; it rethrows the original throwable), keeping the method's `throws` clause as Xtend left it. + This is a **sanctioned exception** to the "migrate off `xbase.lib`" pitfall (which targets API-surface types + like `Pair`): using `Exceptions` keeps the module's `org.eclipse.xtext.xbase.lib` Require-Bundle — the + infrastructure-cleanup step lists it as a dependency blocker, and that is accepted where sneaky-throw + fidelity requires it. +- **DO NOT wrap in a new exception type.** `throw new RuntimeException(e)` / `new IllegalStateException(e)` + changes the exception type callers observe — a real behavioural regression, not a style choice. (Legitimate + `throw new IllegalStateException("message")` on a genuinely-bad state — with no caught cause — is unrelated + and fine.) diff --git a/.agents/skills/xtend-to-java/rules/08-operator-overloads.md b/.agents/skills/xtend-to-java/rules/08-operator-overloads.md index ae1c3a71e..c759730b9 100644 --- a/.agents/skills/xtend-to-java/rules/08-operator-overloads.md +++ b/.agents/skills/xtend-to-java/rules/08-operator-overloads.md @@ -58,5 +58,9 @@ Same in Java. - `list += element` → `list.add(element)` - `list += otherList` → `list.addAll(otherList)` +- ⚠ **Exception**: when the receiver is an `EList` and `JvmTypesBuilder` is an in-scope extension + (every JVM model inferrer), `+=` binds to `JvmTypesBuilder.operator_add`, which **null-skips in BOTH + overloads** (and also no-ops on a null list). A plain `add`/`addAll` is then NOT faithful — see + [`rules/10-jvm-model-inferrer.md`](./10-jvm-model-inferrer.md) §10.4 before translating any `+=` in an inferrer. - `list -= element` → `list.remove(element)` - `map[key]` (Xtend bracket access) → `map.get(key)` diff --git a/.agents/skills/xtend-to-java/rules/09-misc-syntax.md b/.agents/skills/xtend-to-java/rules/09-misc-syntax.md index 5ec959665..106b1d5c4 100644 --- a/.agents/skills/xtend-to-java/rules/09-misc-syntax.md +++ b/.agents/skills/xtend-to-java/rules/09-misc-syntax.md @@ -170,3 +170,22 @@ Rules: - **Copy Javadoc from the Xtend source verbatim.** Never generate, guess, or infer Javadoc that was not in the original. Invented comments are misleading. - **`@throws` tags**: Only add when (1) the method already has Javadoc AND (2) the migrated signature declares a `throws` clause. Do not add Javadoc just to host a `@throws` tag. - Do **not** add `@SuppressWarnings("all")` — the Xtend compiler injects this into `xtend-gen/`; human-converted Java shouldn't have it. + +## 9.11 Charset — a sanctioned deviation from `xtend-gen` + +When the Xtend source constructs a reader/writer with **no charset** (`new InputStreamReader(stream)`, +`new String(bytes)`, `.getBytes()`), `xtend-gen` faithfully reproduces the **platform-default** charset. +**Do not reproduce that.** PMD `RelianceOnDefaultCharset` flags the *implicit* default — an explicit +`Charset.defaultCharset()` would pass the gate, but it keeps the platform dependence, which is the actual +latent bug. Pick the charset from the **data contract** instead: + +- if the data carries its own encoding, honour it — e.g. content read from an Eclipse `IFile` should use + `file.getCharset()` (the repo already does this in hand-written code); +- otherwise use `java.nio.charset.StandardCharsets.UTF_8`, the project-wide source encoding + (`ddk-parent/pom.xml`): `new InputStreamReader(stream, StandardCharsets.UTF_8)`. + +This is one of the few places a migration **should** diverge from `xtend-gen`. General principle: **where a +valid lint rule and a literal `xtend-gen` behaviour conflict on a latent-bug pattern (default charset, an +un-guarded resource, etc.), the migration fixes the bug rather than suppressing the rule.** (Two legacy +`// NOPMD` suppressions of this rule exist in hand-written code — `CheckPreferencesHelper`, +`XtextGMFResourceUtil`; they are grandfathered, not a precedent for migrations.) diff --git a/.agents/skills/xtend-to-java/rules/10-jvm-model-inferrer.md b/.agents/skills/xtend-to-java/rules/10-jvm-model-inferrer.md index bed5baeb4..ffe4c1e2a 100644 --- a/.agents/skills/xtend-to-java/rules/10-jvm-model-inferrer.md +++ b/.agents/skills/xtend-to-java/rules/10-jvm-model-inferrer.md @@ -13,7 +13,7 @@ is the canonical worked example — read it in full before migrating another inf | `def dispatch infer(X x, IJvmDeclaredTypeAcceptor acceptor, boolean preIndexingPhase)` | `_infer(final X x, final IJvmDeclaredTypeAcceptor acceptor, final boolean isPreIndexingPhase)` + the dispatcher pattern ([`rules/09-misc-syntax.md`](./09-misc-syntax.md) §9.7) | | `x.toClass(name)` | `jvmTypesBuilder.toClass(x, name)` | | `acceptor.accept(cls, [ ... ])` | `acceptor.accept(cls, initializer)` where `initializer` is a `Procedure1` (see `FormatJvmModelInferrer.java:182-192`) | -| `members += x` / `superTypes += x` / `annotations += x` | `it.getMembers().add(x)` / `it.getSuperTypes().add(x)` / `it.getAnnotations().add(x)` | +| `members += x` / `superTypes += x` / `annotations += x` | `it.getMembers().add(x)` / … — **only when `x` is provably non-null**; `JvmTypesBuilder.operator_add` null-skips in BOTH its overloads (single element and collection), so see §10.4 before translating any `+=` | | `x.toMethod(name, type) [ ... ]` | `jvmTypesBuilder.toMethod(x, name, type, initializer)` with a `Procedure1` (`:223-235`) | | `x.toField(name, type) [ ... ]` / `x.toParameter(name, type)` | `jvmTypesBuilder.toField(x, name, type, initializer)` / `jvmTypesBuilder.toParameter(x, name, type)` (`:245`) | | `typeRef(T)` / `typeRef(name)` | `_typeReferenceBuilder.typeRef(...)` — the protected field inherited from `AbstractModelInferrer` (`:202-204`); for lookups needing a context object use `typeReferences.getTypeForName(name, context)` (`:235`) | @@ -49,9 +49,65 @@ Xtend assigns bodies two ways; both become `jvmTypesBuilder.setBody(method, ...) (same file, `:113`). - `members += list.map(...).flatten.filterNull` chains: see [`references/xtend-library-replacements.md`](../references/xtend-library-replacements.md) - for `flatten`/`filterNull` stream equivalents; the result feeds `getMembers().addAll(...)`. + for `flatten`/`filterNull` stream equivalents; the result feeds the add — but read §10.4 first + for the null-skip requirement, which is the most dangerous inferrer-migration trap. -## 10.4 Verification +## 10.4 ⚠ `operator_add` (`+=`) SKIPS nulls — plain `add`/`addAll` does NOT + +**This is the highest-risk inferrer defect: it passes every static gate (PMD/Checkstyle/SpotBugs) and +every test that does not happen to feed a null — and then fails at runtime the moment one does.** +The failure is fast and loud, not silent: JVM model containment lists (`getMembers()` etc.) are EMF +`EObjectEList`s with `canContainNull() == false`, so a bare `.add(null)`/`.addAll(...)` throws +`IllegalArgumentException("The 'no null' constraint is violated")` **at the add call**. Xtend's `+=` +never produces that null add in the first place — that is the behaviour a faithful migration must keep. + +`JvmTypesBuilder` overrides **both** `operator_add` overloads, and **both null-skip** (they also no-op +on a null list): `operator_add(EList, T)` is `if (list != null && element != null) list.add(element)`, +and the `Iterable` overload delegates to it per element. So the trap covers the single-element form too: +Xtend `members += toField(...)` silently skips a null factory result, while the doc-obvious +`it.getMembers().add(toField(...))` throws on it. + +And the factories DO return null — verified triggers in `JvmTypesBuilder`: +`toField` / `toMethod` / `toParameter` / `toEnumerationLiteral` return null when the **source element or +the name** is null; `toConstructor` guards only the source element (it has no name argument); +`toGetter` / `toSetter` guard the property/field name. A null **type** argument does *not* trigger a null +return. Any helper with a `return null` fall-through (a `switch`/`if` that doesn't match) is a trigger too. + +So the faithful Java of any `+=` whose right-hand side can be null is a guarded add: + +```java +// WRONG — throws IllegalArgumentException("The 'no null' constraint is violated") at the add +// the first time createConstant returns null (value-less constant): +for (final Constant c : constants) { + it.getMembers().add(createConstant(format, c)); +} + +// RIGHT — reproduce operator_add's null-skip (either form): +for (final Constant c : constants) { + final JvmMember member = createConstant(format, c); + if (member != null) { + it.getMembers().add(member); + } +} +// or, matching the Xtend chain shape with the JDK stream equivalents +// (per references/xtend-library-replacements.md — no xbase.lib in migrated Java): +it.getMembers().addAll(constants.stream().map(c -> createConstant(format, c)).filter(Objects::nonNull).toList()); +``` + +**Checklist for every `+=` site in a migrated inferrer — single element or collection:** can the producer +return null (nullable source element or name, or a `return null` branch)? If yes, there MUST be a null +guard / `Objects::nonNull` filter. A bare `add`/`addAll` over a null-capable producer is a faithfulness +regression. + +> Real shipped example: `FormatJvmModelInferrer.inferConstants` translated the Xtend +> `members += allConstants.map[createConstant]` (null-skipping `operator_add`) into a bare +> `for { it.getMembers().add(createConstant(format, c)); }`. `createConstant` returns null for a +> value-less constant, so the migrated code throws where the original silently skipped — undetected by +> all gates and tests because none declared a value-less constant. The single-element `+=` carries the +> **same** trap: `operator_add(EList, T)` null-skips too, so no `+=` may be translated to a bare `.add` +> unless the value is provably non-null. + +## 10.5 Verification An inferrer is a generator: its OUTPUT (the inferred JVM model, and through it the generated Java) is the ground truth. Byte-verify emitted body/documentation strings against `xtend-gen/` diff --git a/.agents/skills/xtend-to-java/workflow/known-pitfalls.md b/.agents/skills/xtend-to-java/workflow/known-pitfalls.md index 16fa0cde3..76ed6e7b6 100644 --- a/.agents/skills/xtend-to-java/workflow/known-pitfalls.md +++ b/.agents/skills/xtend-to-java/workflow/known-pitfalls.md @@ -41,4 +41,6 @@ Consolidated table of common mistakes and their fixes. Review before and after e | **Empty method body needs a comment** | PMD `UncommentedEmptyMethodBody` fires on a bare `{}`. Keep a comment (e.g. the original `// TODO …`) in genuinely-empty bodies. | | **Text block ≠ inline-`'''` exactly** | Java text blocks strip trailing whitespace on each content line and add a trailing newline before the closing `"""`; an inline-`'''` Xtend template preserves trailing spaces and omits the trailing newline. For string OUTPUT, match `xtend-gen` exactly (`\s` / `\` escapes). When the delta is provably behaviour-inert (e.g. a parser "no syntax errors" assertion) a clean text block is fine — say so in the commit/PR. | | **`final`-on-locals consistency** | Not an enforced gate, but keep locals consistently `final` within a file; mixed `final`/non-`final` siblings is a readability nit only. | -| **Don't carry `xbase.lib` types into migrated Java** | The `->` pair operator compiles to `org.eclipse.xtext.xbase.lib.Pair` — an Xtend runtime type. Don't keep it in the `.java`: replace with a small `private record` (named fields, accepts `null`) or `java.util.Map.entry` — but `Map.entry` **rejects null** keys/values, so use a record when nulls are possible. Bonus: a non-generic record vararg drops the `@SafeVarargs` a `Pair<…>` vararg required. Migrating off Xtend means migrating off `xbase.lib` too. | +| **Don't carry `xbase.lib` types into migrated Java** | The `->` pair operator compiles to `org.eclipse.xtext.xbase.lib.Pair` — an Xtend runtime type. Don't keep it in the `.java`: replace with a small `private record` (named fields, accepts `null`) or `java.util.Map.entry` — but `Map.entry` **rejects null** keys/values, so use a record when nulls are possible. Bonus: a non-generic record vararg drops the `@SafeVarargs` a `Pair<…>` vararg required. Migrating off Xtend means migrating off `xbase.lib` too — for API-surface types; the sanctioned exception is `Exceptions.sneakyThrow` where exception fidelity requires it (see [`rules/05-control-flow.md`](../rules/05-control-flow.md) §5.5). | +| **`operator_add` (`+=`) skips nulls — both overloads** | The single most dangerous inferrer trap — see [`rules/10-jvm-model-inferrer.md`](../rules/10-jvm-model-inferrer.md) §10.4. Any `+=` on an `EList` with `JvmTypesBuilder` in scope binds to `operator_add`, which drops null elements (single-element AND collection overloads); the factories return null on a null source element or name. A bare `add`/`addAll` instead throws `IllegalArgumentException("The 'no null' constraint is violated")` at the add site the first time a null flows — loud in production, invisible to gates and to every test that never feeds a null. Shipped once (`FormatJvmModelInferrer.inferConstants`). Always null-guard / `Objects::nonNull`-filter a null-capable add. | +| **Behavioural equivalence ≠ literal-token equivalence** | When verifying a migration (or reconciling two migrations) against `xtend-gen`, do NOT decide "faithful" by whether a token (`filterNull`, a `catch`, a charset arg) textually appears. `xtend-gen` semantics can live in a call whose Java equivalent needs *extra* code (e.g. `operator_add`'s null-skip → an explicit null filter; §10.4). **Prove every behavioural divergence against fresh `xtend-gen` and cover it with a test — gates and existing tests only catch what they already exercise** (the shipped null-leak passed them all because no test fed a null). The `filterNull`-looks-spurious trap cost a real regression when trusted without such proof. |