API-defined conditional definite assignment

[sharwell]

Proposal

I propose extending the language support for definite assignment to allow an API to declare a variable is "conditionally assigned". In its simplest form, consider the following expression:

obj is SomeType someType

The compiler already understands that someType is only definitely assigned on code flow paths where the expression evaluates to true. Now consider the same operation as an API method:

bool wasInstance = TypeChecks.TryCast(obj, out SomeType someType);

C# currently treats someType as always assigned in this case. For such an API, the compiler would be better able to detect classes of use errors at compilation time if it knew that the someType instance should only be used if wasInstance is true, in the same way the compiler handles the is operator currently.

Definite assignment variable

The definite assignment variable must be a value of type bool returned by the method. It may be any of the following:

• The return value
• If the return value is awaitable, the value returned from the GetResult method of the awaiter
• A read-write parameter passed by ref
• A parameter passed by out
• An element of one of the above cases, if the return value or parameter is a tuple type

📝 Currently the compiler is unable to follow the flow of an is pattern expression when the result of the type check is stored in an intermediate variable:

var wasInstance = obj is int x;
if (wasInstance)
  Console.WriteLine(x); // error: x not definitely assigned

Until such time as this flow analysis changes, the only usable form of the feature proposed in this issue will be cases where the return value itself is the definite assignment variable.

Conditionally-assigned variable

When a method with a definite assignment variable in the signature returns, all other returnable values are called conditionally-assigned variables. These variables are definitely assigned after the method returns if and only if the compiler can statically determine that the definite assignment variable returned is true on that code path.

Implementing methods with a definite assignment variable

When implementing a method with a definite assignment variable in the signature, at the point of return from the method one of the following must be true:

1. All return values are definitely assigned.
2. The definite assignment variable has the value false. Any other write-only return value which is not definitely assigned under the old control flow rules is assigned the value default prior to returning.
3. The definite assignment variable aliases another definite assignment variable, and all return values are definitely assigned if the aliased value is true. (This allows things like a pass-through implementation of TryGetValue.)

Examples

[DAV] bool Example1(object arg, out int value)
{
  if (arg is int x)
  {
    value = x;
    return true;
  }
  else
  {
    return false; // allowed; value is implicitly assigned default(int)
  }
}

[DAV] bool Example2(object arg, out int value)
{
  value = 1;
  return false; // allowed; value has the value 1 on return, but cannot be used
}

[DAV] bool Example3(object arg, out int value) =>
 Example2(arg, out value); // allowed

bool OldAPI(object arg, out int value) { value = 0; return true; }

[DAV] bool Example4(object arg, out int value) =>
  OldAPI(arg, out value); // allowed; OldAPI always assigns to value

[DAV] bool Example5<T>(out T value)
{
  value = default; // Warning; violation of nullable type constraint
  return true; // No warning
}

[DAV] bool Example6<T>(out T value)
{
  return false; // No warning; value is assigned unusable default before returning
}

// Helper: definitely unassigns a value at compile time
[DAV] static bool Unuse<T>(out T value) => false;

Motivation

I believe this feature will provide APIs like IDictionary<TKey, Value>.TryGetValue with a semantically precise way to cooperate with nullable reference types. I was particularly unhappy with two of the "obvious" possibilities for the signature:

• TryGetValue(TKey, out TValue):
  • With this signature, it is possible for a "non-null" value to have a null value at runtime without detection from the compiler.
• TryGetValue(TKey, out TValue?):

  • Ignoring the issues of what happens when TValue is unconstrained (and potentially nullable itself), this signature unnecessarily complicates the usage of the API. In particular, the following idiomatic code would no longer compile without warnings:

    if (map.TryGetValue(key, out var value))
      Console.WriteLine(value);

[Joe4evr]

I wonder if this proposal could be extended to a more general feature of having any bool be able to hint the compiler about certain API usages.

For example: I previously proposed something to work alongside the Nullability Analysis feature where a boolean field or property could indicate whether a different field was null or not. This would indicate to the compiler that checking the value of that bool is functionally equivalent to null-checking the other object.

I think there may be some value in considering both of these as part of a larger whole.

[alrz]

I believe this feature will provide APIs like IDictionary<TKey, Value>.TryGetValue

According to the spec, out arguments are always definitely assigned after the call, wouldn't it be a breaking change to make that conditional (at least for existing APIs)?

I think we could have an analyzer to detect "bool TryX" methods (or opt-in with an attribute) and warn if any out variable is being read on the false branch.

[sharwell]

... For example: I previously proposed something to work alongside the Nullability Analysis feature where a boolean field or property could indicate whether a different field was null or not. ...

The proposal here is intended to supersede the described behavior. In other words, for any case where you would use a Boolean value to indicate if something is null, you could better represent the case to the compiler using the feature proposed here.

I wonder if this proposal could be extended ...

I can think of other features which could benefit from similar expansions to the compiler's understanding of values in code, but none nearly so pressing as the one intended to be addressed by this proposal. This proposal is meant to cleanly address an immediate concern for another highly-anticipated feature (nullable reference types) without limiting the ability to create similar expansions in the future.

According to the spec, out arguments are always definitely assigned after the call, wouldn't it be a breaking change to make that conditional (at least for existing APIs)?

The feature would be opt-in, presumably as part of nullable reference types.

[sharwell]

@alrz Thanks for making me think of the implementation. I left that out of the original proposal; it's not updated with examples.

[sharwell]

@MadsTorgersen I believe this proposal (or one like it) would strongly complement #36 for overall usability/expressiveness/program correctness. Since it would be particularly difficult to implement this change independently of the work on that feature, I'd like to bring it up for discussion in that context.

[sharwell]

@stephentoub This feature as currently described takes an all-or-nothing approach. If it were possible to say that one or more return values were not considered "Conditionally-assigned variables", then it would be possible to implement a move method (related to dotnet/roslyn#160) that disallows the use of the source argument until it is reassigned - similar to the Unuse<T> example above.

[DavidArno]

What you are describing is already handled, far more elegantly, in other languages using discriminated unions:

Maybe<int> Example1(object arg) => arg is int x ? x : None;

...

if (Example1(someObj) is int i)
    // do stuff with i

This "conditional out" solution seems an inelegant fudge in comparison with adding DU support to C#.

[sharwell]

@DavidArno In your example, the Example1 uses a return value which is not bool to convey information. It is unable to express the semantics of existing generic signatures like TryGetValue for the purposes of supporting nullable reference types, so it's solving a totally different problem than the one I'm targeting here.

[DavidArno]

I disagree. As is noted in the Nullable reference types in C# proposal, "Certain patterns on generic types, such as FirstOrDefault or TryGet, have slightly weird behavior with non-nullable type arguments, because they explicitly yield default values in certain situations]. In both cases, rather than having to "nuance the type system", or add a work-around like you propose here:

1. The compiler could just emit warnings for those methods when non-nullable reference support is switched on,
2. Those warnings could direct the user to use new DU-based methods instead, eg:

void Foo<TKey, TValue>(IDictionary<TKey, TValue> dictionary, TKey key)
{
    // using new Maybe<TValue> TryGetValue(TKey key) method
    if (dictionary.TryGetValue(key) is TValue value)
    {
        // use value

With DU's, all problems of not being able to use default with non-nullable references go away, as eg None can be used instead of null. This seems a much cleaner solution over needing to work around null being emitted with various fudges.

[sharwell]

@DavidArno Nevertheless, the fact remains that Maybe<T> is unable to accurately express the semantics of existing signatures. They could provide the basis for an alternative proposed solution to the one seen here, but at that point we are no longer annotating the class library to express behavior with respect to null values. We are instead declaring that a relatively broad set of methods in the current framework is now obsolete and writing a new set of methods to replace them, an approach which comes with its own downsides:

1. Maybe<T> does not aid users in implementing a type like IDictionary<TKey, TValue>. While you can avoid using TryGetValue, users may still need to implement it, and ideally the implementation solution would not require the user to "break out of the type system".

2. Changing from the current TryGetValue method to one that returns Maybe<T> requires users to make more substantial changes to code bases when enabling null analysis. Each change involves a small amount of risk of introducing new bugs in the code; the proposal above mitigates this risk by identifying a large class of current usages and not requiring any code changes be made for those uses. (This proposal does not address the specific scenario where users are using TryGetValue where they really wanted GetValueOrDefault.)

3. Maybe<T> is not able to handle all types T. For example, the following signature could not be represented by Maybe<T>:

   // Returns a reference to the location where a value is stored in a dictionary,
   // if a value for the key is present.
   ref TValue TryGetBucket(TKey key, [DAV] out bool found);
   
   ref int bucket = ref map.TryGetBucket(key, out var found);
   if (found)
     bucket++;

   Note that Please add new syntax keyword "ref?" to represent that "ref return" result might be null #497 contains some alternate solutions targeting this scenario.

[Joe4evr]

for any case where you would use a Boolean value to indicate if something is null, you could better represent the case to the compiler using the feature proposed here.

So you're saying I have to change a simple property-access to a method with an out parameter just to take advantage of this? That's a no-go, because I want things to stay as simple as possible for consumers.

[sharwell]

@Joe4evr Yes and no. To use this feature, yes, it would need to change. However, this feature is not intended to provide benefits for properties because this case is already fully covered by nullable reference types: instead of returning a type T (where T is either a reference type or a generic type without the struct constraint), you would simply modify your property to return type T?.

[alrz]

I think I didn't understand the proposal as a language feature at the first glance, so having these:

[DAV] 
static bool Is(this SyntaxNode node, out BlockSyntax result)
    => node.IsKind(SyntaxKind.Block, out result);

[DAV] 
static bool IsKind<T>(this SyntaxNode node, SyntaxKind kind, out T result)
  where T : SyntaxNode
{
  if (!node.IsKind(kind))
    return false;
  result = (T)node;
  return true;
}

You'd get def assignment errors.

if (!node.Is(out BlockSyntax block)) {
  // block is not definitely assigned here
}

Pretty neat.

The fact that you can leave out parameters unassigned is important, otherwise, even an analyzer could flag undesirable reads at the call-site.

I wonder if the compiler could actually leave result unassigned (with no dummy default value).

[sharwell]

@alrz You'd need to write the IsKind method like this to avoid problems with nullable reference types, but otherwise you get the idea! Note the subtle change to the node parameter and avoiding the direct cast to (T).

[DAV] 
static bool IsKind<T>(this SyntaxNode? node, SyntaxKind kind, out T result)
  where T : SyntaxNode
{
  if (!node.IsKind(kind) || !(node is T value))
    return false;

  result = value;
  return true;
}

[sharwell]

I wonder if the compiler could actually leave result unassigned (with no dummy default value).

No, this would break compatibility with old code and code that does not opt-in to nullable reference type analysis.

[gafter]

This proposal would never be used on any existing TryGet methods, as people already use those methods expecting to get a null output when the method returns false. I've written code like that, and it is perfectly safe. Rejecting such code when compiling would be a breaking change.

[sharwell]

@gafter When nullable reference types are enabled via an explicit user option:

1. It is not safe to assume that a TValue is assigned upon return
2. It is not usable to say that the return value is TValue? and require a bunch of additional checks

Either way you have a breaking change. With this proposal, at least the break leads to a meaningful, usable result with a proper minimizing of the amount of changes users need to make in order to adopt the feature.

[yaakov-h]

Maybe I'm missing something, but I don't see how this would help nullable reference types as a whole.

Definite assignment and nullability are two different concepts. As an out parameter, it always has to have a value once the function has returned. i.e.

object foo;
var result = dictionary.TryGetValue(key, out foo);
// foo is now defined and is either an object, or null.

What you appear to be describing by "definitely assigned" is "definitely assigned a non-null value", or "definitely assigned a non-default value". These are two separate things and this is where I start to get confused.

Regarding nullability, it's quite complex. Assuming here three things from the nullability proposal / how Swift does it today:

1. A reference type as it appears today is non-nullable
2. A reference type with ? suffix is nullable
3. A reference with a ! postfix operator is treated as non-nullable when the compiler cannot prove it

If I have Dictionary<string, object?>, it is legal for any value to be null. i.e.:

object? foo;
var result = dictionary.TryGetValue(key, out foo);
// foo may or may not be null, even if TryGetValue returns true

What should DAV do in this situation?

Furthermore, if I have Dictionary<string, object>, I would expect any value to be non-null, however TryGetValue can return false, and the out parameter becomes null. This appears to be the only case DAV is trying to solve.

object? foo
var result = dictionary.TryGetValue(key, out foo);
// foo may or may not be null, depending on the return value of TryGetValue

Given this, the simplest (although still not perfect) solution for me would simply be:

if (dictionary.TryGetValue(key, out var value)) // value is object?
{
    Console.WriteLine(value!);
}

Alternatively:

if (dictionary.TryGetValue(key, out var value) && value != null) // value is of type object?
{
    // value is of type object, as the compiler can prove that value is non-null within this block
    // due to the guard in the if statement.
    Console.WriteLine(value);
}

A completely different options is new API if dictionary is of type <TKey, TValue> and not <TKey, TValue?>

var foo = dictionary.TryGetValue(key);
// foo is TValue?

However I believe the problem of how to deal with "when T is a reference type then T is nullable, otherwise when T is a value type then T is really Nullable<T>" still has yet to be solved.

[alrz]

It is not usable to say that the return value is TValue? and require a bunch of additional checks

Alternatively we could say that once the method returns true, it is non-nullable, for example:

object? o = null;
if ( o != null ) M(o); // OK
M(o); // possible null dereference

vs

if (map.TryGetValue(str, out var value)) {
  M(value); // OK
}

M(value); // possible null dereference

We should somehow correspond the method return value to out parameter nullability down the rabbit hole.

[alrz]

As an aside, from the last time I tried, an unconstrained type can't be nullable. TryGetValue would give you default(T) on failure anyways. I don't know how we're going to handle type substitution here.

var map  = new Dicitonary<string, string>(); // both non-nullable
map.TryGetValue(key, out var value); // value is TValue=string but it's actually nullable!

[sharwell]

If I have Dictionary<string, object?> ... What should DAV do in this situation?

The proposal works fine with nullable types as well. For this case, it says when TryGetValue returns true then the value parameter has a value of type object?.

Given this, the simplest ... solution for me would simply be ... [uses value!]

This solution is the simplest for authors of the nullable reference types feature. On the other hand, it is a nightmare for consumers for two reasons:

1. Nearly all uses of methods like TryGetValue will be reporting warnings after null checking is enabled. The path which is most obviously safe and recommended cannot be used without warnings.
2. It adds many places in the code where the ! operator needs to be used. This operator only "keeps the code simple" in the way the language currently keeps the code simple - by not checking nullability at all.

Alternatively: ... [checks value != null]

The check value != null adds a dead branch, a fact which will be revealed by code coverage tools and make it impossible to use this approach and also submit a pull request that has 100% coverage. Users would actually have to write this:

if ((dictionary.TryGetValue(key, out var value) | true) && value != null)

A completely different options is new API if dictionary is of type ... [returns TValue? instead of bool]

This requires all code using TryGetValue to change, a process which is likely to introduce bugs in the code through some small mistake in the conversion process. My proposal is intended to address the underlying problem while also mitigating the risk of errors for users adopting the feature.

However I believe the problem of how to deal with "when T is a reference type then T is nullable, otherwise when T is a value type then T is really Nullable<T>" still has yet to be solved.

Are you referring to the difference in meaning of T? for a generic type T based on the constraints on T (as in a problem of readability/clarity)? Or are you referring to something in my proposal?

What you appear to be describing by "definitely assigned" is "definitely assigned a non-null value", or "definitely assigned a non-default value".

This is not what I am intending to describe.

• For a nullable type, it is perfectly acceptable for a variable to be definitely assigned the value null
• For a non-nullable type, it is perfectly acceptable for a variable to be definitely assigned a default value

This proposal doesn't change whether or not a value is assigned to a parameter at runtime before a method returns. It changes the conditions for use of that value such that it may only be used if the compiler knows the type at runtime is valid according to the statically-declared type of the method.

[sharwell]

@alrz

Alternatively we could say that once the method returns true, it is non-nullable

This would eliminate the ability to use this feature on int.TryParse. It would also eliminate the ability of this features to catch certain classes of programming errors (use before valid).

[yaakov-h]

@sharwell what's the use of DAV on int.TryParse? To throw up a warning if you do var isAnInt = int.TryParse("123", out var x); Console.WriteLine(x)? If so, this is well documented to return "zero if the conversion failed", which some code can rely on - e.g. you only care if it's a valid non-zero value for int, or a truthy input for bool, etc.

Also, if I'm re-reading this correctly, this suggests a convention-based approach, rather than a decorated (attributes) approach? If so, this would break existing code such as bool TryDoTheThing(out string errorDetails), which is the exact inverse of TryGetValue or TryParse.

In general, for nullability this is a tricky thing to define well. Even in Code Contracts it's quite cumbersome and hard to read with Contract.Ensures(!Contract.Result<bool>() || Contract.ValueAtReturn(out value) != null) being the canonical way to do so IIRC.

[sharwell]

what's the use of DAV on int.TryParse?

This one is a bit more tricky, which is why I left it out of the original examples. We probably wouldn't decorate this method specifically, but users may find it valuable to decorate deserialization helper methods (wrappers) with this attribute in order to catch cases where code assumes that a value is meaningful, but in reality it was not.

This feature is not intended to avoid cases where methods have unspecified/undefined semantics. It is intended to avoid cases where code uses a value which doesn't have the desired meaning in context. For example, while TryParse is well-defined to return 0 on failure, 0 is not the value a user actually entered (for parsing user inputs). A decorated helper method could eliminate many cases where code makes faulty assumptions.

When looking at the nullable reference types feature, it's important to remember that null is not inherently bad. Null reference exceptions are just a symptom of a broader class of programming errors related to the use of variables that do not have meaningful data. Null is nothing more than the most frequent actual value that arises in cases where bugs of this form manifest. Both the feature I am proposing and the nullable reference types feature expand the compiler's understanding of what it means for a variable to hold a "valid value":

• For reference types when nullable reference types feature is added, the default value has now become an invalid value. Expanded interprocedural analysis (aided by new syntax) allows the compiler to warn in many cases where values are used before they are known to be valid.
• The feature I am proposing does exactly the same thing for an overlapping but not fully-contained set of cases where a value is used before it is known to be valid.

Also, if I'm re-reading this correctly, this suggests a convention-based approach, rather than a decorated (attributes) approach?

At this point, I have not specified the syntax for these decorations, but they would certainly need to be explicit at the point of definition. Also note that I only covered one specific case at this point. Your example of an inverted Boolean is one case that is not currently supported by this feature. Other cases include HRESULT and BOOL in interop signatures. While it is not the intent of this proposal to address those (they are secondary and far behind to the immediate concerns posed by nullable reference types feature), this proposal should not preclude the ability to address those in the future.

In general, for nullability this is a tricky thing to define well. Even in Code Contracts it's quite cumbersome ...

I agree. It's an area I've been looking at for a long time and never been particularly happy with the outcome. FWIW, I actually find this proposal to be one of the more elegant solutions. The key insight for me was realizing that the return value isn't indicating "non-null", but in reality was indicating "not meaningful in the context where the method was called".

[sharwell]

I searched the Roslyn code base for TryGetValue and found 1500+ instances. Looking through some of these, here are my observations:

tl;dr: For Roslyn.sln, this proposal appears to behave superbly for the practices followed by the development team, minimizing the overall code change requirements of nullable reference types without degrading the ability of the feature to detect bugs related to the use of invalid or uninitialized data.

Positive impressions

Overall, the vast majority of code appears to fall into the following "buckets":

1. The vast majority of code uses if statements in a way that leads to use of the conditionally-assigned variable only in cases where the definite assignment variable is true.

• The vast majority of these cases test for the definite assignment variable in a way that already works under the limitations of pattern matching is expressions - i.e. they do not assign the definite assignment variable to an intermediate variable before testing it

1. Most of the remaining cases appear to fall into these categories:
2. The use behaves like a "pass-through", e.g. an implementation of TryGetValue that passes the call on to an underlying dictionary
3. The use is intended to provide "GetValueOrDefault" behavior, which means the code already needs to be updated when using nullable reference type analysis

Areas of concern

Here are some potential cases where the behavior from this proposal would likely not due what a user was hoping for.

Assertions in test code

Example:

Assert.True(map.TryGetValue(key, out value));
Assert.Equal(10, value);

Potential solutions:

1. Expand [Proposal]: Provide a way to annotate methods that will always throw. #739 to understand the meaning of a definite assignment variable input argument

2. Rewrite the second assertion to the following:

   Assert.Equal(10, map[key]);

3. Create a helper method to rewrite the first assertion:

   // GetExistingValue is a test helper method that uses Assert to verify existence
   map.GetExistingValue(key, out value);

4. Create a helper method to rewrite the pair of assertions:

   MapAssert.Contains(10, map, key);

   📝 I've observed in the past that the pair of assertions tend to produce a poor error message when the first assertion fails. The approach of this solution could lead to a substantial improvement in diagnostic messaging in cases where tests fail.

Intentional treatment of TryGetValue as GetValueOrDefault

Example:

// Example 1
map.TryGetValue(key, out var value);
return value;

// Example 2
map.TryGetValue(key, out var value);
return value ?? new T();

Notes:

• When the map values are a reference type, the first example would already need to be updated when enabling the nullable reference types feature.
• The second example is interesting primarily because it leverages knowledge about the return value for the case where TryGetValue returns false without actually changing the return value as seen with nullable reference types enabled.

Potential solutions:

1. Use a conditional expression

   // Example 1
   return map.TryGetValue(key, out var value) ? value : default;
   
   // Example 2
   return map.TryGetValue(key, out var value) ? value : new T();

2. Use a helper method GetValueOrDefault

   // Example 1
   return map.GetValueOrDefault(key);
   
   // Example 2 (without and with lazy evaluation)
   return map.GetValueOrDefault(key, new T());
   return map.GetValueOrDefault(key, () => new T());

Necessary use of intermediate value in pass-through

⚠️ The proposal above actually allows this case to be handled without problems, but based on the current behavior of data flow analysis for the pattern matching is expression it may not work initially.

Example (taken straight from FullMetadataWriter.cs):

protected override bool TryGetTypeDefinitionHandle(ITypeDefinition def, out TypeDefinitionHandle handle)
{
    int index;
    bool result = _typeDefs.TryGetValue(def, out index);
    handle = MetadataTokens.TypeDefinitionHandle(index);
    return result;
}

Notes:

This code also uses the conditionally-assigned variable without testing the value of the definite assignment variable (see the previous case). However, the focus here is the fact that the definite assignment variable from TryGetValue is assigned to a local variable prior to returning it.

Potential solutions:

1. Update the data flow analysis to support the unambiguous use of a local variable to refer to alias a definite assignment variable

2. Rewrite the code to test the return value

   if (_typeDefs.TryGetValue(def, out var index))
   {
     handle = MetadataTokens.TypeDefinitionHandle(index);
     return true;
   }
   else
   {
     handle = MetadataTokens.TypeDefinitionHandle(0);
     return false;
   }

Intentional re-purposing of return values (contrived example)

One example of this situation is a case where an implementation of TryGetValue in user code does the following:

• The method returns false
• The method does not return default for the value
• Callers of the method need to use the value

Potential solutions:

1. Cast the instance to the concrete type containing TryGetValue; this method would not mark the return value as a definite assignment variable because the out variables are unconditionally assigned to meaningful values.

2. Use a storage location for the return value that cannot be tracked by definite assignment (e.g. fields, array elements).

   T[] resultHolder = new T[1];
   customMap.TryGetValue(key, out resultHolder[0]);
   T result = resultHolder[0];

[svick]

@sharwell I don't understand this code:

if (_typeDefs.TryGetValue(def, out var index))
{
  handle = MetadataTokens.TypeDefinitionHandle(index);
  return true;
}
else
{
  handle = MetadataTokens.TypeDefinitionHandle(index);
  return false;
}

I thought the idea here is that index would not be definitely assigned in the else branch. Or was the second assignment supposed to be something like handle = MetadataTokens.TypeDefinitionHandle(default);?

[sharwell]

@svick It was a mistake on my part. I meant to use 0 in that branch. I corrected the original comment code as well.