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
"Simplify Default Expression" produces bad constant pattern. #25456
Comments
/cc @jcouv |
duplicate of #25337 edit: I guess I lost here. closing the other one. |
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.: 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. |
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.
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 |
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. |
They did. Any |
Sorry, I meant a constant literal (as opposed to 'default'). |
My point is: if the user typed On the other hand, changing to I would simply disable the codefix here (for As for converting to literals, that could definitely be considered to be created as a separate refactoring and it would work for both |
And more importantly, "Simplify default expression" is controlled by a code style option "Prefer simple 'default' expression" that says whether to prefer |
Just to weigh in: I think several opinions are correct here.
-- just wanted to separate out all the concerns. |
Related: 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. |
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). |
In this code under a recent language version
The IDE offers to simplify things to
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
Generally speaking, this fixer should offer
null
as a simplification ofdefault(T)
whenT
is a reference type or nullable value type,0m
whenT
isdecimal
,0
whenT
is another numeric type,false
ifT
isbool
, and'\0'
if T ischar
. That covers all of the situations you can use in a pattern, so it is fine to offerdefault
for other types.The text was updated successfully, but these errors were encountered: