Open 333fred opened 4 years ago
[Fact]
public void LambdaReturnSpeculation()
{
var comp = CreateCompilation(@"
class C
{
public static implicit operator C(string s) => null!;
C M()
{
string s = """";
System.Func<string> local = () =>
{
s.ToString();
return s;
};
local();
return null!;
}
}");
comp.VerifyDiagnostics(
);
var tree = comp.SyntaxTrees[0];
var model = comp.GetSemanticModel(tree);
var localFunctionBody = tree.GetRoot().DescendantNodes().OfType<LambdaExpressionSyntax>().Single();
var typeInfo = model.GetTypeInfo(localFunctionBody.DescendantNodes().OfType<ReturnStatementSyntax>().Single().Expression!);
Assert.Equal("System.String", typeInfo.ConvertedType.ToTestDisplayString(includeNonNullable: false));
var @return = (ReturnStatementSyntax)SyntaxFactory.ParseStatement("return s;");
Assert.True(model.TryGetSpeculativeSemanticModel(localFunctionBody.Block!.Statements[0].SpanStart + 1, @return, out var specModel));
typeInfo = specModel!.GetTypeInfo(@return.Expression!);
Assert.Equal("C", typeInfo.ConvertedType.ToTestDisplayString(includeNonNullable: false));
}
This is completely unrelated to nullable analysis: when we speculatively bind we are using the wrong symbol.
Alright, I clarified the historical context here with @mavasani. This is the expected behavior of the API: if you want to speculate in a nested method (lambda or local function), you have to replace it.
That being said, I'm not a particular fan of the behavior. It certainly isn't what I expected, and it's inconsistent with how GetSpeculativeTypeInfo
works today. @jasonmalinowski @CyrusNajmabadi, I know the IDE has some code that expects the local function/lambda speculation to work the way it does today, and actually replaces the local function/lambda entirely when doing speculation. I think most of that code would continue working the same as it does today, given that it specifically walks out of the local function scope to compensate for this behavior, but I'm curious if there's any immediate red flags that pop up for you if we were to change it.
My main red flag would be what about other callers that aren't us? Would this break analyzers that are out in the wild?
Would this break analyzers that are out in the wild?
So, I think that falls into two categories:
Yeah, I believe this is just an oversight/bug in the original implementation, and is likely to not break existing clients, though hard to guarantee that.
This test should pass:
The final assert currently fails because we don't enter a new scope in the walker when entering the local function, so when it's restored for speculative analysis we use the wrong containing symbol. Nullable analysis then reinfers the conversion on the return type, which ends up resulting in the speculative model thinking that the converted type of the return is actually
C!
, as if it was converted to the outer scope.