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

pattern match fails to produce a useful local variable #16466

Closed
gafter opened this Issue Jan 12, 2017 · 10 comments

Comments

Projects
None yet
7 participants
@gafter
Member

gafter commented Jan 12, 2017

@gafter ported this from a customer-reported issue on Microsoft-internal URL https://vsfeedback/comment/736281 and also https://devdiv.visualstudio.com/0bdbc590-a062-4c3f-b0f6-9383f67865ee/_workitems?id=366601

As it stands at the moment, although a pattern match expression can be used in a non-conditional statement, it doesn't produce a local variable whose value can be accessed. That makes some sense, but only just.

In the success1 case, I hope it is just that the code can't currently be analysed well enough to allow it to work.

I'm calling the success2 case an outright bug, because the success3 case works.

    var t = ...;
    var x0 = t as SomeType;
    var success0 = x0 != null;
    var success1 = t is SomeType x1;
    if (success1)
        M(x1); // CS0165 Use of unassigned local variable 'x1'
    bool success2;
    if (success2 = t is SomeType x2)
        M(x2); // CS0165 Use of unassigned local variable 'x2'
    bool success3;
    if (t is SomeType x3)
    {
        success3 = true;
        M(x3); // ok
    }
    else
    {
        success3 = false;
        x3 = null;
    }
    Console.WriteLine(success0 == success3);
    Console.WriteLine(x0 == x3);
@gafter

This comment has been minimized.

Member

gafter commented Jan 12, 2017

Unfortunately, the definite assignment rules of the language do not "see through" the assignment expression. That is, an assignment expression never results in a variable being "definitely assigned when true" or "definitely assigned when false". That explains the behavior of this similar example using previous language features:

using System;

class Program
{
    static void Main(string[] args)
    {
        bool condition = true;
        {
            int x;
            if (condition && M(out x)) Console.WriteLine(x); // ok
        }
        {
            int x;
            bool success;
            if (success = condition && M(out x)) Console.WriteLine(x); // error
        }
        {
            int x;
            bool success = condition && M(out x);
            if (success) Console.WriteLine(x); // error
        }
    }
    static bool M(out int x)
    {
        x = 1;
        return true;
    }
}

@gafter gafter removed their assignment Jan 12, 2017

@markhurd

This comment has been minimized.

markhurd commented Jan 13, 2017

OK, so rather than a lack of analyser smarts or a bug in the implementation, it's just making more obvious an issue with the language itself.

@HaloFour

This comment has been minimized.

HaloFour commented Jan 13, 2017

Indeed, this kind of problem had been highlighted repeated. It was more important for pattern variables to remain consistent in scoping with out declarations than to make sense.

Perhaps with the type of flow analysis proposed for non-nullable reference tracking the compiler could be smarter about knowing when the pattern variables would be definitely assigned. Until then it'll just be a big wart on the language.

@markhurd

This comment has been minimized.

markhurd commented Jan 13, 2017

To be fair, when adding the feature to an existing code base, I've only come across this issue in one place where it was not attempting to replicate x is T y as x.TryGetT(out y), in the source for TryGetT. But it does highlight that an out parameter should be able to be initialised by a pattern match clause, and the lack of being able to copy the boolean result means you need to say if (x is T y) result=true; else result=false; which always looks bad.

SO question that "started" this: http://stackoverflow.com/q/41569072/256431

@DavidArno

This comment has been minimized.

DavidArno commented Jan 13, 2017

@markhurd,

To be really fair, whilst you have helped @gafter uncover an existing bug/limitation in C# prior to v7, the way it all started out for you highlights how the language team really have opened a nasty can of worms here. All because they insisted on making the out var feature work the way they wanted, rather than the way C#'s scoping previously worked.

Your original code from SO is a case in point:

internal bool firstAsSymbol(out Symbol s)
{
    var result = first is Symbol sym;
    s = sym;
    return result;
}

This won't compile, as you say. Whilst the language now lets sym leak out from the first is Symbol sym expression, if first isn't of type Symbol, then sym won't be initialised. So the compiler won't let you use it, even though it permits it to be in scope. You won't be the last person to be tripped up by this nonsense.

@alrz

This comment has been minimized.

Contributor

alrz commented Jan 13, 2017

It was more important for pattern variables to remain consistent in scoping with out declarations than to make sense.

And still I'm struggling to understand what this "consistency" is good for, when they are fundamentally different things. I wonder what will happen when declaration expressions (as an standalone feature) join the party.

@markhurd

This comment has been minimized.

markhurd commented Jan 14, 2017

@alrz I conceded to Microsoft in a comment in the "is this program ready for release?" form that defaulting to setting the value to null may make the x is T v || getT(out var v) feature harder to allow in the specification, let alone implement.

@gafter

This comment has been minimized.

Member

gafter commented Jan 23, 2017

@MadsTorgersen pointed out to me

This is what the spec says about definite assignment and simple assignment:

5.3.3.23 Simple assignment expressions

For an expression expr of the form w = expr-rhs:

  • The definite assignment state of v before expr-rhs is the same as the definite assignment state of v before expr.
  • If w is the same variable as v, then the definite assignment state of v after expr is definitely assigned. Otherwise, if the assignment occurs within the instance constructor of a struct type, if w is a property access designating an automatically implemented property P on the instance being constructed and v is the hidden backing field of P, then the definite assignment state of v after expr is definitely assigned. Otherwise, the definite assignment state of v after expr is the same as the definite assignment state of v after expr-rhs.

To me it looks like this rule should provide the propagation of definite assignment state that these folks are asking for, and that the refusal of the compiler to do so is a bug. No?

@gafter gafter added the Bug label Feb 20, 2017

@jaredpar jaredpar modified the milestones: 2.0 (RTM), 15.3 Mar 27, 2017

@gafter gafter modified the milestones: 15.later, 15.3 Apr 24, 2017

@gafter

This comment has been minimized.

Member

gafter commented Apr 27, 2017

I doubt that @MadsTorgersen interpretation is correct. It would lead to some counterintuitive results. See, for example, dotnet/csharplang#271. Moving to the csharplang repo.

@gafter

This comment has been minimized.

Member

gafter commented Apr 27, 2017

Issue moved to dotnet/csharplang #507 via ZenHub

@gafter gafter closed this Apr 27, 2017

@gafter gafter removed this from Backlog in Compiler: Pattern-Matching Nov 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment