views:

151

answers:

2

I have a very simple code (simplified from the original code - so I know it's not a very clever code) that when I compile in Visual Studio 2010 with Code Analysis gives me warning CA1062: Validate arguments of public methods.

public class Foo
{
    protected static void Bar(out int[] x)
    {
        x = new int[1];
        for (int i = 0; i != 1; ++i)
            x[i] = 1;
    }
}

The warning I get:

CA1062 : Microsoft.Design : In externally visible method 'Foo.Bar(out int[])', validate local variable '(*x)', which was reassigned from parameter 'x', before using it.

I don't understand why do I get this warning and how can I resolve it without suppressing it? Can new return null? Is this a Visual Studio 2010 bug?

UPDATE

I've decided to open a bug report on Microsoft Connect.

+2  A: 

I've reproduced this in Visual Studio 2010 Premium with the code exactly as given and with Microsoft All Rules enabled in the analysis settings.

It looks like this is a bug (see bottom of here: http://msdn.microsoft.com/en-us/library/ms182182.aspx). It is complainng that you are not checking that x is not null before using it, but it's on out parameter so there is no input value to check!

Daniel Renshaw
A: 

It's easier to show than to describe :

public class Program
{
    protected static int[] testIntArray;

    protected static void Bar(out int[] x)
    {
        x = new int[100];
        for (int i = 0; i != 100; ++i)
        {
            Thread.Sleep(5);
            x[i] = 1; // NullReferenceException
        }
    }

    protected static void Work()
    {
        Bar(out testIntArray);
    }

    static void Main(string[] args)
    {
        var t1 = new Thread(Work);
        t1.Start();

        while (t1.ThreadState == ThreadState.Running)
        {
            testIntArray = null;
        }
    }
}

And the correct way is :

    protected static void Bar(out int[] x)
    {
        var y = new int[100];

        for (int i = 0; i != 100; ++i)
        {
            Thread.Sleep(5);
            y[i] = 1;
        }

        x = y;
    }
Diadistis
ok, but why is the correct way correct? Or do you say it's correct because there is no warning thrown in code analysis?
gmagana
What you've shown is certainly an important point (if x might be accessed by multiple threads) but I don't think this is what CA1062 is intended to highlight. If you read the documentation here: http://msdn.microsoft.com/en-us/library/ms182182.aspx, it's obvious this is intended for `ref` parameters and is just a standard "check it's not null before using it" rule. It's a bug that it's being applied to `out` parameters. Does this actually prevent the CA1062 warning?
Daniel Renshaw
@gmagana: The correct way is thread safe and won't throw NullReferenceException no matter what.
Diadistis
@Daniel: "All reference arguments...", out is considered reference argument too.
Diadistis
Yeah, ok. But `out` is an output only reference argument while `ref` is input/output reference argument. There's no need to perform *input validation* (which is what CA1062 is about) on an output-only parameter.
Daniel Renshaw
The only difference between `out` and `ref` is that you have to assign a value to the parameter, it's like a contract. When you access `x` by using the indexer you're actually reading it.
Diadistis
But it's not possible to access it using the indexer until *after* you've assigned it a value *inside* the method. If you try, it will fail to compile. There is no way to pass information into a method via an `out` parameter.
Daniel Renshaw