Why is this this method with an 'out' parameter saying it isn't set?

Question

UGH! Well I was going to post this as a question because I didn't know why I was seeing the error... but of course it's now so obvious when I see it. Slapping myself in the head now. I'll leave it here for fun though. See if you can catch it.

While implementing TryGetValue for our WeakDictionary class tonight, I came across something odd. I'm getting an error and I don't know why.

Here's the code:

public bool TryGetValue(TKey key, out TItem value)
{
    WeakReference<TItem> weakReference;

    if(_itemStorage.TryGetValue(key, out weakReference))
        if(weakReference.TryGetTarget(out value))
            return true;
    else
        value = default(TItem);

    return false;
}

Here's the error I'm getting:

The out parameter 'value' must be assigned to before control leaves the current method.

To me it looks like all code paths do set 'value' before it returns.

If the first 'if' fails, the 'else' clause sets 'value'.

If the first 'if' passes however, doesn't the next line 'weakReference.TryGetTarget' set 'value' for the exact same reason I'm being warned about (i.e. 'TryGetTarget' has an 'out' parameter itself, therefore it too must set its out parameter internally before it returns)?

Like I said, I was missing something obvious. (I need sleep!)

At the end of the day, this is really just a simple typo. Say what you will about always-curly-brace preferences (and automatic code indenting), but this is why I use them. — user2864740, Sep 20 '15 at 06:27
I've just tried this in multiple versions of Visual Studio and it indents the `else` statement properly, in alignment with the second `if` statement, making it obvious that it belongs to that one. Are you using another IDE? — Saeb Amini, Sep 20 '15 at 06:50
Yes, but that simple typo derailed me for a damn half an hour researching it before I slapped my forehead realizing I'm an idiot! :) — Mark A. Donohoe, Sep 20 '15 at 07:18

score 7 · Accepted Answer · edited Sep 20 '15 at 07:24

7

The problem lays in the fact that if your first if statement fails you're left with value being uninitialized.

Basically, you're missing curly braces in your if statement which will make the else statement properly attach to the proper if:

if (_itemStorage.TryGetValue(key, out weakReference))
{
    if (weakReference.TryGetTarget(out value))
        return true;
}

The docs clear this out:

The statement or statements in the then-statement and the else-statement can be of any kind, including another if statement nested inside the original if statement. In nested if statements, each else clause belongs to the last if that doesn’t have a corresponding else.

This means, that your else clause is being attached to the inner if statement, instead of the outter.

You can also re-write this as:

public bool TryGetValue(TKey key, out TItem value)
{
    WeakReference<TItem> weakReference;

    if (_itemStorage.TryGetValue(key, out weakReference))
        return weakReference.TryGetTarget(out value);

    value = default(TItem);
    return false;
}

edited Sep 20 '15 at 07:24

Mark A. Donohoe

28,442
25
137
286

answered Sep 20 '15 at 06:08

Yuval Itzchakov

146,575
32
257
321

Omitting curly braces doesn't make a difference in the sample code as each conditional statement is followed by a single statement. – Saeb Amini Sep 20 '15 at 06:18
1

@SaebAmini Try it out for yourself and see. – Yuval Itzchakov Sep 20 '15 at 06:21
The first one, as the one I have in my answer. The second will redundantly set `value` twice if both `if` statements fall through. – Yuval Itzchakov Sep 20 '15 at 06:25
@user2864740 I was looking for the proper documentation that states what I've explained. Added it to the answer. – Yuval Itzchakov Sep 20 '15 at 06:39
It seems we were talking about different sample codes. – Saeb Amini Sep 20 '15 at 06:58
@Saeb, actually as Yuval correctly pointed out, it's not a case of a single statement after each 'if'. It's a case of the indenting throwing off where the 'else' was actually applied, which was the second 'if' not the first as I thought. Silly mistake. – Mark A. Donohoe Sep 20 '15 at 07:19
@MarqueIV, I was talking about your second sample code, which you have now edited out. Your problem is really just a case of not using best practices _and_ not using proper auto-formatting to make the pitfalls of not using them obvious. As I've pointed out under your question, if you format your code with any version of Visual Studio, it makes it obvious which `else` statement is paired with which `if` statement. – Saeb Amini Sep 20 '15 at 07:55
As stated in the comments that went with that code, that code worked and wasn't the problem (which is why I removed it to shorten it) so I'm not sure why you would comment on them. I think that's why myself and others were confused by your comments. And I do use VS2015, but I do *not* like the way it formats the code, so we disabled that feature. – Mark A. Donohoe Sep 20 '15 at 10:25

Richard Schneider · Answer 2 · 2015-09-20T06:34:48.720

Remove the else statement.

Effectively the same as @Yuval's answer, but I like deleting code.

public bool TryGetValue(TKey key, out TItem value)
{
  WeakReference<TItem> weakReference;

  if(_itemStorage.TryGetValue(key, out weakReference))
    if(weakReference.TryGetTarget(out value))
        return true;

  value = default(TItem);

  return false;
}

Also note that if(c1) if(c2) is equivalent to if (c1 && c2); which reads better and would not have your problem.

score 2 · Answer 3 · answered Sep 20 '15 at 06:33

Your code is being compiled as this:

public bool TryGetValue(TKey key, out TItem value)
{
    WeakReference<TItem> weakReference;

    if (_itemStorage.TryGetValue(key, out weakReference))
    {
        if (weakReference.TryGetTarget(out value))
        {
            return true;
        }
        else
        {
            value = default(TItem);
        }
    }

    return false;
}

In this code the value variable isn't being set if the first if is false.

What you really wanted - and thought you got - was this:

public bool TryGetValue(TKey key, out TItem value)
{
    WeakReference<TItem> weakReference;

    if (_itemStorage.TryGetValue(key, out weakReference))
    {
        if (weakReference.TryGetTarget(out value))
        {
            return true;
        }
    }
    else
    {
        value = default(TItem);
    }

    return false;
}

This is the danger of not specifying the braces and assuming that the column position of the else was correct.

Why is this this method with an 'out' parameter saying it isn't set?

3 Answers3