Fix go to definition for conversions on invalid constructor overloads#78514
Fix go to definition for conversions on invalid constructor overloads#78514333fred merged 11 commits intodotnet:mainfrom
Conversation
|
We need to have direct tests for SemanticModel API, plus an explanation what was the old behavior, why it was wrong, etc. #Closed |
|
@AlekseyTs this is ready for review |
Was the explanation provided? Basically we need a clear description of the problem with the SemanticModel API. The reason why you think the present behavior is wrong, a description of the expected behavior with a reason behind the expectation. #Closed |
I can reword it if you feel like it doesn't provide enough information |
|
I think this isn't quite what I was looking for. I was looking for a description of your intent in this PR and the reason behind the intent. Something like: "I intend to change behavior *** API for the scenarios ***. The current behavior of the API is ***. I think it should be *** instead, because ***." Obviously your ultimate goal is to get some IDE scenarios working, but simply stating that is not sufficient for changing how compiler API behaves. In order for the Compiler team to review the PR, we need the intent stated in "Compiler API" terms. #Closed |
|
@AlekseyTs updated the description and brought the PR up to date, ptal |
| var overloadResolutionSuffices = resultKind == LookupResultKind.OverloadResolutionFailure | ||
| && boundExpr.Kind is BoundKind.BadExpression | ||
| && highestBoundNode is BoundConversion { Conversion.Method: not null }; | ||
| if (!overloadResolutionSuffices && highestBoundNode is BoundExpression highestBoundExpr) |
There was a problem hiding this comment.
if (!overloadResolutionSuffices && highestBoundNode is BoundExpression highestBoundExpr)
The original if statement looked like a "voodoo magic" and with this adjustment it looks even more "magical".
I was able to clarify/simplify it to the following:
if (highestBoundNode is BoundBadExpression badExpression)
{
// Downgrade result kind if highest node is BoundBadExpression
LookupResultKind highestResultKind;
bool highestIsDynamic;
ImmutableArray<Symbol> unusedHighestMemberGroup;
OneOrMany<Symbol> highestSymbols = GetSemanticSymbols(
badExpression, boundNodeForSyntacticParent, binderOpt, options, out highestIsDynamic, out highestResultKind, out unusedHighestMemberGroup);
if (highestResultKind != LookupResultKind.Empty && highestResultKind < resultKind)
{
resultKind = highestResultKind;
isDynamic = highestIsDynamic;
}
}
else if (boundExpr is BoundMethodGroup &&
resultKind == LookupResultKind.OverloadResolutionFailure &&
highestBoundNode is BoundConversion { ConversionKind: ConversionKind.MethodGroup } boundConversion)
{
LookupResultKind highestResultKind;
bool highestIsDynamic;
ImmutableArray<Symbol> unusedHighestMemberGroup;
OneOrMany<Symbol> highestSymbols = GetSemanticSymbols(
boundConversion, boundNodeForSyntacticParent, binderOpt, options, out highestIsDynamic, out highestResultKind, out unusedHighestMemberGroup);
if (highestSymbols.Count > 0)
{
symbols = highestSymbols;
resultKind = highestResultKind;
isDynamic = highestIsDynamic;
}
}
With this code all tests but two are passing, and the two failing tests are:
- GetSymbolInfo_ImplicitUserDefinedConversionOnMethodGroup_InLocalDeclaration
- GetSymbolInfo_ImplicitUserDefinedConversionOnMethodGroup_InAssignment
According to #75833, the failures is a desired behavior change. Though, #75833 is not completely fixed. #Closed
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetSemanticInfoTests.cs
Show resolved
Hide resolved
|
Done with review pass (commit 8) #Closed |
Co-Authored-By: AlekseyTs <AlekseyTs@users.noreply.github.com>
|
@AlekseyTs resolved your comments, could you ptal again? |
|
@dotnet/roslyn-compiler For a second review for a community PR. |
2 similar comments
|
@dotnet/roslyn-compiler For a second review for a community PR. |
|
@dotnet/roslyn-compiler For a second review for a community PR. |
Closes #73498
Closes #77545
Fixes one part of #75833, the appropriate tests were adjusted to reflect the new behavior
With this PR, the
GetSymbolInfoAPI and all the corresponding APIs using it will be affected for the scenarios where an object creation expression used a constructor overload that doesn't exist, whilst the expression was being implicitly converted to another type. The previous behavior would resolve to the target type of the conversion, since the binding result would be an overload resolution, whereas now it has been changed to resolve to the existing overload resolution result.The goal of this change is also reflected in GTD which will now correctly navigate to the type whose constructor was not found, instead of the target conversion type, or the implicit conversion method.
This also adds tests for explicit conversions on object creation expressions, covering the above scenaria, as well as testing the existing correct behaviors of when there is no conversion, or when there is no overload resolution failure.