Skip to content

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

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

chaitijain593-prog wants to merge 4 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.

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.

🟢 Approval recommended

No active review issues are reported for the Go map-field generation changes.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@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 for working through the earlier feedback so thoroughly. Map fields now come out as valid exported Go struct fields (Name map[K]V with the original name kept in the json tag). The scalar key/value resolution is null-safe, import "time" is now written when the only DateTime is inside a map, and the inheritance case is handled with an own-property check and a regression test. I read the logic at this head and found no correctness problems, and the tests and snapshot changes follow from the fix. The two points below are small, optional polish. Nothing here blocks the PR; it now needs a maintainer's review.

Nothing blocking; some suggestions below. Findings: 2 nit.


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

if(field.getType() === 'DateTime') {
return true;
}
if(typeof field.getParent === 'function' && field.getParent() === classDecl && ModelUtil.isMap(field)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit · conventions

The map key/value resolution (get the map declaration, follow scalar declarations to the underlying type) is now written out twice: here and in visitField. A small private helper that returns the resolved key and value types would keep the two copies from drifting apart. Optional.

… $-prefixed map field

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

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.

🟢 Approval recommended

The review identifies no active issues blocking the Go map-field generation fix.

No open issues 🎉

Review effort: Balanced

@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, chaitijain593-prog, for a careful fix. Map fields now generate a valid map[K]V in Go, with scalar key and value types unwrapped, a leading $ stripped from the field name (the original name kept in the json tag), and import "time" added when a map key or value is a DateTime. I read the code at this head and found nothing that needs changing. The earlier suggestions are all in: resolveMapTypes is now shared by visitField and containsDateTimeField, there is a $dictionary test, and the check is limited to maps owned by the class being visited, so a child that only inherits a map from another namespace no longer gets an unused time import (the new test covers it). The snapshot changes follow from the fix. The remaining open thread is the earlier helper suggestion, which is now done in the code, so it can be resolved. After that this is ready for a maintainer to approve.

No problems found.


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

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

3 participants