Initial support for Unions conversions #81586
Conversation
dotnet-policy-service
bot
added
the
Needs API Review
|
This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging. |
|
@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review |
| object System.Runtime.CompilerServices.IUnion.Value => throw null; | ||
| public static explicit operator S1(int x) | ||
| { |
333fred
left a comment
333fred
left a comment
There was a problem hiding this comment.
Only a couple of small comments.
src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversion.cs
Show resolved
Hide resolved
| Microsoft.CodeAnalysis.CSharp.ForEachStatementInfo.MoveNextAwaitableInfo.get -> Microsoft.CodeAnalysis.CSharp.AwaitExpressionInfo | ||
| static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetAwaitExpressionInfo(this Microsoft.CodeAnalysis.SemanticModel? semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.LocalDeclarationStatementSyntax! awaitUsingDeclaration) -> Microsoft.CodeAnalysis.CSharp.AwaitExpressionInfo | ||
| static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetAwaitExpressionInfo(this Microsoft.CodeAnalysis.SemanticModel? semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.UsingStatementSyntax! awaitUsingStatement) -> Microsoft.CodeAnalysis.CSharp.AwaitExpressionInfo | ||
|
|
There was a problem hiding this comment.
The empty line is intentional. It makes it easy to deal with merges between branches
| // PROTOTYPE: The errors are coming from an attempt to classify Union conversion during overload resolution. | ||
| // Is there a relatively easy way to avoid reporting them? Perhaps that can be done if we limit Union | ||
| // types to structs only. This way we can expect all interfaces to be explicitly listed on the struct. | ||
| // IUnion cannot be implemented indirectly, i.e. by listing only a derived interface, or inheriting | ||
| // from a base implementing it. Therefore, we could rely on InterfacesNoUseSiteDiagnostics instead of | ||
| // AllInterfacesWithDefinitionUseSiteDiagnostics to check whether IUnion is among implemented interfaces. |
There was a problem hiding this comment.
My 2cents is that these won't be common enough to warrant concern about in general. There may be other good reasons to limit, but avoiding these diagnostics isn't one of them. #Resolved
There was a problem hiding this comment.
My 2cents is that these won't be common enough to warrant concern about in general.
It is not clear what exactly you find uncommon. I think this kind of break to existing code might be common, because we will start reporting errors about interfaces that we never were interested in before.
There was a problem hiding this comment.
I wouldn't assume that the break will be limited to list pattern scenarios. This is just an example of an impact from examining interfaces in scenarios where we didn't do that before.
There was a problem hiding this comment.
Specifically, the additional missing assemblies errors. It's certainly possible that there is more direct fallout that has bad behavior, but the additional errors, specifically, are not concerning to me.
There was a problem hiding this comment.
but the additional errors, specifically, are not concerning to me.
I do not consider additional errors as breaking. In this specific scenario, for example, they are the only errors. There were no errors before we started checking for Union conversions.
|
@RikkiGibson, @dotnet/roslyn-compiler For a second review |
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM, comments are not blocking.
| return Conversion.NoConversion; | ||
| } | ||
|
|
||
| // SPEC: Find the set of applicable constructors |
There was a problem hiding this comment.
|
|
||
| // Don't need to worry about checked user-defined operators | ||
| Debug.Assert(!conversion.IsUserDefined || result == ConstantValue.False || result == ConstantValue.Bad); | ||
| Debug.Assert((!conversion.IsUserDefined && !conversion.IsUnion) || result == ConstantValue.False || result == ConstantValue.Bad); |
There was a problem hiding this comment.
These are checked together a lot, it might be helpful to declare property IsUserDefinedOrUnion to use in these places
3 participants
No description provided.