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

Specify SizeConst when marshalling field as ByValArray #36134

Closed
elinor-fung opened this issue May 8, 2020 · 3 comments · Fixed by dotnet/roslyn#68992
Closed

Specify SizeConst when marshalling field as ByValArray #36134

elinor-fung opened this issue May 8, 2020 · 3 comments · Fixed by dotnet/roslyn#68992
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@elinor-fung
Copy link
Member

elinor-fung commented May 8, 2020

When marshalling using UnmanagedType.ByValArray, the SizeConst field should be specified.

struct MyStruct
{
    [MarshalAs(UnmanagedType.ByValArray)]
    int[] Foo;
}

Category: Interoperability
Default: Enabled

cc @terrajobst @stephentoub @AaronRobinsonMSFT @jkoritzinsky

@elinor-fung elinor-fung added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer labels May 8, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 8, 2020
@AaronRobinsonMSFT
Copy link
Member

I think any of these type of requirements should be in some analyzer.

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Jun 8, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 5.0 milestone Jun 8, 2020
@elinor-fung elinor-fung modified the milestones: 5.0.0, 6.0.0 Jul 21, 2020
@jkoritzinsky jkoritzinsky modified the milestones: 6.0.0, 7.0.0 Jul 9, 2021
@jkoritzinsky jkoritzinsky added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 9, 2021
@terrajobst
Copy link
Member

  • The analyzer should probably also handle UnmanagedType.ByValTStr
    • Should be the same diagnostic ID with a slightly different message
  • Assuming code like this wouldn't work at runtime, turning the rule on by default seems fine
    • If code like this works today, then on by default might be noisy

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 13, 2021
@carlossanlop carlossanlop modified the milestones: 7.0.0, 8.0.0 Jul 6, 2022
@carlossanlop carlossanlop added the help wanted [up-for-grabs] Good issue for external contributors label Jul 6, 2022
@jkoritzinsky
Copy link
Member

Roslyn already requires SizeConst for ByValTStr. Should we instead extend Roslyn to also require SizeConst for ByValArray instead of writing a separate analyzer?

@jkoritzinsky jkoritzinsky added the in-pr There is an active PR which will close this issue when it is merged label Jul 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants