Skip to content

Fix unmanaged→managed event marshalling order and return conversion in source generator#525

Merged
ikkentim merged 7 commits into
mainfrom
copilot/fix-unmarshal-order-issue
May 10, 2026
Merged

Fix unmanaged→managed event marshalling order and return conversion in source generator#525
ikkentim merged 7 commits into
mainfrom
copilot/fix-unmarshal-order-issue

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 7, 2026

  • Inspect new review comment about out parameter behavior in unmanaged→managed generation
  • Reproduce/verify generated behavior for an out bool event parameter scenario
  • Apply minimal generator fix if out-parameter marshalling is broken
  • Run targeted build/tests for the source generator changes
  • Respond to the new review comment with result and commit hash

Copilot AI changed the title [WIP] Fix unmarshal order in native-to-managed marshalling generator Fix unmanaged→managed event marshalling order and return conversion in source generator May 7, 2026
Copilot AI requested a review from ikkentim May 7, 2026 08:57
}

private static ExpressionSyntax AddReturnValueAssignmentToInvoke(MarshallingStubGenerationContext ctx, ExpressionSyntax invoke)
private static ExpressionSyntax AddReturnValueAssignmentToInvoke(MarshallingStubGenerationContext ctx, ExpressionSyntax invoke, MarshalDirection marshallingDirection)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@copilot why add marshallingDirection parameter? Why not use ctx.ReturnValue.Direction like it did before?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added marshallingDirection because ctx.ReturnValue.Direction no longer reliably indicates the invocation assignment target. For event delegates we set return context to UnmanagedToManaged to select marshaller mode, but the invoke result is still managed and must be assigned to __retVal first. Using ctx.ReturnValue.Direction here could assign directly to native and break the flow; using generator direction keeps assignment correct (commit cbe8157).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@copilot are you sure about that? Look to me as if the change in src/SampSharp.SourceGenerator/Generators/OpenMpEventHandlerSourceGenerator.cs was the actual culprit of the bug

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You were right—the OpenMpEventHandlerSourceGenerator return-direction change was the key mismatch. I’ve now simplified this accordingly by removing the extra marshallingDirection parameter and using ctx.ReturnValue.Direction again, while keeping the corrected assignment target logic. Fixed in 2af5e80.

@ikkentim ikkentim marked this pull request as ready for review May 10, 2026 21:01
@ikkentim ikkentim merged commit d80b13a into main May 10, 2026
1 check passed
@ikkentim ikkentim deleted the copilot/fix-unmarshal-order-issue branch May 10, 2026 21:01
ikkentim added a commit that referenced this pull request May 10, 2026
…nt-npcs

* origin/main:
  Fix unmanaged→managed event marshalling order and return conversion in source generator (#525)
  Refactor: Convert player classes to ECS components (#513)
  Implement rich command processor (#523)
  Configure artifact retention: 14 days for PRs, 90 days for push (#522)
  docs: Remove 'open.mp only' restriction from IsGhostModeEnabled (#520)
  fix: Windows build scripts fail correctly on build errors (#518)
  Fix 'cdecl' attribute ignored warnings in Linux x86_64 builds (#516)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Native-to-managed marshalling source generator produces incorrect unmarshal order

2 participants