-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Added initial support for explicit tuple conversions #12068
Conversation
@dotnet/roslyn-compiler - please take a look |
@@ -7,6 +7,7 @@ | |||
using Microsoft.CodeAnalysis.CSharp.Syntax; | |||
using Microsoft.CodeAnalysis.Text; | |||
using Roslyn.Utilities; | |||
using System.Linq; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIll remove that. It is unnecessary.
LGTM aside from concern about |
@dotnet-bot test this please |
@@ -53,7 +54,10 @@ public struct Conversion : IEquatable<Conversion> | |||
private const byte IsExtensionMethodMask = 1 << 0; | |||
private const byte IsArrayIndexMask = 1 << 1; | |||
|
|||
private readonly Conversion[] _nestedConversionsOpt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could not make this ImmutableArray because of a false-circularity bug in CLR loader.
The shape of this type is likely to change as a part of #12067
@@ -1,12 +1,10 @@ | |||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | |||
|
|||
using Microsoft.CodeAnalysis.CSharp.Symbols; | |||
using Roslyn.Utilities; | |||
using System; | |||
using System.Collections.Immutable; | |||
using System.Diagnostics; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the system ones are supposed to go at the top. ¯\_(ツ)_/¯
@dotnet-bot test perf_correctness_prtest please |
{ | ||
return true; | ||
return Conversion.ExplicitNullable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return an implicit conversion in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should report ExplicitNullable. Both implicit and explicit conversions exist in this case and you get one or another depending on what you are classifying.
The caller used to report ExplicitNullable in all preexisting cases here even before this change.
By itself supporting explicit tuple conversions is fairly trivial. We just apply explicit conversions element-wise. Similar to implicit tuple conversions.
The complicated part is interaction between implicit and explicit conversions. In particular C# permits cases where T converts to U via distinct implicit and explicit conversions. In such and cases a cast in code can act as disambiguator.
In this change we are trying to generalize this rule to tuple conversions. (yet to be confirmed with LDM).
UNDONE in this change:
tracked by Implement ExplicitTupleLiteralConversion #11804
This could be a big but fairly mechanical change, not affecting any functionality (hopefully) so better be done separately.
As a part of this change it could make sense to make Conversion type more compact, possibly even make it a reference type.
Tracked by Consider storing Conversion on bound conversion nodes and use at lowering. #12067
Some similar helper are separated between two files, some helpers for conversions from expressions and types and those that handle casts in code are not named uniformly. It is possible there is some duplication.
Makes sense to bundle with Consider storing Conversion on bound conversion nodes and use at lowering. #12067
Especially once Propagate preferences between explicit and implicit conversions into nullable conversions #12064 and Implement ExplicitTupleLiteralConversion #11804 are resolved and it is confirmed that the whole deal with distributing "Cast In Code" stays.