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

Added initial support for explicit tuple conversions #12068

Merged
merged 2 commits into from
Jun 18, 2016

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Jun 17, 2016

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.

 T t = new T();
 U u = t;          // implicit cast T->U is used
 U u = (U)t;       // explicit cast T->U is preferred

In this change we are trying to generalize this rule to tuple conversions. (yet to be confirmed with LDM).

 (T, T) tt = (new T(), new T());
 (U, U) uu = tt;                // implicit cast T->U is used
 (U, U) uu = ((U, U))tt         // explicit cast T->U is preferred
 (U, U)? uu = ((U, U)?)tt       // explicit cast T->U is preferred 

UNDONE in this change:

@VSadov
Copy link
Member Author

VSadov commented Jun 17, 2016

@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;
Copy link
Member Author

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.

@agocke
Copy link
Member

agocke commented Jun 17, 2016

LGTM aside from concern about dynamic

@VSadov
Copy link
Member Author

VSadov commented Jun 17, 2016

@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;
Copy link
Member Author

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;
Copy link
Member

@gafter gafter Jun 17, 2016

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. ¯\_(ツ)_/¯

@gafter
Copy link
Member

gafter commented Jun 17, 2016

:shipit:

@VSadov
Copy link
Member Author

VSadov commented Jun 18, 2016

@dotnet-bot test perf_correctness_prtest please
@dotnet-bot test windows_eta_open_prtest please

@VSadov VSadov merged commit ec6ac69 into dotnet:master Jun 18, 2016
{
return true;
return Conversion.ExplicitNullable;
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in some cases below?


In reply to: 67725469 [](ancestors = 67725469)

Copy link
Member Author

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.

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

Successfully merging this pull request may close these issues.

6 participants