11

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.

brickner
  • 6,595
  • 3
  • 41
  • 54
  • I wonder if the problem is elsewhere ... – Hamish Grubijan May 18 '10 at 20:42
  • Again no repro. There's nothing to validate. You've posted other code analysis warnings that don't repro. If you've made any config changes then be sure to document them. – Hans Passant May 18 '10 at 20:46
  • @Hans Passant, are you sure you're running all microsoft code analysis rules in Visual Studio 2010? – brickner May 18 '10 at 20:48
  • I don't see anybody else repro-ing this either. – Hans Passant May 18 '10 at 23:11
  • @Hans Passant, I can repro it without problems. I have a project in a solution that repro it every compilation without a doubt. I'm not sure that everyone can't repro that, you're the only one that wrote he tried and failed. – brickner May 19 '10 at 11:20
  • 2
    I *have* reproduced this in Visual Studio 2010 Premium. I just pasted in the class as given in the question, turned on *Microsot All Rules* in the settings and analysed the project. – Daniel Renshaw May 20 '10 at 12:14

2 Answers2

9

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
  • 33,729
  • 8
  • 75
  • 94
8

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
  • 12,086
  • 1
  • 33
  • 55
  • ok, but why is the correct way correct? Or do you say it's correct because there is no warning thrown in code analysis? – Gabriel Magana May 18 '10 at 21:06
  • 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 May 18 '10 at 21:07
  • 1
    @gmagana: The correct way is thread safe and won't throw NullReferenceException no matter what. – Diadistis May 18 '10 at 21:13
  • @Daniel: "All reference arguments...", out is considered reference argument too. – Diadistis May 18 '10 at 21:14
  • 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 May 18 '10 at 21:18
  • 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 May 18 '10 at 21:23
  • 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 May 20 '10 at 12:12