Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/8.0-preview5] Use fully qualified type names for parameters and don't copy parameter attributes #86899

Merged
merged 3 commits into from
May 30, 2023

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented May 30, 2023

Fixes dotnet/source-build#3483 by not copying the attributes for the parameters on generated base implementation methods since they're not strictly necessary.

…r attributes

The issue dotnet/source-build#3483 looks similar to the issues I found before fixing in #86731. The relevant changes were in ComMethodContext.cs. If it's the same issue I was hitting, the attribute syntax was being copied over without adding 'using' statements or changing the attribute name to be fully qualified. I haven't validated yet, but this should fix it by just not copying the attributes for the parameters since they're not strictly necessary.
@ghost
Copy link

ghost commented May 30, 2023

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

The issue dotnet/source-build#3483 looks similar to the issues I found before fixing in #86731. The relevant changes were in ComMethodContext.cs. If it's the same issue I was hitting, the attribute syntax was being copied over without adding 'using' statements or changing the attribute name to be fully qualified. I haven't validated yet, but this should fix it by just not copying the attributes for the parameters since they're not strictly necessary.

Author: jtschuster
Assignees: jtschuster
Labels:

area-System.Runtime.InteropServices

Milestone: -

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

Can we add a test to make sure we've fixed the issue?

@jtschuster jtschuster requested a review from mthalman May 30, 2023 18:27
@lewing lewing changed the title Use fully qualified type names for parameters and don't copy parameter attributes [release/8.0-preview5] Use fully qualified type names for parameters and don't copy parameter attributes May 30, 2023
@lewing lewing added the Servicing-consider Issue for next servicing release review label May 30, 2023
@hoyosjs hoyosjs added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 30, 2023
@carlossanlop
Copy link
Member

@jtschuster @jkoritzinsky @mthalman I see the test was added. There were test failures. Let me know if they are not related and if this is ready to merge.

@jtschuster
Copy link
Member Author

The failures look like unrelated internet issues. I'll let @jkoritzinsky confirm, but it should be ready to merge.

@lewing
Copy link
Member

lewing commented May 30, 2023

yeah wasm failure doesn't look related

@carlossanlop
Copy link
Member

carlossanlop commented May 30, 2023

The CI got reset with the branch merge. But since the CI passed before, I'll merge.

@carlossanlop carlossanlop merged commit bc78804 into release/8.0-preview5 May 30, 2023
@carlossanlop carlossanlop deleted the jtschuster-patch-1 branch May 30, 2023 21:13
@jkoritzinsky
Copy link
Member

/backport to main

@github-actions
Copy link
Contributor

Started backporting to main: https://github.com/dotnet/runtime/actions/runs/5126550773

@ghost ghost locked as resolved and limited conversation to collaborators Jun 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants