-
Notifications
You must be signed in to change notification settings - Fork 510
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
Fix SA1023 for C# 9 function pointer parameters #3252
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3252 +/- ##
=======================================
Coverage 94.39% 94.40%
=======================================
Files 967 969 +2
Lines 108429 108525 +96
Branches 3675 3677 +2
=======================================
+ Hits 102356 102452 +96
Misses 5143 5143
Partials 930 930 |
StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/SpacingRules/SA1023CSharp9UnitTests.cs
Outdated
Show resolved
Hide resolved
StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/SpacingRules/SA1023CSharp9UnitTests.cs
Outdated
Show resolved
Hide resolved
StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/SpacingRules/SA1023CSharp9UnitTests.cs
Outdated
Show resolved
Hide resolved
.../StyleCop.Analyzers/SpacingRules/SA1023DereferenceAndAccessOfSymbolsMustBeSpacedCorrectly.cs
Show resolved
Hide resolved
.../StyleCop.Analyzers/SpacingRules/SA1023DereferenceAndAccessOfSymbolsMustBeSpacedCorrectly.cs
Outdated
Show resolved
Hide resolved
StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/SpacingRules/SA1023CSharp9UnitTests.cs
Outdated
Show resolved
Hide resolved
StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/SpacingRules/SA1023CSharp9UnitTests.cs
Outdated
Show resolved
Hide resolved
unsafe delegate {|#0:*|}<int*> FuncPtr1; | ||
unsafe delegate{|#1:*|} <int*> FuncPtr2; | ||
unsafe delegate {|#2:*|} managed<int*> FuncPtr3; | ||
unsafe delegate{|#3:*|}managed<int*> FuncPtr4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 IIRC, the unmanaged
keyword only works for .NET 5 and newer. I believe there are some instances of tests in the repository using ReferenceAssemblies.Net.Net50
explicitly. Considering function pointers are only officially supported for .NET 5 and newer, it would be fine to just update this test to always use it and test both managed
and unmanaged
cases. I'm happy to leave this for a follow-up PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... error CS8889: The target runtime doesn't support extensible or runtime-environment default calling conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler is looking for this field, which is only available in .NET 5 and newer:
https://github.com/dotnet/runtime/blob/2445324a501efaca19e136b7a7ca46d8ab5d95ec/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeFeature.cs#L18-L21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I specify a calling convention to make the build pass as in #3254?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer the test be updated to this form instead:
Lines 41 to 54 in 6399f49
await new CSharpTest(LanguageVersion.CSharp9) | |
{ | |
ReferenceAssemblies = GenericAnalyzerTest.ReferenceAssembliesNet50, | |
TestCode = testCode, | |
ExpectedDiagnostics = | |
{ | |
// /0/Test0.cs(2,15): warning SA1300: Element 'r' should begin with an uppercase letter | |
Diagnostic().WithLocation(0).WithArguments("r"), | |
// /0/Test0.cs(2,15): warning SA1300: Element 'r' should begin with an uppercase letter | |
Diagnostic().WithLocation(0).WithArguments("r"), | |
}, | |
FixedCode = fixedCode, | |
}.RunAsync(CancellationToken.None).ConfigureAwait(false); |
Thanks @NextTurn ! |
No description provided.