-
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
Pick well-known types from corlib over other references #51571
Conversation
Does that mean roslyn/src/Compilers/CSharp/Portable/CSharpResources.resx Lines 1568 to 1570 in 64d1bee
Note: I cannot find tests for it. #Resolved |
/// This method handles duplicate types in a few different ways: | ||
/// - for types before C# 7, the first candidate is returned with a warning | ||
/// - for types after C# 7, the type is considered missing |
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.
nit: the comment mentions "before C# 7" and "after C# 7". It's not clear which of these meant to be including C# 7 itself.
You still get ambiguities when two libraries have the well-known type, but corlib doesn't. Example:
|
@@ -858,11 +866,31 @@ private NamedTypeSymbol ApplyGenericArguments(NamedTypeSymbol symbol, Type[] typ | |||
continue; | |||
} | |||
} | |||
else if (isWellKnownType && warnings is null) |
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.
isWellKnownType && warnings is null [](start = 29, length = 35)
Is this how we detect "well-known types after C# 7"? Consider caching this value in a dedicated local before the loop, for clarity. #Closed
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 is not clear to me why isWellKnownType
is required, it feels like the adjusted rules should apply for a case of a not well-known type.
In reply to: 585695313 [](ancestors = 585695313)
{ | ||
conflicts = (result.ContainingAssembly, candidate.ContainingAssembly); | ||
result = null; | ||
if (isWellKnownType) | ||
{ | ||
onlyConsiderCorLib = true; |
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.
onlyConsiderCorLib = true; [](start = 28, length = 26)
It feels like this would conflict with ignoreCorLibraryDuplicatedTypes
flag when it is true. #Closed
@@ -818,10 +820,16 @@ private NamedTypeSymbol ApplyGenericArguments(NamedTypeSymbol symbol, Type[] typ | |||
} | |||
|
|||
// Lookup in references | |||
bool onlyConsiderCorLib = false; |
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.
bool onlyConsiderCorLib = false; [](start = 12, length = 32)
It feels like the implementation could be simpler and, perhaps, faster for scenarios when the type is indeed defined in core library. I would expect it roughly structured this way:
If we give priority to core library among other references and there are references (i.e. we are definitely not inside a core library, otherwise we already performed the lookup above), look in the CorLibrary
. If found, that is our result. If not found, continue with lookup in all references, skipping the CorLibrary
as an optimization.
It is quite possible I am missing some some detail and the suggested structure wouldn't work for some specific scenario. That is why, I think, we need to state the lookup rules in words here, clearly and precisely explaining the lookup rules and what gets priority and when. Also, we should clearly explain how ignoreCorLibraryDuplicatedTypes
is supposed to affect the lookup, because it appears to be in conflict with a general rule. #Closed
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.
In the process, it might be worth checking if CorLibrary
is missing.
In reply to: 585718223 [](ancestors = 585718223)
The title of the PR feels misleading. It doesn't affect just IsExternalInit type, it has much wider effect. When merging this PR, consider reflecting that in the commit title. |
Dim candidate As NamedTypeSymbol = references(i).LookupTopLevelMetadataType(metadataName, digThroughForwardedTypes:=False) | ||
|
||
Dim reference = references(i) | ||
If onlyConsiderCorLib And reference IsNot CorLibrary Then |
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.
And [](start = 42, length = 3)
AndAlso? #Closed
@@ -563,10 +570,27 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols | |||
result = candidate | |||
Continue For | |||
End If | |||
ElseIf isWellKnownType Then |
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.
isWellKnownType [](start = 35, length = 15)
Similar to C#. Why have this condition? #Closed
if (IsInCorLib(result)) | ||
{ | ||
// ignore candidate | ||
continue; |
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.
continue; [](start = 28, length = 9)
Is there a good reason to continue? #Closed
{ | ||
// drop previous result | ||
result = candidate; | ||
continue; |
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.
continue; [](start = 28, length = 9)
Is there a good reason to continue? #Closed
' For well-known types, we prefer corlib (unless ignoreCorLibraryDuplicatedTypes is set) | ||
If IsInCorLib(result) Then | ||
' ignore candidate | ||
Continue For |
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.
Continue For [](start = 36, length = 12)
Is there a good reason to continue? #Closed
If IsInCorLib(candidate) Then | ||
' drop previous result | ||
result = candidate | ||
Continue For |
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.
Continue For [](start = 36, length = 12)
Is there a good reason to continue? #Closed
Return Nothing | ||
result = Nothing | ||
If isWellKnownType Then | ||
onlyConsiderCorLib = True |
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.
onlyConsiderCorLib = True [](start = 32, length = 25)
Same comment as for C# #Closed
@@ -534,13 +534,20 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols | |||
|
|||
result = Nothing | |||
|
|||
Dim onlyConsiderCorLib As Boolean = False |
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.
Dim onlyConsiderCorLib As Boolean = False [](start = 12, length = 41)
Same suggestion for implementation strategy as for C# #Closed
// (4,12): error CS0518: Predefined type 'System.Runtime.InteropServices.InAttribute' is not defined or imported | ||
// public ref readonly int M() => throw null; | ||
Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "ref readonly int").WithArguments("System.Runtime.InteropServices.InAttribute").WithLocation(4, 12)); | ||
var comp = CreateCompilation(user, references: new[] { ref1, ref2 }).VerifyDiagnostics(); |
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.
CreateCompilation [](start = 23, length = 17)
Perhaps we should also keep the flavor of the test with ambiguity, i.e. by targeting core library without the type. #Closed
@@ -24810,7 +24810,9 @@ public struct ValueTuple<T1, T2> | |||
var libWithVTRef = libWithVT.EmitToImageReference(); | |||
|
|||
var comp = CSharpCompilation.Create("test", references: new[] { libWithVTRef, corlibWithVTRef }); | |||
Assert.True(comp.GetWellKnownType(WellKnownType.System_ValueTuple_T2).IsErrorType()); | |||
var found = comp.GetWellKnownType(WellKnownType.System_ValueTuple_T2); | |||
Assert.False(found.IsErrorType()); |
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.
Assert.False(found.IsErrorType()); [](start = 12, length = 34)
Do we have a test covering an ambiguity that doesn't involve core library? #Closed
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.
"attribute in two libraries" scenario in GetWellKnownType
test
In reply to: 585742024 [](ancestors = 585742024)
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.
"attribute in two libraries" scenario in GetWellKnownType test
Are you referring to PickAmbiguousAttributeFromCorlib unit-test? It doesn't look like it tests ValueTuple type.
In reply to: 585931231 [](ancestors = 585931231,585742024)
@@ -5,6 +5,7 @@ | |||
#nullable disable | |||
|
|||
using System.Linq; | |||
using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE; |
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.
Is this change necessary? #Closed
// (2,32): error CS0518: Predefined type 'System.Runtime.InteropServices.UnmanagedType' is not defined or imported | ||
// public class Test<T> where T : unmanaged | ||
Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "unmanaged").WithArguments("System.Runtime.InteropServices.UnmanagedType").WithLocation(2, 32)); | ||
CreateCompilation(user, references: new[] { ref1, ref2 }).VerifyDiagnostics(); |
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.
CreateCompilation(user, references: new[] { ref1, ref2 }).VerifyDiagnostics(); [](start = 12, length = 78)
Do we have a test covering an ambiguity that doesn't involve core library? #Closed
.EmitToImageReference(); | ||
|
||
{ | ||
// attribute in source |
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.
attribute [](start = 19, length = 9)
Are we actually dealing with an attribute? #Closed
var comp = CreateEmptyCompilation(new[] { source, IsExternalInitTypeDefinition }, references: new[] { corlibWithoutIsExternalInitRef }); | ||
comp.VerifyEmitDiagnostics(); | ||
var modifier = getUsedModifier(comp); | ||
Assert.True(modifier.Modifier.IsInSource()); |
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.
IsInSource [](start = 46, length = 10)
Relying on IsInSource helper here feels fragile, we are relying on an assumption that all references are metadata references. I think it would be better to verify that the type belongs to the source module of the compilation. #Closed
@@ -21676,7 +21676,9 @@ End Namespace | |||
Dim libWithVTRef = libWithVT.EmitToImageReference() | |||
|
|||
Dim comp = VisualBasicCompilation.Create("test", references:={libWithVTRef, corlibWithVTRef}) | |||
Assert.True(comp.GetWellKnownType(WellKnownType.System_ValueTuple_T2).IsErrorType()) | |||
Dim found = comp.GetWellKnownType(WellKnownType.System_ValueTuple_T2) | |||
Assert.False(found.IsErrorType()) |
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.
Assert.False(found.IsErrorType()) [](start = 12, length = 33)
Do we have a test covering an ambiguity that doesn't involve core library? #Closed
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.
"attribute in two libraries" scenario in GetWellKnownType test
In reply to: 585751950 [](ancestors = 585751950)
@dotnet/roslyn-compiler for a second review. Thanks |
@@ -839,7 +872,7 @@ private NamedTypeSymbol ApplyGenericArguments(NamedTypeSymbol symbol, Type[] typ | |||
continue; | |||
} |
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.
Consider extracting these checks and reusing in the new CorLibrary
case abve. #Resolved
bool ignoreCorLibraryDuplicatedTypes = false) | ||
{ | ||
// Type from source always wins. |
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.
Type from source [](start = 15, length = 16)
Should this be "from this assembly" rather than "from source"? #Resolved
// we prefer the type from corlib | ||
var m = comp.GlobalNamespace.GetTypeMember("Test").GetMethod("M"); | ||
var attribute = m.RefCustomModifiers.Single().Modifier; | ||
Assert.NotNull(attribute.ContainingAssembly.GetTypeByMetadataName("System.Object")); |
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.
Assert.NotNull [](start = 12, length = 14)
What are we verifying? Won't GetTypeByMetadataName()
look though all references to find "System.Object"? #Resolved
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.
This is verifying that the attribute we found came from corlib.
In reply to: 586780243 [](ancestors = 586780243)
@@ -3878,7 +3878,7 @@ class C : I | |||
NamedTypeSymbol iEquatable = compilation.GetWellKnownType(WellKnownType.System_IEquatable_T); | |||
Assert.False(iEquatable.IsErrorType()); | |||
Assert.Equal(1, iEquatable.Arity); | |||
Assert.Null(compilation.GetTypeByMetadataName("System.IEquatable`1")); | |||
Assert.True(iEquatable == (object)compilation.GetTypeByMetadataName("System.IEquatable`1")); |
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.
Assert.True(iEquatable == (object) [](start = 20, length = 34)
Assert.Same(iEquatable, ...);
#Resolved
(Not isWellKnownType OrElse IsValidWellKnownType(corLibCandidate)) AndAlso | ||
IsAcceptableMatchForGetTypeByNameAndArity(corLibCandidate) AndAlso | ||
Not corLibCandidate.IsHiddenByVisualBasicEmbeddedAttribute() AndAlso | ||
Not corLibCandidate.IsHiddenByCodeAnalysisEmbeddedAttribute() Then |
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.
Consider extracting these checks and reusing in loop below. #Resolved
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 is a bit of repetition. I think I still prefer this way though, especially in absence of local functions for VB...
In reply to: 586787930 [](ancestors = 586787930)
End If | ||
End Sub | ||
|
||
Private Sub GetWellKnownType_Verify(comp As VisualBasicCompilation, expectedAssemblyName As String) |
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.
Private [](start = 8, length = 7)
Shared
#Resolved
@@ -574,7 +574,7 @@ End Class | |||
Dim iEquatable As NamedTypeSymbol = compilation.GetWellKnownType(WellKnownType.System_IEquatable_T) | |||
Assert.False(iEquatable.IsErrorType()) | |||
Assert.Equal(1, iEquatable.Arity) | |||
Assert.Null(compilation.GetTypeByMetadataName("System.IEquatable`1")) | |||
Assert.True(iEquatable Is compilation.GetTypeByMetadataName("System.IEquatable`1")) |
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.
Assert.True(iEquatable Is [](start = 12, length = 25)
Assert.Same(iEquatable, ...)
#Resolved
Return result | ||
End Function | ||
|
||
Private Function IsValidCandidate(candidate As NamedTypeSymbol, isWellKnownType As Boolean) As Boolean | ||
|
||
Return candidate IsNot Nothing AndAlso |
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.
candidate IsNot Nothing [](start = 19, length = 23)
Minor: It looks like IsAcceptableMatchForGetTypeByNameAndArity
and IsValidWellKnownType
already check for null reference. #Closed
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.
LGTM (commit 10)
Hmm, I think the rename of |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Sorry, I meant to rename the PR before merging, but forgot to do it :-/ |
Fixes #50696
For well-known types (after LangVer 7 for C# and for all version for VB), conflicts between references is currently treated as an error.
From discussion with Aleksey, Chuck and Jared, this makes it difficult for users to implement their own types for down-level support, as those types will conflict with the BCL types of the new TFM.
This PR leaves the lookup of regular types and the lookup of well-known types before C# 7 unchanged.
For well-known types after C# 7 and in VB, we'll prefer: