Infer type in get accessor declarations, expression bodied local functions and local functions inside lambda expressions

[rik-smeets]

Customer scenario

Customer generates a variable or method from within:

• an expression bodied getter
• an expression bodied local function/
• a block bodied location function inside a lambda expression

and notices type inferences does not work. See the bug below for a code example, or refer to the newly added unit tests.

Bugs this fixes

#26993

Workarounds, if any

Manually fix the type of the generated variable / the return type of the generated method.

Risk

Risk is limited: this fix is basically an extension to the way similar scenarios (like expression bodies properties) are handled in the same file.

Performance impact

Low, there's been some minor changes when inferring types in arrow expression clauses and return statements.

Is this a regression from a previous update?

Not that I know of.

Root cause analysis

These situations were not covered by unit tests yet. I added these tests now, including some extra unit tests validating situations which were already working as expected to prevent possible regressions in the future.

How was the bug found?

Customer reported, see #26993.

Test documentation updated?

No impact on test documentation.

[CyrusNajmabadi]

could you add a test with a getter with different accessibility? Thanks!

[rik-smeets]

I added a test where the getter has a different accessibility.

[CyrusNajmabadi]

nit: consider rewriting all three of these 'ifs' as a switch.
nit: just use pattern matching for all three checks.

[rik-smeets]

I updated it to use pattern matching, thanks for the suggestion!

[Neme12]

since you know this has to be an accessor, use a cast

[Neme12]

or pattern matching - this should work for any accessor, not just a getter

[rik-smeets]

Pattern matching is now used, both for getters and setters.

[Neme12]

this method is same as above, consider removing duplication

[rik-smeets]

I extracted the duplication to a separate method.

[CyrusNajmabadi]

We should also handle "SetAccessors" (and probably add/remove as well). In these cases, the inferred type would be 'void'. You should be able to validate that by doing something like this: class C { int P { set => Foo(value); } } and generating a method for 'Foo'. We'd want it to infer 'void' as the return type. Thanks!

[Neme12]

I think that should work if we handle them all here:
if (arrowClause.Parent is AccessorDeclarationSyntax accessor)

[rik-smeets]

I now added some tests in GenerateMethodTests.cs as well for both the getter and the setter situation. It indeed worked as expected after applying pattern matching.

[Neme12]

is there a case for local functions somewhere?

[Neme12]

I just checked and they always generate object, both with expression body or block with a return statement. Please fix that too.

[Neme12]

note: the issue number is wrong

Customer reported, see #26933.

[Neme12]

nit: consider using a switch statement

[rik-smeets]

How can I make this work inside a switch statement?

[Neme12]

switch (arrowClause.Parent)
{
    case PropertyDeclarationSyntax propertyDeclaration:
        return InferTypeInPropertyDeclaration(propertyDeclaration);
    case BaseMethodDeclarationSyntax baseMethodDeclaration:
        return InferTypeInBaseMethodDeclaration(baseMethodDeclaration);
    case AccessorDeclarationSyntax accessorDeclaration:
        return InferTypeInAccessorDeclaration(accessorDeclaration);
    default:
        return SpecializedCollections.EmptyEnumerable<TypeInferenceInfo>();
}

[rik-smeets]

Thanks! Didn't know that was possible too, nice to learn something new :). I've changed it, running tests now, after that I will push to server again.

[Neme12]

Yes, pattern matching is nice isn't it 😄 Glad you learned something new. You can use a pattern in an is expression or a switch case, regardless of what kind of pattern it is. In the future, this should be even shorter when we get the switch expression.

[Neme12]

My idea was actually to replace both InferTypeInBaseMethodDeclaration and InferTypeInAccessorDeclaration with one method taking a SyntaxNode. I don't know what it would be called though. InferTypeInMethodLikeDeclaration? MethodSymbolDeclaration?

[Neme12]

Actually, the best approach might be to call GetDeclaredSymbol regardless of what we found (eliminating the switch statement) and then check if the symbol is a method symbol or a property symbol and get its type either way.

[rik-smeets]

I tried that, but I can't get it more readable / with less code that way. Getting the method symbol requires either the BaseMethodDeclarationSyntax or AccessorDeclarationSyntax to be passed to GetDeclaredSymbol. They don't share a base class. If you have a good idea, please let me know.

[Neme12]

ok, I thought there was an overload that takes a SyntaxNode

[rik-smeets]

There is, and it returns an ISymbol. But I don't know of a way to get the (necessary) method symbol based on that alone… If someone does know, I'd love to hear it.

[Neme12]

You'd have to check whether the symbol is an IMethodSymbol or an IPropertySymbol and then get its type. The upside is that you'd handle both methods, accessors and properties this way and could get rid of all 3 methods and the switch statement.

[Neme12]

Actually, you could just use this extension method:

roslyn/src/Workspaces/Core/Portable/Shared/Extensions/ISymbolExtensions.cs

Line 843 in 84e1af7

public static ITypeSymbol GetSymbolType(this ISymbol symbol)

[Neme12]

Wait, no... it doesn't handle methods.

[rik-smeets]

Of course, pattern matching to the rescue again! See my newest commit, hope this is to your liking.

[Neme12]

I would just extract a method (or local function if you like) to get the type from the symbol:

private static ITypeSymbol GetSymbolReturnType(ISymbol symbol)
{
    switch (symbol)
    {
        case IPropertySymbol property: return property.Type;
        case IMethodSymbol method: return method.ReturnType;
        default: return null;
    }
}

which you could use and then only do the type != null check once here and avoid all the duplication.

[Neme12]

but you don't have to listen to me anymore 😄 i'm just giving suggestions

[Neme12]

Note that this last change should fix local functions too, but we still need a test for that.

[rik-smeets]

Well it's another good suggestion, so I did that. I also added unit tests for local functions, where type inference now also works as expected.

[Neme12]

nit: ?. is unnecessary, if parentSymbol is null, none of the patterns will match

[Neme12]

we should probably pass in CancellationToken

[Neme12]

I think this isn't fixed. If you change the return type of Y to void, this test will fail. It's still working incorrectly by taking the return type of the containing method, not the local function. You only fixed expression bodies. Block bodied local functions are still working incorrectly.

[Neme12]

Just try this simple case:

class C
{
    void M()
    {
        int F()
        {
            return y;
        }
    }
}

the generated field will be of type object.

[Neme12]

Also, I think we shouldn't use Foo as a name for some reason, but I can't tell you why.

[rik-smeets]

You're right regarding the block bodied types. I think I'll include that too in this pull request, might take me up to a week though because I'm busy for a while. I'll tag you once it's done.

[tmat]

1 [](start = 7, length = 1)

I know other tests already do this, but it would be nicer to define a couple of constants for these indices. It's not immediately clear what these mean.

[tmat]

Since this function is only called once I'd prefer writign it more directly:

ISymbol symbolReturnType;
switch (parentSymbol)
 {
       case IPropertySymbol property: 
          symbolReturnType = property.Type;
          break;
       case IMethodSymbol method: 
          symbolReturnType = method.ReturnType;
          break;
       default: 
          return null;
}

[Neme12]

Sorry, it was my suggestion to use a function... I think it's more concise, readable and avoids duplication (of the assignment).

[Neme12]

It also separates the logic, which I think is nice regardless of how many times it is called.

[Neme12]

If this was a method it could be reused by the code that deals with block bodies and return statements. There still has to be some modification to that because of the bug with local functions. Reusing this might be a nice solution.

[tmat]

Making it a method would be fine. As it is it it actually does not separate the logic and can't be reused since it's using parentSymbol directly.

[Neme12]

Yes I would prefer either a method or non-capturing local function instead personally

[jcouv]

Please use another term than "Foo" in all the added tests. Thanks
Method(), local() or property would be fine.

[rik-smeets]

Using property is not possible unfortunately, no code actions are available then. Which might actually be a bug, since it's not a reserved keyword as far as I know. Creating the property manually with the identifier property is just fine. Do you agree?

I'll just call it prop for now (and I'm working on all the other review comments now as well, I hope to finish it later today!).

[Neme12]

Just call it P or whatever, it doesn't matter at all. It's not that Foo is discouraged because it's not descriptive. It's just a bad word for some reason and doesn't pass some kind of MS internal check.

[rik-smeets]

I know, however, since we now have the opportunity to make the identifiers more descriptive as well, why not? I already did @jcouv's other rename suggestions too.

[Neme12]

I don't think this is correct. If you have a local function inside a lambda, the previous checks will win (find the outer lambda) and this code will never reach. What we really want is the first ancestor that is either a lambda or a local function.

[rik-smeets]

I'll create and add some tests, which should fail if your analysis is correct (good thinking either way!). I'll get to it later today.

[Neme12]

nit: static

[rik-smeets]

Turns out there already is a private method for this purpose (and yes, that one IS static :-)). To prevent duplication, I'll use that one.

[Neme12]

you can simplify this to e is AnonymousFunctionExpressionSyntax|| e is LocalFunctionStatementSyntax

[Neme12]

ParenthesizedLambdaExpressionSyntax and SimpleLambdaExpressionSyntax derive from LambdaExpressionSyntax which together with AnonymousMethodExpressionSyntax derive from AnonymousFunctionExpressionSyntax

[rik-smeets]

Good one, I have updated it.

[Neme12]

note: SimpleLambdaExpressionSyntax can be async too, so the fact that isAsync = ... isn't here must be a bug (it probably doesn't unwrap the return type from Task)

[Neme12]

you can remove this else if and handle both above if you check for is LambdaExpressionSyntax instead

[rik-smeets]

I fixed this too. Also did some minor improvements later in the method (pattern matching once more, yay!).

[Neme12]

This could likely be simplified too, because lambdas and anon. methods share all this info in AnonymousFunctionExpressionSyntax, but meh... this whole mess needs a rewrite 😄 you don't need to do that

[rik-smeets]

I agree, I'll leave that to you ;).

[Neme12]

Not related to your change but I'd be really interested to know how we can return into a field.

[tmat]

test windows_release_vs-integration_prtes please

[jcouv]

LGTM Thanks

[jinujoseph]

Approved to merge for 15.8.Preview3

[Neme12]

@rik-smeets

Using property is not possible unfortunately, no code actions are available then. Which might actually be a bug, since it's not a reserved keyword as far as I know.

Did you open an issue for that? If not, please do.

[Neme12]

Customer generates a variable or method from within:

an expression bodied getter
an expression bodied local function/
a block bodied location function inside a lambda expression

Note: you actually fixed even more cases than that.
For example generating from an anonymous method inside a lambda
or generating from an async simple lambda - it didn't use to unwrap the task, now it does (in the code it looked like someone thought they cannot be async)

One thing that still isn't fixed though is unwrapping the Task from an expression bodied async method/function:

using System.Threading.Tasks;

class C
{
    async Task<int> M1()
    {
        return foo; // correctly generates int
    }

    async Task<int> M2() => foo; // generates Task<int>
}

Please create an issue for that too. Thanks.

[Neme12]

I filed both issues: #27646 #27647

[rik-smeets]

Thanks for doing that @Neme12, I didn't have time for it lately.