Mark A. Donohoe
Mark A. Donohoe

Reputation: 30428

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

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!)

Upvotes: 0

Views: 431

Answers (3)

Yuval Itzchakov
Yuval Itzchakov

Reputation: 149588

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;
}

Upvotes: 7

Richard Schneider
Richard Schneider

Reputation: 35477

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.

Upvotes: 4

Enigmativity
Enigmativity

Reputation: 117144

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.

Upvotes: 2

Related Questions