New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend definite assignment to understand null and nullable values and ?. #916

Open
IEvangelist opened this Issue Sep 17, 2017 · 11 comments

Comments

Projects
None yet
7 participants
@IEvangelist

IEvangelist commented Sep 17, 2017

I am currently using the following IDE:

Microsoft Visual Studio Enterprise 2017 
Version 15.3.4
VisualStudio.15.Release/15.3.4+26730.15
Microsoft .NET Framework
Version 4.7.02046

Installed Version: Enterprise

I'm have a simple .NET Core 2.0 application that I have built targeting C# 7.1 from the Build > Advanced > Language Version selection. When I attempt to compile the following program, I get an unexpected compiler error.

using System.Collections.Concurrent;

namespace IEvangelist.Csharp.Seven
{
    class Program
    {
        public static void Main() { }
    }

    public static class ConcurrentExtensions
    {
        public static T TryPeekOrDefault<T>(this ConcurrentQueue<T> queue)
            => (queue?.TryPeek(out T result) ?? false) ? result : default;
    }
}

Error reported:

Severity Code Description Project File Line Suppression State
Error CS0165 Use of unassigned local variable 'result' IEvangelist.Csharp.Seven C:\Users\david.pine\Documents\Visual Studio 2017\Projects\IEvangelist.Csharp.Severn\IEvangelist.Csharp.Severn\Program.cs 33 Active

Expected Behavior

No compiler error. The compiler has all the details it needs to know that this is valid. The true path of the ternary is what the IDE is complaining about; however, I think it shouldn't.

image

@HaloFour

This comment has been minimized.

Show comment
Hide comment
@HaloFour

HaloFour Sep 17, 2017

Contributor

The error is correct, the ?. operator short-circuits, so if queue is null then result will never be assigned.

Contributor

HaloFour commented Sep 17, 2017

The error is correct, the ?. operator short-circuits, so if queue is null then result will never be assigned.

@MkazemAkhgary

This comment has been minimized.

Show comment
Hide comment
@MkazemAkhgary

MkazemAkhgary Sep 17, 2017

Compiler does not know, if expression returns true, result will be assigned, it can not make assumptions. hence the error you get. I see. you are expecting more intelligence! :-)

you can fix this by

queue == null ? default(T) : queue.TryPeek(out T result) ? result : default(T);

I don't think this can be simplified further in one line.

MkazemAkhgary commented Sep 17, 2017

Compiler does not know, if expression returns true, result will be assigned, it can not make assumptions. hence the error you get. I see. you are expecting more intelligence! :-)

you can fix this by

queue == null ? default(T) : queue.TryPeek(out T result) ? result : default(T);

I don't think this can be simplified further in one line.

@MkazemAkhgary

This comment has been minimized.

Show comment
Hide comment
@MkazemAkhgary

MkazemAkhgary Sep 17, 2017

@HaloFour I guess some of new c#7 does not work very well. this one is good find. you can not use out T value in combination with .? operator. if you do, content of value will be completely unusable unless you assign something else to it in which case you loose other content.

similar issue where you can not use contents of local variable..

switch(obj) 
{     
    case A a:     
    case B b:     
      // impossible to use content of a or b
     break; 
}

There are similar issues too not just with c#7, its just matter of writing right syntax for it.

how ever maybe they could introduce ?= operator, this will be conditional assignment and happens if value is not assigned. then in current post user could do

=> queue?.TryPeek(out T result) == null ? default(T) : result ?= default(T);

just for fun, oh does this require changes in CLR? :-)

MkazemAkhgary commented Sep 17, 2017

@HaloFour I guess some of new c#7 does not work very well. this one is good find. you can not use out T value in combination with .? operator. if you do, content of value will be completely unusable unless you assign something else to it in which case you loose other content.

similar issue where you can not use contents of local variable..

switch(obj) 
{     
    case A a:     
    case B b:     
      // impossible to use content of a or b
     break; 
}

There are similar issues too not just with c#7, its just matter of writing right syntax for it.

how ever maybe they could introduce ?= operator, this will be conditional assignment and happens if value is not assigned. then in current post user could do

=> queue?.TryPeek(out T result) == null ? default(T) : result ?= default(T);

just for fun, oh does this require changes in CLR? :-)

@HaloFour

This comment has been minimized.

Show comment
Hide comment
@HaloFour

HaloFour Sep 17, 2017

Contributor

@MkazemAkhgary

C#7 has nothing to do with it. If the variable was declared in a prior statement (but not assigned) the result would be exactly the same. And that behavior is correct. If queue is null then TryPeek is never called and result is never passed as an out parameter which is the only path to which it is assigned a value. The compiler does not want you to use a variable if it is not definitely assigned by all paths through which that code could flow. That's been the case since C# 1.0.

As the behavior is correct there is nothing to correct here. But, for fun, no, changing that would not require CLR changes. The CLR only has a flat method scope for variables and no concept of definite assignment. Those are strictly C# features.

Contributor

HaloFour commented Sep 17, 2017

@MkazemAkhgary

C#7 has nothing to do with it. If the variable was declared in a prior statement (but not assigned) the result would be exactly the same. And that behavior is correct. If queue is null then TryPeek is never called and result is never passed as an out parameter which is the only path to which it is assigned a value. The compiler does not want you to use a variable if it is not definitely assigned by all paths through which that code could flow. That's been the case since C# 1.0.

As the behavior is correct there is nothing to correct here. But, for fun, no, changing that would not require CLR changes. The CLR only has a flat method scope for variables and no concept of definite assignment. Those are strictly C# features.

@svick

This comment has been minimized.

Show comment
Hide comment
@svick

svick Sep 17, 2017

Contributor

@HaloFour If queue is null, then the condition is false and so the result variable is not accessed. So the variable is assigned whenever it's accessed, even if definite assignment rules from the spec don't allow it to be accessed.

So, there is indeed nothing to correct, but I think there is something to improve. (Whether it's worth improving is another question entirely.)

Contributor

svick commented Sep 17, 2017

@HaloFour If queue is null, then the condition is false and so the result variable is not accessed. So the variable is assigned whenever it's accessed, even if definite assignment rules from the spec don't allow it to be accessed.

So, there is indeed nothing to correct, but I think there is something to improve. (Whether it's worth improving is another question entirely.)

@HaloFour

This comment has been minimized.

Show comment
Hide comment
@HaloFour

HaloFour Sep 17, 2017

Contributor

@svick

Maybe. That starts to wade into mixing definite assignment with flow analysis. Even though it all looks like a single expression it expands out to numerous statements and the compiler requires that both operands meet definite assignment since it can't base that assignment on whether the bool expression is true or false. I'm certainly not opposed to such flow analysis, though.

Contributor

HaloFour commented Sep 17, 2017

@svick

Maybe. That starts to wade into mixing definite assignment with flow analysis. Even though it all looks like a single expression it expands out to numerous statements and the compiler requires that both operands meet definite assignment since it can't base that assignment on whether the bool expression is true or false. I'm certainly not opposed to such flow analysis, though.

@IEvangelist

This comment has been minimized.

Show comment
Hide comment
@IEvangelist

IEvangelist Sep 18, 2017

Thank you all for the replies, clarifications and thoughts... much appreciated. I agree with @svick after revisiting this again. I do however see it as an opportunity for improvement. Flow analysis (outside of definite assignment) is starting to make its way into proposals and prototypes for C# 8. I'd love to see this improved as part of that.

IEvangelist commented Sep 18, 2017

Thank you all for the replies, clarifications and thoughts... much appreciated. I agree with @svick after revisiting this again. I do however see it as an opportunity for improvement. Flow analysis (outside of definite assignment) is starting to make its way into proposals and prototypes for C# 8. I'd love to see this improved as part of that.

@mattwar mattwar added the Discussion label Sep 18, 2017

@gafter

This comment has been minimized.

Show comment
Hide comment
@gafter

gafter Sep 23, 2017

Member

@IEvangelist You say you'd like to see this "improved". What would you like to be better? Is the error message unclear?

Member

gafter commented Sep 23, 2017

@IEvangelist You say you'd like to see this "improved". What would you like to be better? Is the error message unclear?

@gafter

This comment has been minimized.

Show comment
Hide comment
@gafter

gafter Sep 23, 2017

Member

Oh, I see you'd like definite assignment (which is part of flow analysis) to infer that the variable is only referenced where it was definitely assigned in this case. A change to accomplish that would require a change to the definite assignment spec at https://github.com/dotnet/csharplang/blob/master/spec/variables.md#definite-assignment .

Member

gafter commented Sep 23, 2017

Oh, I see you'd like definite assignment (which is part of flow analysis) to infer that the variable is only referenced where it was definitely assigned in this case. A change to accomplish that would require a change to the definite assignment spec at https://github.com/dotnet/csharplang/blob/master/spec/variables.md#definite-assignment .

@gafter gafter changed the title from Unexpected compiler error: use of unassigned local variable to Extend definite assignment to understand null and nullable values and ?. Sep 23, 2017

@IEvangelist

This comment has been minimized.

Show comment
Hide comment
@IEvangelist

IEvangelist Apr 13, 2018

Has there been any internal discussion on this issue?

IEvangelist commented Apr 13, 2018

Has there been any internal discussion on this issue?

@TheFlow0360

This comment has been minimized.

Show comment
Hide comment
@TheFlow0360

TheFlow0360 Aug 24, 2018

Encountered this "problem" today, and I'd really love if definite assignment could be extended to work in this case. In my case I have to preassign my out value which is an enum. Using default(EnumType) at least doesn't look as bad as randomly assigning one value, but it's still not pretty.

static void Main(string[] args)
{
    IDictionary<String, MyType> dictionary = null;
   
   ... // code potentially assigning dictionary        

    var x = default(MyType);           // <-- this is not pretty
    var mappedValue = dictionary?.TryGetValue("abc", out x) ?? false ? Map(x) : OtherType.None;
}

enum MyType  // this enum doesn't really have any default value
{
    Whatever,
    DontCare,
    Irrelevant
}

TheFlow0360 commented Aug 24, 2018

Encountered this "problem" today, and I'd really love if definite assignment could be extended to work in this case. In my case I have to preassign my out value which is an enum. Using default(EnumType) at least doesn't look as bad as randomly assigning one value, but it's still not pretty.

static void Main(string[] args)
{
    IDictionary<String, MyType> dictionary = null;
   
   ... // code potentially assigning dictionary        

    var x = default(MyType);           // <-- this is not pretty
    var mappedValue = dictionary?.TryGetValue("abc", out x) ?? false ? Map(x) : OtherType.None;
}

enum MyType  // this enum doesn't really have any default value
{
    Whatever,
    DontCare,
    Irrelevant
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment