locked
CA1001 error missed by code anlysis ?

    Soru

  • Hello everybody,

    We have noticed a strange behavior with rule CA1001 in code analysis: if a class have an IDisposable member, and does not implement IDisposable, CA reports well the error. If the member is declared using and auto-implemented property, the error is not detected by C.A.

    //Error	1	CA1001 : Microsoft.Design : Implement IDisposable on 'MyFirstDisposableContainer' because 
    //it creates members of the following IDisposable types: 'MemoryStream'.
    class MyFirstDisposableContainer
    {
    	private Stream Stream;
    
    	public MyFirstDisposableContainer() { Stream = new MemoryStream(); }
    
    	public void Foo() {...}
    }
    
    //No error reported
    class MySecondDisposableContainer
    {
    	private Stream Stream { get; set; }
    
    	public MySecondDisposableContainer() { Stream = new MemoryStream(); }
    
    	public void Foo() {...}
    }
    
    


    Is this normal ? In my humble opinion this is a C.A. bug because the 2 things are the same... Am I wrong ?

     

    Jojo

    02 Şubat 2012 Perşembe 13:16

Tüm Yanıtlar

  • They are different.

    If you were to look at these two classes in the MSIL (or the FxCop visitor) you will see that when the compiler automatically generates a backing field for a property, it gives it a special name.  (I believe it uses the $ in the name, which illegal in user code but acceptable for compiler-generated names).

    It is likely that CA1001 ignores fields with special names.  Whether that is a bug or not is debatable.  I've written a few CA rules myself (as they pertain to compiler-generated backing fields for auto-implemented properties).  I know that it is absolutely possible to determine which if any fields are auto-implemented, and so it should be possible to raise CA1001 safely in this case without producing false positive elsewhere.

    02 Şubat 2012 Perşembe 19:34
  • CA1001 (and the related CA2213 rule) will only create a violation when a field of disposable type is directly assigned an instance created using "new".  Assignment via a property or method will not trigger either rule.  If you would prefer the rule to be stricter, consider voting for this rather old Connect bug: https://connect.microsoft.com/VisualStudio/feedback/details/485291/typesthatowndisposablefieldsshouldbedisposable-rule-ca1001-is-too-permissive.

    02 Şubat 2012 Perşembe 19:43
  • Evan, you're right, the auto-implemented properties have a field that have a special name: "<NameOfTheProperty>k__FackingField", They are also marked with a 'CompilerGeneratedAttribute'. 

    There is an option in the properties of the project that is called "Suppress results from generated code" that mutes the warning when it is on automatically generated code. I guess that C.A. uses the CompilerGeneratedAttribute to determine wether a warning has to be reported or not. I have unchecked this option, and ..... unfortunatly Nicole is right too: CA does not detect the error asthe field is not assigned directly.

    //No error reported
    class MyThirdDisposableContainer
    {
    	private Stream _Stream;
    	private Stream Stream
    	{
    		get { return _Stream; }
    		set { _Stream = value; }
    	}
    
    	public MyThirdDisposableContainer() { Stream = new MemoryStream(); }
    
    	public void Foo() { }
    }
    
    No error is reported on this one, and this time it cannot be due to Generated code.

    We really hope that we will avoid writing rules such as "Do not provide a property when field is disposable"... Does anyone have (another) idea to help us detecting problem ? 

    Jojo


    09 Şubat 2012 Perşembe 16:58
  • Hi Jojo,

    I try to repro this app with your code, it seems that it is not the whole project, so can you share me a test project through "SkyDrive", and let me know how to repro the issue your described? We try to run it with our computer.

    As Nicole’s suggestion, if it is a feedback, you can submit this feedback to: http://visualstudio.uservoice.com/forums/121579-visual-studio. Microsoft engineers will evaluate them seriously, thanks for your understanding.

    Have a nice day,


    Jack Zhai [MSFT]
    MSDN Community Support | Feedback to us

    10 Şubat 2012 Cuma 07:54
  • I have uploaded a simple VS2010 project (https://skydrive.live.com/#cid=825547A800F11DA7&id=825547A800F11DA7%21125)

    We you build this one you get only 1 error report while I was hoping having 3.

    Jojo

    10 Şubat 2012 Cuma 08:46
  • Hi Jojo,

    Thank you for sharing with us, you are great! J

    But I try to build this project, it works successfully no matter whether I check or uncheck the option"Suppress results from generated code" . Does it have any settings I should do? If possible, can you tell
    me the detailed steps I can do? I really want to check it. And can you give us the scren shot about the error report?

    Best Regards,



    Jack Zhai [MSFT]
    MSDN Community Support | Feedback to us

    13 Şubat 2012 Pazartesi 08:14
  • Hi Jack,

    I have uploaded a picture showing that there is only one warning when I build while I was expecting 3 warnings.

    There is nothing special to do: just build my project and check compilation warnings in "Error List" 

    Jojo

    13 Şubat 2012 Pazartesi 10:59


  • Hi Jojo,

    I get the same result as yours, but I find that if we change the class, it can generate the Warnings. Like the following screen shot.

    Best Regards,



    Jack Zhai [MSFT]
    MSDN Community Support | Feedback to us


    14 Şubat 2012 Salı 10:15
  • Hi Jack,

    I know that if we modify the code, then the error is detected. But we have thousands of objects in our source code, and we would like CA help us to find potential leaks. So what I need is:

     - A C.A. rule that well detect errors when using properties 

    OR

    - Another rule that detects that a property is provided on a Disposable object, so we will be able to remove property for Disposable and then the CA1001 rule will work well.

    Jojo

    14 Şubat 2012 Salı 19:27
  • Jojo,

    The custom ruleset available at http://bordecalfxcop.codeplex.com/ includes a stricter CA2213 substitute that, amongst other things, detects assignment of a disposable to an automatic property.  Is that enough for you, or do you need a CA1001 substitute as well?  (Like CA2213, the substitute rule only detects a problem if the parent class is already disposable.)

    Nicole

    14 Şubat 2012 Salı 19:47
  • Hi Nicole,

    Thank you for your friendly assistance.

    Hi Jojo,

    I am trying to involve someone familiar with this topic to further look at this issue. There might be some time delay. Appreciate your patience.

    Best Regards,



    Jack Zhai [MSFT]
    MSDN Community Support | Feedback to us

    15 Şubat 2012 Çarşamba 06:22
  • Hi Nicole,

    Thanks for your package I just DL it and have a (very) quick look to it. Il may help us to acheive what we want, but I would have prefered that the Code Analysis team enhance their tool so I would have nothing to do....

    I am on hollidays tomorrow for 3 weeks, so I can be patient for your answer Jack, and for Nicole I'll study your package when I'll be back (please be patient before i give you comments/feedback :)

    Thanks to both of you

    Jojo

    15 Şubat 2012 Çarşamba 21:25
  • Hi Jojo,

    Just a quick asking, what is your version of VS2010?

    Thanks!


    Sophia Lu [MSFT]

    31 Mart 2012 Cumartesi 02:44