Roslyn: "Simplify Default Expression" produces bad constant pattern.

[gafter]

In this code under a recent language version

``` c#
class Program
{
public static void Main()
{
object o = null;
int i = 1;
char c = '\0';

    if (o is default(object)) { }
    if (i is default(int)) { }
    if (c is default(char)) { }
}

}

The IDE offers to simplify things to

``` c#
        if (o is default) { }
        if (i is default) { }
        if (c is default) { }

However, that is not correct and causes a compile-time error. default cannot be used as a pattern.

The IDE should instead offer to simplify things to

c# if (o is null) { } if (i is 0) { } if (c is '\0') { }

Generally speaking, this fixer should offer null as a simplification of default(T) when T is a reference type or nullable value type, 0m when T is decimal, 0 when T is another numeric type, false if T is bool, and '\0' if T is char. That covers all of the situations you can use in a pattern, so it is fine to offer default for other types.

[gafter]

/cc @jcouv

[Neme12]

duplicate of #25337

edit: I guess I lost here. closing the other one.

[CyrusNajmabadi]

However, that is not correct and causes a compile-time error.

Did that change at some point? I thought 'default' was allowed here once. Thanks!

--

Also, it's a bit weird that this happens. We run a check that asks the compiler if "o is default" has hte same semantics as "o is default(object)". i.e.:

c# else if (currentOriginalNode.Kind() == SyntaxKind.DefaultExpression) { return !TypesAreCompatible((ExpressionSyntax)currentOriginalNode, (ExpressionSyntax)currentReplacedNode); }

This checks if "default(object)" and "default" have the same types. As "default" is not legal here, it's surprising that the compiler says hte type is "object". Because of that, it looks fine for us to replace as nothing about it has changed.

If the compiler reported that "default" here has the error type (or 'null' type), then this would work as expected.

[Neme12]

The IDE should instead offer to simplify things to

IMHO it's a little weird that a fix called "Simplify default expression" would actually remove the default keyword and put in a constant. I would expect it to just no be offered. I guess inserting a constant literal might be what people want, but it could use a different message. I doubt that many people would be affected by this though, so I'm not sure if that's worth doing just for this 1 special case.

---

I guess inserting a constant literal might be what people want

but actually if they wanted a constant, why didn't they make it a constant in the first place? They sure knew they could do that when they wrote the code as constants aren't a new feature. Therefore typing default(whatever) was very deliberate. I doubt this would actually be desired behavior.

[CyrusNajmabadi]

IMHO it's a little weird that a fix called "Simplify default expression" would actually remove the default keyword and put in a constant. I would expect it to just no be offered.

I'm fine with either behavior. But i lean more toward simply not offering the feature there. If a user wrote "default(...)" it stands to reason that they prefer that form. We should keep it. in other words, they've always been able to write constants since c# 1. But default is a 'newer' construct. If they're using it, we should probably assume that's what they want.

[gafter]

but actually if they wanted a constant, why didn't they make it a constant in the first place?

They did. Any default(T) expression that is accepted in a pattern is a constant and can be written more clearly as a literal.

[Neme12]

Sorry, I meant a constant literal (as opposed to 'default').

[Neme12]

My point is: if the user typed default(int) as opposed to 0, that was a very deliberate choice (because they sure knew they could have just typed 0) and we shouldn't interfere with their (albeit unusual) preference.

On the other hand, changing to default might likely be desirable for them because either that simply wasn't an option before or they just don't know about the feature. But I'm pretty sure everyone is familiar with numeric literals.

I would simply disable the codefix here (for default(int) inside a pattern). And maybe also create a separate code fix for this specific compiler error to convert default to default(int).

As for converting to literals, that could definitely be considered to be created as a separate refactoring and it would work for both default, default(int) and not just inside a pattern. I would not make this an analyzer with a code fix unless we also add a code style option for that. But either way, I would consider that a separate proposal independent from just this particular scenario. But as I said, such a feature doesn't seem as important to me because the user must have typed default as opposed to 0 for a reason, whether we agree with it or not.
@CyrusNajmabadi what do you think?

[Neme12]

And more importantly, "Simplify default expression" is controlled by a code style option "Prefer simple 'default' expression" that says whether to prefer default to default(T). It doesn't mention anything about (non-default) literals nor do I think it should. We would be conflating two different features/style preferences here. What would such a setting even be called? "Prefer either default or an equivalent type-specific literal based on whether the expression is inside a pattern"? or "based on whether the value of the expression is a constant"? Doesn't seem very intuitive to me. These are clearly two different options.

[CyrusNajmabadi]

Just to weigh in:

I think several opinions are correct here.

1. It's clearly wrong when the IDE produces broken code. We need to address that.
2. We have a feature today that advertises itself as simplifying default(...) to default. We should probably keep that feature as is.
3. It sounds like a reasonable idea to introduce a feature that simplifies default(...) (or even default) to a constant. Users could set their preference there and then get the behavior they want.

--

just wanted to separate out all the concerns.

[Neme12]

Related:
c# switch (true) { case (bool)default: break; }
"Remove unnecessary cast" produces broken code as well here.

---

This leads me to think that there will likely be many IDE features that break this and they'll all have to treat this as a special edge case, unless something was done in the compiler as @CyrusNajmabadi mentioned.

[Neme12]

I hope it's OK with @gafter if I consider this fixed as part of #25538 and create two separate issues for converting default to other literals (#25556) and for a code fix specifically aimed at the the compiler error (#25557) if the user already wrote a default literal in the wrong place a needs to fix the error(s).