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

SA1133 on P/Invoke method parameters #1882

Closed
yaakov-h opened this issue Dec 2, 2015 · 6 comments
Closed

SA1133 on P/Invoke method parameters #1882

yaakov-h opened this issue Dec 2, 2015 · 6 comments
Assignees
Milestone

Comments

@yaakov-h
Copy link

yaakov-h commented Dec 2, 2015

Sample code:

#pragma warning disable SA1600
#pragma warning disable SA1649
#pragma warning disable SA1652

namespace SA1133Crash
{
    using System;
    using System.Runtime.InteropServices;

    internal static class NativeMethods
    {
        [DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
        public static extern IntPtr CreateJobObject([In, Optional] IntPtr security_attributes, string lpName);
    }
}

Of particular concern is this [In, Optional] IntPtr security_attributes,.

  1. Is this a valid violation of SA1133? How is [In] [Optional] any better than [In, Optional]? I agree for attributes on classes, methods and properties that they should be separate and on separate lines, but not neccesarily for parameters.
  2. Attempting to use the code fix throws this exception, which is likely a duplicate of SA1133CodeFixProvider crash #1878:
System.InvalidCastException occurred
Message: Exception thrown: 'System.InvalidCastException' in StyleCop.Analyzers.CodeFixes.dll
Additional information: Unable to cast object of type 'Microsoft.CodeAnalysis.CSharp.Syntax.AttributeListSyntax' to type 'Microsoft.CodeAnalysis.CSharp.Syntax.AttributeSyntax'.

Using VS2015 Update 1 (RTM), StyleCop.Analyzers 1.0.0-RC

@sharwell
Copy link
Member

sharwell commented Dec 2, 2015

I would be fine with excluding attributes on parameters.

/cc @vweijsters

@sharwell sharwell added this to the 1.0.0 RC 2 milestone Dec 2, 2015
@vweijsters
Copy link
Contributor

I'm fine with excluding attributes checks for parameters, since SA1134 will not be enforced for parameters as well.
This would in my opinion include generic parameters.

@vbfox
Copy link

vbfox commented Dec 2, 2015

I saw the same thing and was annoyed too. Even if parameter attributes aren't excluded by default, it should be a configurable setting.

And @yaakov-h if you enjoy P/Invoke you should come play with us on https://github.com/AArnott/pinvoke we're trying to build nuget packages with all the Win32 P/Invoke

@yaakov-h
Copy link
Author

yaakov-h commented Dec 2, 2015

@vbfox I don't think "enjoy" is the right verb to use for any discussion of P/Invoke 😄

@sharwell
Copy link
Member

sharwell commented Dec 2, 2015

@vweijsters Are you able to update SA1133 and SA1134 (documentation + impl + tests) to exclude parameters and type parameters from analysis? I'd like to include this in RC 2 since it makes these new rules easier to adopt.

@vweijsters
Copy link
Contributor

@sharwell I will work on this today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants