dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.04k stars 4.03k forks source link

'Introduce local for x' changes code and introduces errors #25838

Open vsfeedback opened 6 years ago

vsfeedback commented 6 years ago

When extracting code with "Introduce local for 'x'" refactoring:

This issue has been moved from https://developercommunity.visualstudio.com/content/problem/63032/introduce-local-for-x-changes-code-and-introduces.html VSTS ticketId: 444218 These are the original issue comments:

Sam Harwell [MSFT] on ‎6‎/‎2‎/‎2017, 00:30 PM (300 days ago):

Hi Rafael,

I'm currently trying to reproduce this. I tested a handful of samples in both 15.2 and 15.3 Preview 1 with things like nullable types and LINQ helper methods, but was so far unable to observe the behavior you described. It sounds like you may have hit a special case related to your specific code that wasn't accounted for by the refactoring yet. Would you be able to share a code sample that reproduces this to help me track it down?

Thanks,

Sam

Sam Harwell [MSFT] on ‎7‎/‎3‎/‎2017, 02:38 PM (269 days ago):

Thank you for your feedback! For us to investigate this further, could you please provide some sample code which reproduces the issue? We look forward to hearing from you!

Kevin Pilch [MSFT] on ‎7‎/‎24‎/‎2017, 03:29 PM (248 days ago):

Thank you for your feedback! Unfortunately, right now we don't have enough information to investigate this issue and find a solution. If this is still an issue for you, please update to our latest version. If you are still able to repro it, please provide us with more info! Thank you for helping us build a better Visual Studio!

Rafael de Lima Reis on ‎3‎/‎27‎/‎2018, 01:37 PM (2 days ago): Repro steps.Create an extension method like below: public static Runtime.CompilerServices.ConfiguredTaskAwaitable IgnoreContext(this Task task) { return task.ConfigureAwait(false); }Call the method from some Task-valued expresion, e.g: var text = await FileEx.ReadAllText(outputFilePath).IgnoreContext();Select "await FileEx.ReadAllText(outputFilePath)" and activate "Introduce local for" refactoring.Voilá: var task = FileEx.ReadAllText(outputFilePath); var text = await System.Threading.Tasks.TaskExtensions.IgnoreContext(task);We can observe: 1. The extension method call was rewritten as a static method call. 2. A type variable argument was introduced where there wasn't any.Depending on the case, the result can be way worse (the more complicated the line where you extract the code, the worse). I've seen casts being introduced where none was needed. More fundamentally, all these transformations introduces syntactic changes -- altering the original text of the program.Thanks. These are the original issue solutions: (no solutions)

sharwell commented 6 years ago

Here is a reformatted copy of the repro steps from the last comment:

  1. Create an extension method like below:

    public static ConfiguredTaskAwaitable IgnoreContext(this Task task)
    {
      return task.ConfigureAwait(false);
    }
  2. Call the method from some Task-valued expresion, e.g:

    var text = await FileEx.ReadAllText(outputFilePath).IgnoreContext();
  3. Select await FileEx.ReadAllText(outputFilePath) and activate "Introduce local for" refactoring. Voilá:

    var task = FileEx.ReadAllText(outputFilePath);
    var text = await System.Threading.Tasks.TaskExtensions.IgnoreContext(task);

We can observe:

  1. The extension method call was rewritten as a static method call.
  2. A type variable argument was introduced where there wasn't any.

Depending on the case, the result can be way worse (the more complicated the line where you extract the code, the worse). I've seen casts being introduced where none was needed. More fundamentally, all these transformations introduces syntactic changes -- altering the original text of the program.

Thanks.

CyrusNajmabadi commented 6 years ago

Looks like an issue with complexification/simplification.