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.

Sign up for GitHub

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

Jump to bottom

Require SizeConst for UnmanagedType.ByValArray #68992

Merged
merged 13 commits into from
Jul 31, 2023
Merged

Require SizeConst for UnmanagedType.ByValArray #68992

merged 13 commits into from
Jul 31, 2023

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jul 11, 2023 •
edited
Loading

MarshalAsAttribute.SizeConst is required by all runtimes for UnmanagedType.ByValArray, so warn when it is not provided.

As mentioned below, we cannot make it an error due to back-compat.

Fixes #68988
Fixes dotnet/runtime#36134

@jkoritzinsky jkoritzinsky requested a review from a team as a code owner July 11, 2023 21:58
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 11, 2023
jaredpar
jaredpar previously requested changes Jul 11, 2023
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

This cannot be an error

@jaredpar jaredpar modified the milestones: 17.8, Backlog Jul 12, 2023
@jkoritzinsky jkoritzinsky dismissed jaredpar’s stale review July 18, 2023 20:50

The diagnostic is now a warning.

@jkoritzinsky
Copy link
Member Author

@jaredpar @AlekseyTs can I get another review pass?

AlekseyTs
AlekseyTs reviewed Jul 20, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
@@ -209,6 +209,7 @@ internal static int GetWarningLevel(ErrorCode code)
switch (code)
{
case ErrorCode.WRN_AddressOfInAsync:
case ErrorCode.WRN_ByValArraySizeConstRequired:
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

case ErrorCode.WRN_ByValArraySizeConstRequired:

Are we testing the level? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is tested here: https://github.com/dotnet/roslyn/pull/68992/files#diff-4604a322566a4681e081c334cb9516691a7308ac601887932d0d76d34b34b62dR449

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 20, 2023 •
edited
Loading 

    public void NativeTypeFixedArray()

Instead of modifying attributes in this test, we should assert the warnings. Otherwise we are not verifying metadata emitted for the affected scenario.

In reply to: 1644180537

Refers to: src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_MarshalAs.cs:565 in 60d535c. [](commit_id = 60d535c, deletion_comment = False)

@AlekseyTs
Copy link
Contributor 

    public void NativeTypeFixedArray()

Also should test that the warning is suppressed if CompilationOptions.WarningLever < 8 is used.

In reply to: 1644180537

Refers to: src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_MarshalAs.cs:565 in 60d535c. [](commit_id = 60d535c, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 20, 2023 •
edited
Loading 

    public void NativeTypeFixedArray()

We should check if any diagnostic is reported for similar VB test.

In reply to: 1644195564

Refers to: src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_MarshalAs.cs:565 in 60d535c. [](commit_id = 60d535c, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 10)

AlekseyTs
AlekseyTs approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 12)

@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler For the second review

@jkoritzinsky jkoritzinsky requested a review from a team July 26, 2023 19:57
@jaredpar jaredpar modified the milestones: Backlog, 17.8 Jul 31, 2023
@jaredpar
Copy link
Member

@jkoritzinsky can you get the merge conflicts fixed and then we should be good to merge.

@jkoritzinsky jkoritzinsky enabled auto-merge (squash) July 31, 2023 16:57
@jkoritzinsky jkoritzinsky merged commit aa308ec into dotnet:main Jul 31, 2023
@ghost ghost modified the milestones: 17.8, Next Jul 31, 2023
@jkoritzinsky jkoritzinsky deleted the sizeconst-byvalarray branch July 31, 2023 18:27
@allisonchou allisonchou mentioned this pull request Aug 3, 2023
Closed
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@jaredpar jaredpar jaredpar approved these changes

Assignees
No one assigned
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.8 P2
4 participants
@jkoritzinsky @AlekseyTs @jaredpar @dibarbet