Skip to content

fix(golang): emit valid struct type for map fields - #315

Open
chaitijain593-prog wants to merge 3 commits into
accordproject:mainfrom
chaitijain593-prog:fix/golang-map-struct-field
Open

chaitijain593-prog wants to merge 3 commits into
accordproject:mainfrom
chaitijain593-prog:fix/golang-map-struct-field

Conversation

@chaitijain593-prog

Copy link
Copy Markdown

Closes #314

Fixes GoLangVisitor struct field generation for map fields by emitting exported struct types (Field map[Key]Value) instead of procedural initialization statements (field := make(...)), and adds optional chaining when resolving scalar map key/value types.

Changes

  • Updated GoLangVisitor.prototype.visitField to capitalize map property names and emit valid Go struct map field declarations (FieldName map[Key]Value \json:"name"``).
  • Added optional chaining (?.isScalarDeclaration?.()) to getType lookups to handle unresolved or cross-namespace types safely.
  • Updated unit test in test/codegen/fromcto/golang/golangvisitor.js to assert the valid struct field declaration.
  • Added a unit test to verify null-safety when getType returns null.
  • Updated test snapshots in test/codegen/__snapshots__/codegen.js.snap.

Flags

  • None. Non-breaking bug fix for generated Go output.

Screenshots or Video

  • Not applicable (CLI code-generation fix).

Related Issues

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
  • Merging to main from fork:branchname

Signed-off-by: chaitijain593 <chaitijain593@gmail.com>
@mira-accord-project

Copy link
Copy Markdown

Thank you for your first contribution to concerto-codegen, and for fixing the Go output for map fields. Your change comes with a visitor test and an updated snapshot, which makes it much easier to review.

What happens next: I have asked the codegen maintainers to review this PR. If a review comment or a CI result needs your attention, please reply here and we will help where we can. Your commits need a DCO sign-off (git commit -s), and npm test should pass locally with the snapshot updated.

Drafted by an Accord Project agent, reviewed by a maintainer.

@mira-accord-project

Copy link
Copy Markdown

Thank you for this contribution, and welcome to the Accord Project. Making the Go generator emit a valid struct type for map fields is a useful fix, and it's good to see you've included updated snapshots and unit tests in test/codegen/fromcto/golang/golangvisitor.js.

What happens next: the PR is in the concerto-codegen review queue and a maintainer will pick it up in due course. I can't give you a firm date. If you'd like to help it along, please keep the branch up to date with main. If a maintainer asks for changes, reply here and push to the same branch.

If you haven't already, please check that your commits carry a DCO sign-off (git commit -s), as the repository requires it.

Drafted by an Accord Project agent, reviewed by a maintainer.

@mira-accord-project mira-accord-project Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for this fix, and welcome to the project. Turning x := make(map[K]V) into a plain Name map[K]V field is the right change: the old output is not valid inside a Go struct, and the new field name and JSON tag follow the same rules as the non-map path. The ?. guards and the updated snapshots match the change. One edge case remains: a model whose only DateTime use is inside a map still produces Go that will not compile. Handling that in containsDateTimeField, with a small test, would make the fix complete. The CI workflows beyond DCO are waiting for a maintainer to approve them, as is usual for a first contribution, and a maintainer review is still needed before this can merge.

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


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

Signed-off-by: chaitijain593 <chaitijain593@gmail.com>
@chaitijain593-prog

Copy link
Copy Markdown
Author

Thank you for the review and for catching the DateTime edge case!

I have addressed both findings in the latest commit:

  1. containsDateTimeField resolution: Updated the helper to inspect map fields and resolve both the key and value types (following scalar declarations to their underlying primitive). If either resolves to DateTime, it properly triggers the import "time" emission.
  2. Field name normalization: Moved the $-stripping and capitalization logic to the top of visitField so it is computed once across both paths.
  3. Unit tests: Added unit tests covering:
    • A model whose map value directly uses DateTime.
    • A model whose map value uses a scalar extending DateTime.
    • Verification that import "time" is written to the generated file when the only DateTime is inside a map.

All local unit tests (34 passing) and npm run lint are passing cleanly.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

An inherited DateTime-valued map can trigger an unused time import in the child’s generated Go file, causing compilation to fail.

1 open issue
What changed in this PR

Fixes Concerto-to-Go map field generation for issue #314 by emitting exported, typed struct fields instead of initialization statements.

Changes:

  • Emits valid map field declarations while preserving JSON property names.
  • Adds null-safe scalar lookups and detects DateTime map types for the time import.
  • Updates unit tests and generated-output snapshots.
File Description
test/​codegen/​fromcto/​golang/​golangvisitor.js Tests map declarations, null lookups, and DateTime import detection.
test/​codegen/​__snapshots__/​codegen.js.snap Updates expected Go map fields to exported declarations.
lib/​codegen/​fromcto/​golang/​golangvisitor.js Corrects map field output and extends type resolution and import detection.

Review effort: Balanced

Signed-off-by: chaitijain593 <chaitijain593@gmail.com>
@chaitijain593-prog

Copy link
Copy Markdown
Author

Addressed! Added the field.getParent() === classDecl check to prevent unused time imports on child classes inheriting maps, and added an inheritance regression test.

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(golang): generated output produces invalid struct syntax for map fields

2 participants