Skip to content
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

C# 7.0 patterns design concerns #13432

Closed
controlflow opened this issue Aug 29, 2016 · 18 comments
Closed

C# 7.0 patterns design concerns #13432

controlflow opened this issue Aug 29, 2016 · 18 comments

Comments

@controlflow
Copy link

controlflow commented Aug 29, 2016

Hi Roslyn team!

After observing the C# 7.0 design proposals and trying to imagine future ReSharper features/suggestions/transformations that would help C# developers to adopt all the great language improvements, I have a few design concerns around patterns I wish to express here. I really hope this would be any helpful to anybody.

1. The conditional nature of is expression

Ordinary is expression is actually not only type-checking, but also the null-checking expression in user's intuition...

string s = null;
s is string // false

...and 'var' is the "no-op" construct that just enables the type inference...

string s = "abc";
var s = "abc"; // exactly the same semantic

Both of those assumptions/intuitions are broken by the x is var y construct:

string s = null;
s is var t // true, t == null

Just like x is *, the x is var t construct has no real use. I can't even use it to "declare" the variable for expression somewhere deep inside other expressions without special method taking dummy boolean true from is expression like Foo(Bar( Let(Baz() is var baz, h * h) )).

I really like the idea to extend existing is and switch constructs, but maybe C# 7.0 can save user's intuition by splitting the sets of patterns into conditional/unconditional ones and restricting the use of unconditional patterns inside is expressions (and switch as well?). This would help preserving the "conditional nature" of is expression and avoid it's use to simply introduce some names (we would have better ways to do this, right?).

2. The kind of "deconstruction" we can use the most in C# code

In short: what we really need is the "!= null" + variable pattern.

After looking at my daily code in ReSharper (which is quite similar to code you have in Roslyn: lot of null and type checking over trees of closed type hierarchies), I realized I can only rewrite a very limited set of code in terms of the pattern-matching, even with all the reach proposals like positional/property patterns now postponed to C# 7+.

For example, this is the typical "deconstruction" routine:

var binaryExpression = expression as IBinaryExpression;
if (binaryExpression == null) return false;

if (binaryExpression.Kind != BinaryExpressionKind.Addition) return false;

var leftOperand = binaryExpression.LeftOperand;
if (leftOperand == null) return false;

var rightOperand = binaryExpression.RightOperand;
if (rightOperand == null) return false;

// do work

I can really turn it into the declarative pattern with property patterns, except the sad part - there is no way to express that leftOperand and rightOperand pattern variables are expected to be non-null, so I have to do manual null-checks after pattern matching:

if (expression is IBinaryExpression {
                    Kind is BinaryExpressionKind.Addition,
                    LeftOperand is var leftOperand,
                    RightOperand is var rightOperand
                  }
    && leftOperand != null // the sad part
    && rightOperand != null)
{
  // do work
}

Technically, I can workaround null-checks with the use of type patterns (damn, this looks nearly awesome!):

if (expression is IBinaryExpression {
                    Kind is BinaryExpressionKind.Addition,
                    LeftOperand is IExpression leftOperand,
                    RightOperand is IExpression rightOperand
                  })
{
  // do work
}

But I'm not sure this is the desired use of type patterns, since if we change the type of LeftOperand/RightOperand, the pattern code would compile fine but with dramatic semantic change.

I believe it's really important to have "null-checking" pattern from the beginning of "patterns invasion" into C#, since null-checks are really common thing to do during "deconstruction" of the data in C#. Type pattern has implicit null check, I believe property pattern is doing it is well, but there is no simple way to get a fresh variable bound to non-null value.

I can imagine splitting variable pattern into conditional "null-checking variable" and unconditional "nullable variable" patterns, maybe expressed in a x is var notNull and x is var? canBeNull forms respectively. This would preserve our intuition of conditional is expression + preserve type-inference-enabling nature of var:

string s = null;
s is var x // false
s is string x // false
s is var? x // true, x == null

So, "null-checking variable pattern" is really a form of type pattern with type-inferred type. And the explicit question mark for nullable variables patterns would notify user for possible null checks needed:

if (expression is IBinaryExpression {
                    LeftOperand is var leftOperand,
                    RightOperand is var? rightOperand
                  })
{
  // work with possibly unfinished binary expression
}

I know that would slightly break intuition around var-as-a-thing-that-can-be-null, but we want less nulls in future C# and nullable reference types explicitly expressed in a language syntax, do we?

3. Constness of is expressions with unconditional patterns.

Even if is expression is allowed to have unconditional patterns, I expect is expression to became constant boolean expression with true as a value, so we get the compiler warning on unreachable code:

int M(int x) {
  if (x is var t) {
    return t;
  } else {
    throw null; // unreachable
    // no return needed
  }
}
@benaadams
Copy link
Member

benaadams commented Aug 29, 2016

What if you want to typed stuff with the null?

null string -> string.Empty
null other -> other.Default

etc?

More like generics semantics than is semantics?

if (typeof(T) == typeof(sting)) 
{

}
else if (typeof(T) == typeof(other)) 
{

}

@HaloFour
Copy link

HaloFour commented Aug 29, 2016

  1. You're right that it doesn't make a lot of sense as a top-level pattern. It exists primarily for recursive patterns where you'd want to deconstruct some child value into an identifier. Is this pattern even permitted in a top-level pattern?
  2. Variable patterns aren't supposed to enforce any additional conditions, including whether or not the value is null. This is handled by type patterns. This is true of pattern matching in any functional language, including Scala and F# that have to coexist in an ecosystem full of nulls. I think that Proposal: Nullable reference types and nullability checking #5032 would alleviate many of the additional concerns as the type of the variable for a nullable reference would itself be nullable leading to warnings on dereference unless flow analysis could demonstrate a proper guard.

@controlflow
Copy link
Author

@HaloFour

  1. This exactly is why I'm calling it "null-checking variable pattern", not "variable pattern" :) To be fair, var x pattern in C# already looks different from Scala/F#'s "name patterns". But I understand your concern, maybe "null-checking pattern" deserves it's own different syntax. Maybe x is var! t or x is let t or something...

By the way, I missed this "null-checked name pattern" in F# too... you forced to write ugly match e with null -> ... | notNull -> ....

@HaloFour
Copy link

HaloFour commented Aug 29, 2016

@controlflow

I understand, and I agree that it would be useful to have a quick way to pattern-match away null. Personally I'm kind of hoping that "custom is" or "active patterns" will allow for writing helper patterns that will fill that gap and allow for writing something like the following:

var person = new Person() { FirstName = "John", LastName = "Smith" };
// later
if (person is Person { LastName is Some(var lastName) }) {
    Console.WriteLine($"LastName is not null and is \"{lastName}\".");
}

Alternately maybe a not pattern:

if (person is not null) // feels like VB?

@alrz
Copy link
Contributor

alrz commented Aug 29, 2016

@HaloFour

Variable patterns aren't supposed to enforce any additional conditions, including whether or not the value is null.

var never change the semantics of the code. The current implementation is actually inconsistent with rules that we have today.

I'll note that in property patterns you will be able to elide the type and obviously, check for nulls. It'd be unfortunate to lose this ability in the simplest form of the patterns.

@HaloFour
Copy link

@alrz

The inconsistency comes from blending type declaration and matching into one expression and it's kind of unavoidable. The only way to really fix that would be through the following, which I don't think anybody wants:

string s1 = null;
Debug.Assert(s1 is string s2 && object.ReferenceEquals(s1, s2));

Which puts var into the awkward position, but var can implicitly be anything so it only makes sense that it can match anything.

That said, I totally don't think that it should be legal to include var_pattern under pattern, but instead only allow it under subpattern or property_subpattern. That way you avoid the whole:

string s1 = null;
if (s1 is var s2) { ... } // what's the point of this?

@alrz
Copy link
Contributor

alrz commented Aug 29, 2016

@HaloFour I think that was initially proposed to make case var x possible which is useful if you are switching on an expression that is not a variable itself. But I agree, is var doesn't make any sense.

if (s1 is var s2)

Looks like a useless declaration expression (#254) because it always return true.

@VSadov
Copy link
Member

VSadov commented Aug 29, 2016

cc @gafter

@HaloFour
Copy link

@alrz

That's true, it does make sense with switch (and match) expressions as a catch-all that happens to capture the value of the matching expression. Still feels wrong to allow it with is expressions but I don't know that it would be worth it to separate the grammar out to disallow it in that situation. A compiler warning seems to be in order, though.

I've probably seen the current syntax for so long that I've grown accustomed to it. Having var be akin to a capturing wildcard just feels right at this point. I don't disagree that a non-nullable form would be useful but I can't think of a syntax for it that would also fit in with other syntax in C#.

@yume-chan
Copy link

yume-chan commented Aug 29, 2016

if (Foo() is var foo && foo.bar) { /* Use foo */ } is reasonable, although with the new scope rule it leaks to outside of if, as few people want.

I'm always wanting a is not b when I write !(a is b).

@controlflow
Copy link
Author

@CnSimonChan do we really think, that turning conditional language construct used to do type testing today... into unconditional variable introduction construct tomorrow... is "reasonable"? I'm not sure :)

@alrz
Copy link
Contributor

alrz commented Aug 30, 2016

I am convinced that var patterns should not do any null check; if it would ever be allowed to use recursive patterns in the context of out parameters in addition to tuple deconstructions (#11293), the simplest form will be a var pattern which must not do any null check.

Note that var already infers nullabiliy of a variable e.g. var x = new int?(2) so if we treat var as an inferred non-nullable type and var? as an inferred nullable type in patterns , it wouldn't be consistent.

However, I do believe something akin to implicitly unwrapped optionals in Swift would be useful when non-nullable references are introduced in the language. So I think var! syntax would be a good candidate for this purpose.

@qrli
Copy link

qrli commented Aug 31, 2016

Maybe just disallow is var in C# 7?

@controlflow
Copy link
Author

I must admit: this was a bad idea to suggest var x as a "not-null variable pattern" syntax. var! x or something else would be a better choice. I was thinking of this syntax only because it some kind of duality between var s = ...;/string s = ...; declarations and t is T x/t is var x expressions.

Anyway, I think two core problems must be addressed in some way:

  1. Unconditional patterns under is expression,
  2. Missing not-null variable pattern.

@gafter
Copy link
Member

gafter commented Sep 1, 2016

We already discussed this at great length, and found the current state to be the least offensive overall solution.

@gafter gafter added this to the 2.0 (RTM) milestone Sep 1, 2016
@alrz
Copy link
Contributor

alrz commented Jan 2, 2017

What about an "optional pattern" (from Swift),

case var x?:
// shorthand for 
case var x when x != null:

I think this would be very useful, specifically in scenarios mentioned by OP.

@gafter
Copy link
Member

gafter commented Jan 2, 2017

@alrz The OP's use cases are recursive patterns, which are not part of C# 7.

@gafter
Copy link
Member

gafter commented Mar 11, 2017

We'll be tracking language design issues in the csharplang repo, so I'm closing this in the Roslyn repo. If there are still issues here that interest you, you are most welcome to open issues there.

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

No branches or pull requests

9 participants