Roslyn: Code fix suggestion: fix return type

[gerjandev]

I want to propose a code fix for fixing the return type.

Example 1:

Here I would like to press CTRL + . and get the code fix suggestion: Fix return type from void to IEnumerable<InputParameter>

Mind that void is just a return type here (a Unit). So for example when the return type was InputParameter you would get the suggestion: Fix return type from InputParameter to IEnumerable<InputParameter>

Sorry for the brief suggestion, maybe I can add more details later @kuhlenh

---

Update (jcouv):
As of 16.1p1, some scenarios are now handled (see PR https://github.com/dotnet/roslyn/pull/33457):

• [x] void M() { return 1; } (CS0127)
• [x] async void M() { return 1; } (CS1997)
• [x] void M() => 1; (CS0201)

Other scenarios listed below are not handled yet. In particular:

• fixing an existing return type that is not void,
• giving a clever type, instead of object, when typeless expressions (lambdas) are returned,
• VB scenarios.

[jcouv]

Add a +1, as I've encountered this request before: https://twitter.com/dotMorten/status/1014208268342710272

[jnm2]

Fixing parameter, field and property types would be great too! I follow these things around all the time when refactoring.

[sharwell]

Design Meeting Notes

This went to a design review, and seems like a reasonable improvement. The preferred implementation would follow the rough design of other similar situations:

• Prefer to offer the diagnostic, even in cases where a change to the signature would break the code (e.g. the method implements an interface method before the change, but does not implement the interface method after the change).
• Add this code fix to the set of motivating examples for improving the conflict UI (@jinujoseph do we have an issue for this?).

Edge cases: Each of these cases has an initial design which can be implemented as-is, or a specific (detailed) alternative can be proposed.

• Asynchronous methods: If the method is marked async, wrap the return value in Task<TResult> to form the return type of the method.
• Iterator methods: If the statement is a yield return, wrap the return value in IEnumerable<T> to form the return type of the method.
• Async iterator methods: Depending on when this feature is implemented relative to the async iterator language feature, make sure the two work together. @jcouv should be able to provide updated details to whoever works on the implementation.
• Multiple returns: Do not analyze multiple return statements when determining the type of the method.
• Accessibility: Do not consider or change the accessibility of the containing member due to restrictions on the return type (e.g. do not make a public method internal just because the static type of the return statement has internal accessibility).
• Callers: Do not update callers with the new return type.
• Inherited/extended/implemented methods: Do not update related methods to match the new return type (@jinujoseph we need a work item to track creating a UI for "completing the current refactoring-assisted operation").
• Returning null: If the declared type is a struct, offer to convert it to the nullable form. Otherwise, if the declared type is void offer to convert it to object.

Other notes:

• This feature likely needs to handle several error codes. @MaStr11 may be able to narrow down some of these.
• This should ideally be implemented in the same release as #3164. Some of the error codes used by the fixes will overlap.

[sharwell]

Fixing parameter, field and property types would be great too!

Yes, this plays directly to my comments that this operation is distinct from #3164, but the two complement each other.

[jnm2]

Here's an example of behavior I'd expect:

c# public async Task<SomeStruct> Foo() { return null; // Should offer fix to change return type to Task<SomeStruct?> }

[sharwell]

@jnm2 Thanks, updated the list of edge cases to include null

[Neme12]

Asynchronous methods: If the method is marked async, wrap the return value in Task to form the return type of the method.

Additionaly, if the async method already returns ValueTask or some other task-like, I would expect the code fix to keep it and only change the type argument, as opposed to replacing it with Task.

Also, IEnumerator likely shouldn't be changed to IEnumerable.

[jnm2]

Right, the generic type shouldn't change but the generic parameter should. Also:

```c#
public async ValueTask<(int, string)> Foo()
{
return null; // Should offer fix to change return type to ValueTask<(int, string)?>
}

public IEnumerable<(int, string)> Foo()
{
yield return null; // Should offer fix to change return type to IEnumerable<(int, string)?>
}
```

[jnm2]

And:

c# public (int, string) Foo() { return (null, null); // Should offer fix to change return type to (int?, string) }

[MaStr11]

Prefer to offer the diagnostic, even in cases where a change to the signature would break the code (e.g. the method implements an interface method before the change, but does not implement the interface method after the change).

and

Inherited/extended/implemented methods: Do not update related methods to match the new return type (@jinujoseph we need a work item to track creating a UI for "completing the current refactoring-assisted operation").

In a related code fix #22314 AddParameterCodeFixProvider: Add support for method invocations. we offer two options for his problem (fix this method only or fix this and all related methods). My first thought was to do the same here. I haven't heard of a completing the current refactoring-assisted operation dialog before. It would be nice to get a description because #22314 is likely affected too. Furthermore, @heejaechang suggested in https://github.com/dotnet/roslyn/pull/22314#issuecomment-385201107 that cascading (aka fixing all related methods) is the better of the two options. This seems to contradict what you think is a good default for this code fixer.

This feature likely needs to handle several error codes. @MaStr11 may be able to narrow down some of these.

I will look into this, but I will likely miss some diagnostics (due to the nature of this task). I'm not sure whether Fixing parameter, field and property types as @jnm2 suggested should be all included in one fixer. If so, I would add local variables to the mix. Here are some examples that show, what I think could be handled in conjunction with the return type fix:

Fields (Properties behave the same):

class C
{
    int field; // All fixes will offer to change the declaration to string field;
    C()
    {
        field="Test"; // Offer fix on assignment
        M(field); // Offer fix if expression is not compatible to the overload. Attention: This will change the type of the field to string and not the string parameter to int parameter.
    }

    void M(string parameter) { }
}

Method parameter (local variables behave the same):

class C
{
    void M(int parameter) // All fixes will offer to change the parameter to string parameter
    {
        parameter="Test"; //Offer fix on assignment
        string s=parameter; //Offer fix if expression is not compatible to the expression target.
    }
}

These are the steps that the fixer would go through:

Find the symbol declaration to fix

This is different for the use cases:

• return: Containing method return type declaration
• field, property: Field/property declaration
• parameter: method argument
• local variable: variable declaration

Find a good type suggestion

If the method already has a return type, find a common type that the expression and return type share:
MemoryStream GetStream() => new FileStream(); should suggest Stream.

This two way merge of types is also valid for properties, fields, parameters and local variables.

Fixing of the return type

As @sharwell listed above, depending on the method kind (async, iterator, etc.), the return type may need to be wrapped. This applies to return types only.

Changing the declaration

This is different for all kinds (but follow the same pattern).

Cascading

This is not an issue as long as we don't support cascading. Otherwise, the return type, parameter, and properties are affected and the implementation likely differs for each.

While all of these have a lot in common, I'm not sure if it is a good idea to do all at the same time (there might be all kind of complications with generics and whatsoever). When I look into the error codes I will do so for the return type fix only for now.

Regarding #3164. There is only a loose coupling. Both fixers don't share any code. They can fix the same error (changing either the return type of the method to fit the expression type or fix the expression by a cast to fit the return type), but they could also be shipped independently.

[CyrusNajmabadi]

I would personally prefer cascading, however gated behind a separate code-action. So it would specifically offer things like:

1. change return type to 'X'
2. change return type to 'X' (including related methods)

(Probably using wording similar to what we've picked in the past for similar features). This feels fairly natural, and fortunately pretty easy to do.

[MaStr11]

WIP: My current list of compiler error candidates. I will finish this list later.

cs0127 Since 'function' returns void, a return keyword must not be followed by an object expression

        void M()
        {
            [|return|] 1;
        }

Applies to: return

cs0029 Cannot implicitly convert type 'type' to 'type'

        string M()
        {
            return [|1|];
        }

Applies to: return, property, field, local variable

CS0201 Only assignment, call, increment, decrement, and new object expressions can be used as a statement

void M() => 1;

Applies to: return

cs0508 and cs1715 'Type 1': return type must be 'Type 2' to match overridden member 'Member Name'

abstract public class Clx  
{  
   abstract public int F();  
}  

public class Cly : Clx  
{  
   public override double [|F|]()  
   {  
      return 0.0;
   }  
}

Applies to: return, properties

cs1520 Method must have a return type

Should add void

public class x  
{  
   [|MyMethod|]()
   {}  
}  

Applies to: return

cs1983 The return type of an async method must be void, Task or Task

Should fix to Task<int>

        async int [|MyMethod|]()
        {
            return 1;
        }

Applies to: return

cs0738'type name' does not implement interface member 'member name'. 'method name' cannot implement 'interface member' because it does not have the matching return type of ' type name'.

This one deviates to much from the other dignostics:

        interface ITest
        {
            int TestMethod();
        }
        public class Test : [|ITest|]
        {
            public void TestMethod() { } 
        }

Applies to: return

cs0407 'return-type method' has the wrong return type

    public delegate int MyDelegate();

    class C
    {
        public void G()
        {
            var d = new MyDelegate([|G|]);
        }
    }