fix(rust): resolve scalar maps and recursion by Ani07-05 · Pull Request #282 · accordproject/concerto-codegen

Ani07-05 · 2026-10-02T07:09:41Z

Closes #264

Changes

Unwrap scalar map keys/values to their underlying primitive in pub type map aliases (HashMap<SSN, Employee> becomes HashMap<String, Employee>), matching how field sites already behave. Scalars are not emitted, so the alias previously referenced undefined types (bugs 1 and 2)
Box a field as Box<T> (Option<Box<Manager>>) only when it is recursive, to break the infinite-size type (bug 3). A field is recursive when its target reaches the struct that holds it through non-array properties (direct or indirect cycle). Inherited properties are written into every subclass, so each subclass copy is checked against its own struct, not the declaring class. With flattenSubclassesToUnion, a union-typed field also reaches every variant of the <Base>Union enum
The cycle check is an iterative worklist (deep acyclic models cannot overflow the call stack), and an abstract base is not counted as a variant of its own union, matching the enum generation
Relationships are emitted as plain named types, never unions, so cycle detection no longer expands them into union variants (avoids needless Box)
With flattenSubclassesToUnion, only reference <Base>Union when the enum is actually generated, i.e. when a subclass is in the same model file. Previously a base whose subclasses were all in other namespaces emitted BaseUnion without defining it (E0425). One unionSubclasses() helper now drives the enum, the field type and the cycle check
Remove the Rust skip entries for hr_base and hr_integration in test/verification/cases.js so they are verified with cargo check
Add unit tests for scalar map unwrapping, direct/indirect cycles, subclass and union recursion, and the cases that stay unboxed; update codegen snapshots

Flags

Boxing is conditional, so output only changes for recursive fields (see snapshot diff). Non-recursive relationships are emitted as before
Vec<T> fields and map values are left unboxed since they already provide indirection
Union variants are limited to direct subclasses declared in the same model file, matching how the union enum is generated

Testing

npx mocha test/codegen/fromcto/rust/*.js: 68 passing
npx mocha test/verification/rust.compile.test.js (cargo check): 32 passing. hr_base and hr_integration pass with the skips removed; 8 inline cases with flattenSubclassesToUnion cover union cycles, relationship cycles, an inherited field that loops only in the subclass, a map whose value holds the owner, and an abstract base that refers to its own union, and a cross-namespace union base
Besides compiling, each generated crate gets a no-unneeded-Box check: it reads the by-value type graph from the emitted Rust and requires every Box field to point at a type that leads back to its owner. It runs on the 8 corpus cases and the 9 inline shapes, and fails if the abstract-base over-boxing is reintroduced
npm test: 741 passing, 0 failing on the branch rebased onto current main; lint, licence-header check and JSDoc are clean

Related Issues

Issue bug: Rust verification - generated output fails cargo check #264

Author Checklist

Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
Vital features and changes captured in unit and/or integration tests
Commits messages follow AP format
Extend the documentation, if necessary

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

apoorv7g · 2026-10-03T06:02:28Z

@Ani07-05 Has this been well tested ?

Ani07-05 · 2026-10-03T06:31:18Z

Yes, tested. I ran:

npm test: 686 passing, 0 failing
npm run lint: eslint, lockfile-lint, syncpack all pass
cargo check on all 8 Rust verification cases, including hr_base and hr_integration
Added a unit test for scalar map unwrapping, updated relationship tests and snapshots

Jason-123-cyber

ok

Jason-123-cyber

jk

mira-accord-project

Thank you for this contribution, Ani07-05, and for the clear PR description. Unwrapping scalar map keys and values in visitMapDeclaration is a clean fix for the undefined SSN and Time types, and removing the two Rust skips in test/verification/cases.js is the right follow-through. Boxing non-array relationships does break the Manager cycle in the HR model. Nothing here is blocking, but a few things would make this ready to merge: (1) rebase onto main and regenerate the snapshot, (2) either box recursive containment fields as well or narrow the title and description to relationship recursion, (3) list the unconditional boxing as a numbered question for the maintainers and consider limiting it to relationships that are actually recursive, and (4) add a unit test for a scalar map value.

Nothing blocking; some suggestions below. Findings: 3 should-fix, 1 nit.

should-fix · hygiene: GitHub reports this branch as conflicting with main. Please rebase, run npm run test:updateSnapshots, and read the changes in test/codegen/__snapshots__/codegen.js.snap to check that each changed line still follows from the fix. The commit also needs to be able to pass npm test on the rebased branch.

_{Automated review by Mira, an Accord Project agent, at f7c8295. Findings are labelled blocking, should-fix or nit; a maintainer makes the final call.}

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

mira-accord-project

Thank you for the updates, Ani07-05. The new head addresses the earlier feedback well. Boxing now happens only where a property really reaches its declaring class, which keeps the generated output unchanged for most models. Containment fields are covered as well as relationships. The snapshot change is limited to Employee.manager and Manager.manager. The scalar key and value map handling has unit tests, and the two stale verification skips are removed. One edge case remains, under the opt-in flattenSubclassesToUnion parameter. Once that is handled, with a unit test, this looks ready for a maintainer to approve. The earlier review threads look resolved by these changes, so it would help to mark them resolved. The CI workflows for a first contribution need a maintainer to approve them, so only the DCO check has run so far.

Nothing blocking; some suggestions below. Findings: 1 should-fix.

should-fix · correctness · lib/codegen/fromcto/rust/rustvisitor.js: isRecursive only boxes when the target is the declaring class, reaches it through properties, or is a subclass of it. With flattenSubclassesToUnion on, a field typed with a superclass is emitted as <Base>Union, and that enum has a variant for every subclass. Take abstract concept Base {} and concept A extends Base { o Base other }. This generates A { other: BaseUnion } and BaseUnion::A(A), which is an infinitely sized type that cargo check rejects with E0072. isRecursive returns false here because Base never reaches A. Suggested fix: pass useUnion into isRecursive. When it is true, also treat the property as recursive if the owner is, or inherits from, the property's target type (compare owner.getAllSuperTypeDeclarations() with the target) and the target has subclasses in the same model file. Please add a unit test for this case alongside the other recursion tests.

_{Automated review by Mira, an Accord Project agent, at ae6fb34. Findings are labelled blocking, should-fix or nit; a maintainer makes the final call.}

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

mira-accord-project

Thank you for sticking with this, Ani07-05. The latest commit is a clear improvement. Boxing now happens only when a property reaches its declaring class, which covers direct, indirect, subclass and flattenSubclassesToUnion cases and leaves other output unchanged. Earlier comments about containment recursion, unconditional boxing and missing scalar-value tests are addressed, and the new unit tests for the cycle and union cases are well chosen. I found no new problems in the code. To make this mergeable: (1) the test and verification workflows haven't run on this commit, so cargo check for the Rust cases whose skips you removed is not yet confirmed. A maintainer needs to approve the workflow runs, and they need to pass; (2) please resolve the three earlier review threads that are now addressed; (3) a maintainer needs to approve. Please also include the commands you ran locally and the pass counts in the description, as the repo's guidance asks.

No problems found.

_{Automated review by Mira, an Accord Project agent, at 6938e9c. Findings are labelled blocking, should-fix or nit; a maintainer makes the final call.}

Ani07-05 · 2026-11-07T06:18:31Z

@mttrbrts could you approve the CI workflow runs ?

Copilot

🟡 Changes recommended

Nested fields bypass emitted unions during cycle detection, leaving some multi-union cycles unboxed and causing Rust compilation to fail with infinite-size types.

1 open issue

Flattened subclass unions can leave recursive types unboxed · New

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

Nested union variants are not expanded during recursion checks, leaving mutual by-value cycles unboxed and causing generated Rust to fail compilation with infinite-size type errors.

1 open issue

Flattened subclass unions can leave recursive types unboxed

_{Review effort: Balanced}

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

Unwrap scalar map key/value types in type aliases to their primitives and box non-array properties that reach their declaring class, so generated output passes cargo check. Remove the Rust verification skips for hr_base and hr_integration. Closes accordproject#264 Signed-off-by: Anirudh <anirudh.ashrith2005@gmail.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

Copilot

🟡 Changes recommended

The traversal can exhaust the JavaScript call stack on deeply nested acyclic models and unnecessarily box non-recursive fields by including abstract base types as union variants.

2 open issues

1 resolved since last review

Exclude relationships from subclass-union expansion

_{Review effort: Balanced}

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

With flattenSubclassesToUnion, a field typed with a base class is emitted as <Base>Union, which has a variant for each local subclass. A subclass declaring such a field (A { o Base other }) produced an infinitely sized type that cargo check rejects with E0072. isRecursive now also treats the field as recursive when any union variant reaches the declaring class. Signed-off-by: Anirudh <anirudh.ashrith2005@gmail.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

Copilot

🟡 Changes recommended

Flattened relationships can be boxed unnecessarily, verification can misidentify cycles across namespaces, and repeated reachability checks cause quadratic generation time.

3 open issues

2 resolved since last review

_{Review effort: Balanced}

+        const seen = new Set();
+        const pending = typesReachedBy(property);


        let type = relationshipDeclaration.type;
        if (relationshipDeclaration.isArray?.()) {
            type = `Vec<${type}>`;
+        } else if (this.isRecursive(relationshipDeclaration, false, parameters.emittingClass)) {


+            .readdirSync(src)
+            .filter((file) => file.endsWith('.rs'))
+            .map((file) => fs.readFileSync(path.join(src, file), 'utf-8'))
+            .join('\n');


Ani07-05 · 2026-11-13T17:28:21Z

@jamieshorten thank you for your constant patience and merging the PR , appreciate it :D

Copilot AI balanced review requested due to automatic review settings October 2, 2026 07:09

Copilot AI reviewed Oct 2, 2026

Jason-123-cyber approved these changes Oct 7, 2026

View reviewed changes

Jason-123-cyber approved these changes Oct 9, 2026

View reviewed changes

radhikaa06 mentioned this pull request Oct 12, 2026

fix(avro): drop the version from Avro namespaces #290

Merged

5 tasks done

mttrbrts-agent mentioned this pull request Nov 1, 2026

docs: add AGENTS.md with guidance for AI coding agents #311

Merged

3 tasks done

mira-accord-project Bot reviewed Nov 3, 2026

View reviewed changes

Comment thread lib/codegen/fromcto/rust/rustvisitor.js Resolved

Comment thread lib/codegen/fromcto/rust/rustvisitor.js Resolved

Comment thread test/codegen/fromcto/rust/rustvisitor.js Resolved

mira-accord-project Bot added ai/decision:needs-review ai/risk:medium labels Nov 3, 2026

Ani07-05 force-pushed the anirudh/i264/rust-verification branch from f7c8295 to ae6fb34 Compare November 4, 2026 07:03

Copilot AI balanced review requested due to automatic review settings November 4, 2026 07:03

Copilot AI reviewed Nov 4, 2026

mira-accord-project Bot added the ai/kind:bugfix label Nov 4, 2026

mira-accord-project Bot reviewed Nov 5, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings November 6, 2026 13:23

Copilot AI reviewed Nov 6, 2026

mira-accord-project Bot reviewed Nov 6, 2026

View reviewed changes

jamieshorten requested a balanced review from Copilot November 11, 2026 13:03

Copilot started reviewing on behalf of jamieshorten November 11, 2026 13:03

View session

Copilot AI reviewed Nov 11, 2026

View reviewed changes

Comment thread lib/codegen/fromcto/rust/rustvisitor.js Resolved

jamieshorten requested a balanced review from Copilot November 11, 2026 13:28

Copilot started reviewing on behalf of jamieshorten November 11, 2026 13:29

View session

Copilot AI reviewed Nov 11, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings November 11, 2026 15:45

Copilot AI reviewed Nov 11, 2026

jamieshorten requested a balanced review from Copilot November 12, 2026 16:43

Copilot started reviewing on behalf of jamieshorten November 12, 2026 16:43

View session

Copilot AI reviewed Nov 12, 2026

Ani07-05 force-pushed the anirudh/i264/rust-verification branch from 7ecfec7 to 91671c4 Compare November 12, 2026 17:32

Copilot AI balanced review requested due to automatic review settings November 12, 2026 17:32

Copilot AI reviewed Nov 12, 2026

Ani07-05 force-pushed the anirudh/i264/rust-verification branch from 91671c4 to 82ec254 Compare November 12, 2026 17:34

Copilot AI balanced review requested due to automatic review settings November 12, 2026 17:34

Copilot AI reviewed Nov 12, 2026

jamieshorten requested a balanced review from Copilot November 12, 2026 17:57

Copilot started reviewing on behalf of jamieshorten November 12, 2026 17:59

View session

Copilot AI reviewed Nov 12, 2026

View reviewed changes

Comment thread lib/codegen/fromcto/rust/rustvisitor.js Resolved

Comment thread lib/codegen/fromcto/rust/rustvisitor.js Resolved

Ani07-05 force-pushed the anirudh/i264/rust-verification branch from 82ec254 to e8f8954 Compare November 12, 2026 18:08

Copilot AI balanced review requested due to automatic review settings November 12, 2026 18:08

Copilot AI reviewed Nov 12, 2026

Ani07-05 force-pushed the anirudh/i264/rust-verification branch from e8f8954 to 118c66b Compare November 12, 2026 18:17

Copilot AI balanced review requested due to automatic review settings November 12, 2026 18:17

Copilot AI reviewed Nov 12, 2026

jamieshorten requested a balanced review from Copilot November 13, 2026 16:52

Copilot started reviewing on behalf of jamieshorten November 13, 2026 16:53

View session

Copilot AI reviewed Nov 13, 2026

View reviewed changes

jamieshorten closed this Nov 13, 2026

jamieshorten reopened this Nov 13, 2026

jamieshorten enabled auto-merge (squash) November 13, 2026 17:23

jamieshorten approved these changes Nov 13, 2026

View reviewed changes

jamieshorten disabled auto-merge November 13, 2026 17:24

jamieshorten merged commit 42a0971 into accordproject:main Nov 13, 2026
50 of 51 checks passed

Ani07-05 deleted the anirudh/i264/rust-verification branch November 13, 2026 17:26

Ani07-05 restored the anirudh/i264/rust-verification branch November 13, 2026 17:26

		const seen = new Set();
		const pending = typesReachedBy(property);

Conversation

Ani07-05 commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Flags

Testing

Related Issues

Author Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

apoorv7g commented Oct 3, 2026

Uh oh!

Ani07-05 commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jason-123-cyber left a comment

Choose a reason for hiding this comment

Uh oh!

Jason-123-cyber left a comment

Choose a reason for hiding this comment

Uh oh!

mira-accord-project Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

mira-accord-project Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

mira-accord-project Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Ani07-05 commented Nov 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Ani07-05 commented Oct 2, 2026 •

edited

Loading

Ani07-05 commented Oct 3, 2026 •

edited

Loading