Improve circularity analysis in bases for some scenarios.#62799
Improve circularity analysis in bases for some scenarios.#62799333fred merged 2 commits intodotnet:mainfrom
Conversation
|
@dotnet/roslyn-compiler For the second review |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
@dotnet/roslyn-compiler For the second review |
2 similar comments
|
@dotnet/roslyn-compiler For the second review |
|
@dotnet/roslyn-compiler For the second review |
|
|
||
| [Fact] | ||
| [WorkItem(58424, "https://github.com/dotnet/roslyn/issues/58424")] | ||
| public void NestedTypes_53() |
There was a problem hiding this comment.
📝 This comment in the bug was helpful for reviewers to understand the bug:
The error is reported because, while binding
Test's bases, we are bindingIInner's bases as well. Lookup triggers that binding while resolving "IOuter", it tries to look for this name withinIInner's bases, that tries to resolve the "Other" and the error is reported once we try to look it up withinTest, which needsTest's bases and the circle completes.
In the particular scenario we can avoid an attempt to lookup "IOuter" within
IInner's bases because, we first look in theIInneritself (without inheritance). If we found the type there, no reason to look for it inIInner's bases, anything found will be shadowed by the immediate result anyway. BTW, the shadowing check causes a stack overflow, see #62795.
Avoiding the error in a more general case is likely to add more complexity to circularity analysis in bases. Not sure if it is worth it. #Resolved
| { | ||
| b.OriginalDefinition.AddUseSiteInfo(ref useSiteInfo); | ||
|
|
||
| if (TypeSymbol.Equals(b, baseType, TypeCompareKind.ConsiderEverything2)) |
There was a problem hiding this comment.
nit: Can we avoid introducing new usage of ConsiderEverything2?
There was a problem hiding this comment.
Can we avoid introducing new usage of
ConsiderEverything2?
This line of code is a clone of an existing line in the same function. Figuring out appropriate mode of comparison and adding related tests was outside of scope of this PR.
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (iteration 2) with a nit
Fixes #58424.
Fixes #62795.