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

Inconsistent/ambigious C# 'default' operator behavior with value types #2125

Closed
ghost opened this issue Jan 8, 2019 · 16 comments
Closed

Inconsistent/ambigious C# 'default' operator behavior with value types #2125

ghost opened this issue Jan 8, 2019 · 16 comments

Comments

@ghost
Copy link

ghost commented Jan 8, 2019

Hi, I am not entirely sure about this. But personally, I don't think this is ever right in terms of secure coding.
Here it is:

Guid? x = default(Guid?); this is equivalent to Guid? x = default;
which produce null (it is fine).

Let's have the following
Guid y = Guid.NewGuid();
then
Guid? z = (y == default) ? y : default(Guid?);
this will make z equal to null. Which again is valid. But if you do
Guid? z = (y == default) ? y : default;
this will make z equal to 00000000-0000-0000-0000-000000000000 (default value of System.Guid)

Implicitly, you mean here the default for z not y.

I think this is a problem in terms of secure coding since z == null will always fail since it is equal to 00000000-0000-0000-0000-000000000000
More worse (in my opinion), this z == default will always evaluate to false since it contains the default value of Guid not Guid?

By the way, it seems that this behavior is consistent with all T? or Nullable<T> where T is a value type.

Thanks.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jan 8, 2019

This is all as per the C# language specification. Importantly, changing any of this would be a breaking change in behavior. If this is something you don't like, you can always write a Roslyn analyzer to check your code and identify these places for you.

@Grauenwolf
Copy link

Grauenwolf commented Jan 9, 2019

Implicitly, you mean here the default for z not y.

Let's pretend that is true. Here is an example of why it would be a bad idea.

If you extract the expression as a variable, then the result of that expression would change:

Guid y = Guid.NewGuid();
var temp = (y == default) ? y : default; //this line doesn't know about z
Guid? z = temp;

There is also the issue of var itself. If you use Guid? z to determine the type of default, then you must also use var temp to determine the type of default in my example. But wait, the type of default determines the type of var temp, so the question is undecidable.

Furthermore, what about implicit conversions.

Foo z = (y == default) ? y : default;

Since there is an implicit conversion from Guid to Foo, does that mean default is of type Foo?

@ghost
Copy link
Author

ghost commented Jan 9, 2019

@Grauenwolf

this line doesn’t know about z

I think you donot get the idea here. Before just saying default, it was default(Guid) or maybe default(Guid?) which both are valid. That is why using former syntax is better because it does what it says and reads.

Moreover, the line you put is ambiguous. Even to you, it is ambigious. Read your words, you will see what am saying.

Cherry on top, this is exclusively for value types.

It is bad for readability (God forbid somebody read the code and didnot notice that you using a value type) and it is not clear at all when it is a value type.

@Grauenwolf
Copy link

Personally I think you should be using null or .Empty for comparisons in the first place. default is mostly for generics and default properties.

Why does more characters than necessary?

@Grauenwolf
Copy link

Grauenwolf commented Jan 9, 2019

Also, this is a intentionally bad example.

Guid? z = (y == default) ? y : default;

Reduces to

Guid? z = (y == default) ? default : default;

And that reduces to

Guid? z = default(Guid);

And now the code isn't confusing at all.

@ghost
Copy link
Author

ghost commented Jan 9, 2019

@CyrusNajmabadi here are a couple of points for you:

  1. Compilers should not choose for programers, instead, there should be an error saying that this statement/expression is ambigious. This is not something new or I am inventing. It is the current state in all languages I have seen.
  2. Using default over default(...) will hurt the readability of C#. Because it is insane that everytime I find somebody using default, i have to trace the type to see whether it is a value or reference type because the behavior is different.
  3. Let’s say, that what I am saying is true, would like to push this as a change to C# lang minor version and do a Rosylin Analyser to tell people about the breaking change. I think you will find this unreasonable, at least, it is for me.

@yaakov-h
Copy link
Member

yaakov-h commented Jan 9, 2019

Guid? z = (y == default) ? y : default;

Reduces to

Guid? z = (y == default) ? default : default;

This seems misleading.

 Guid? z = (y == default) ? y : default;

This is effectively, assuming y is still a Guid:

 Guid? z = (y == default(typeof(y))) ? y : default(target-type);

Which is then:

 Guid? z = (y == default(Guid)) ? y : default(target-type);

This can then be simplified to:

 Guid? z = (y == default(Guid)) ? default(Guid) : default(target-type);

However, the target type of the entire expression is Guid?, not Guid. The two defaults refer to two different types and should not be confused for one another.

Because it is insane that everytime I find somebody using default, i have to trace the type to see whether it is a value or reference type because the behavior is different.

If you're using Visual Studio, the hover tooltip will tell you the exact type being inferred, in exactly the same manner as var.

@ghost
Copy link
Author

ghost commented Jan 9, 2019

Also, this is a intentionally bad example.

Guid? z = (y == default) ? y : default;

Reduces to

Guid? z = (y == default) ? default : default;

And that reduces to

Guid? z = default(Guid);

And now the code isn't confusing at all.

You are giving a plausible example which I find unrealistic at all. The piece of code I originally posted comming out of an actual program that is in production.

Another point worth mentioning (@CyrusNajmabadi @Grauenwolf), very ironic one, if you wrote default(Guid?), you will find an analyzer telling you to just remove the brackets, which clearly is not right. Also, seems that the programer who did the analyzer did not know that it will behaive differently with nullable value types.

@ghost
Copy link
Author

ghost commented Jan 9, 2019

@yaakov-h Visual Studio is not a part of the C# language. It is an IDE, simply, an text editor tool. You can not just say, well, look at the tooltip!

@CyrusNajmabadi
Copy link
Member

Compilers should not choose for programers, instead, there should be an error saying that this statement/expression is ambigious.

It's not ambiguous. The language defines exactly what it means. And, as i mentioned above, changing this would be a breaking change. it will not happen.

If you do not like this, you can write your own analyzer that prevents you from writing this.

@brandondahler
Copy link

brandondahler commented Jan 9, 2019

The issue you have has nothing explicitly to do with value types but instead with implicit type casting, order of operations, and null semantics.

@Grauenwolf hit on this, but I wanted to expand on what exactly is going on in the line that you wrote that causes it to assume that default should be a Guid in different parts.

Given the code below:

Guid y = Guid.NewGuid();
Guid? z = (y == default) ? y : default;

The z assignment line is broken up into a couple of different sub-expressions. These expressions are:

  • Expr1: (y == default) which is a bool expression
  • Expr2: Expr1 ? y : default which is a Guid expression
  • Expr3: Guid? z = Expr2 with is a void expression

Expr1 is a bool because it is an equality type. default in this case implicitly determines that it is supposed to be a Guid because y is of type Guid.

Expr2 is a Guid because of how the ternary conditional operator determines its type. The ternary conditional operator will first assume the type of the true (first) value, it then checks the type of the false (second) value to ensure it is equal to or implicitly convertible to the first type. If the second type is not convertible to the first type, it'll try to implicitly convert the first type to the second type. If it cannot make both the first and second type equal, a compilation error will occur. In this particular case the only type that default could implicitly take is Guid as there are no other types available in the expression to assume.

Expr3 is an assignment operator that implicitly converts the type of Expr2 to the defined type -- Guid?. In this particular case, conversion from Guid to Guid? results in the Nullable type wrapping the underlying type.


If we were to rewrite your code out using explicit casts where the implicit casts are occuring would look like this:

Guid y = Guid.NewGuid();
Guid? z = (Guid?) ((y == default(Guid)) ? y : default(Guid)));

Based on all of this, it is my opinion that this isn't an issue with the language itself. This type of ambiguity is inherent with implicit conversions and is the reason why you must be aware of what you're writing when doing implicit casting.

Stylistically, I think it is better to do one of these two:

Guid y = Guid.NewGuid();

Guid? z = default;

if (y == default)
   z = y;

or (given that I like to use anonymous blocks to group and set aside complex variable initialization)

Guid y = Guid.NewGuid();

Guid? z;
{
    if (y == default)
        z = y;
    else
        z = default;
}

@CyrusNajmabadi
Copy link
Member

Using default over default(...) will hurt the readability of C#. Because it is insane that everytime I find somebody using default, i have to trace the type to see whether it is a value or reference type because the behavior is different.

Yup. And any time any sort of inference/whatever is used, you need to go figure out what it means. This is not new to this feature. This is inherent to C#, and has been since 3.0.

Let’s say, that what I am saying is true, would like to push this as a change to C# lang minor version

You can't. It's a breaking change it will not happen.

and do a Rosylin Analyser to tell people about the breaking change. I think you will find this unreasonable, at least, it is for me.

This is not your call. You've raised a concern. But it's how the language works, and is certainly in use by code out there. The behavior cannot be changed. If you don't like this behavior, you are welcome to write your own analyzer that prevents you from using this language feature. But the language will not be changed to have this meaning now be different from one version of the language to another.

@CyrusNajmabadi
Copy link
Member

@yaakov-h Visual Studio is not a part of the C# language. It is an IDE, simply, an text editor tool. You can not just say, well, look at the tooltip!

Yes you can. Because that's how it works for so much of the language. You can't tell just from looking at text what (x, y, z) => ... means. And you can't tell what var means in many cases. The language has lots of features that allow you to write less, but which might make things harder to understand if you don't have tooling.

That's the direction the language decided to go in. If you don't like that, there are simple solutions. Just don't use those features, and/or create analyzers to prevent their use in your codebase.

@CyrusNajmabadi
Copy link
Member

Another point worth mentioning (@CyrusNajmabadi @Grauenwolf), very ironic one, if you wrote default(Guid?), you will find an analyzer telling you to just remove the brackets, which clearly is not right.

That is not true. The IDE understands this:

image

However, if you write:

image

Then the IDE correctly determines that this can be simplified to:

image

@CyrusNajmabadi
Copy link
Member

@SherifRefaat I would recommend taking the discussion about breaking changes to gitter.im/dotnet/csharplang. It's going to be a non-starter here.

@ghost
Copy link
Author

ghost commented Jan 9, 2019

@CyrusNajmabadi Okay. Thanks for your time.

@ghost ghost closed this as completed Jan 9, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants