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
18.99k stars 4.03k forks source link

CodeFix for CS1998 does not consider/adapt all kinds of usages #39890

Closed penenkel closed 1 year ago

penenkel commented 4 years ago

Version Used: VS2019.3.9 - .NET Core 3.0 ConsoleApp

Issue It is my opinion that applying a CodeFix should not cause syntax errors (assuming the code was valid before). Under that assumption I find it rather annoying that the CodeFix for CS1998 does not respect the usages of the methods it modifies (at least not all), resulting in syntax errors.

Steps to Reproduce: Consider the following example

    class A
    {
        public async Task DemoUsage1()
        {
            var result = await Method();
        }
        public void DemoUsage2()
        {
            var result = Method().Result;
        }
        public async Task DemoUsage3()
        {
            Func<Task<string>> func = Method;
            var result = await func.Invoke();
        }

        public async Task<string> Method() // CS1998: This async method lacks 'await' operators and will run synchronously.
        {
            return "abc";
        }
    }

which is fixed to:

    class A
    {
        public async Task DemoUsage1()
        {
            var result = Method(); // usage adapted -> valid
        }
        public void DemoUsage2()
        {
            var result = Method().Result; // usage not adapted -> invalid syntax
        }
        public async Task DemoUsage3()
        {
            Func<Task<string>> func = Method;  // usage not adapted -> invalid syntax
            var result = await func.Invoke();
        }

        public string Method() // now synchronous
        {
            return "abc";
        }
    }

which shows that only one of the usage kinds has been adapted correctly.

Expected Behavior: As stated above: I would expect that a CodeFix does not produce incorrect code. Obviously this is not so trivial to fix. In case 2 it would probably be OK to just remove the .Result (ignoring the fact that such a usage should better not exist at all). But as soon as the method is stored in a variable one probably should not modify the signature at all and instead use something like return Task.FromResult("abc") within the method. (This is especially relevant when used with lambda functions like Func<Task<string>> func = async () => "abc", which should probably transformed to `Func<Task> func = () => Task.FromResult("abc")`` or similar.

Joe4evr commented 4 years ago

It is my opinion that applying a CodeFix should not cause syntax errors

Well, sadly, your opinion doesn't align with how the compiler team has designed it. Sure, if applying a CodeFix results in a syntax error, that's bad, but it's never claimed that it wouldn't do that. On the contrary, it's specifically allowed to do so for the case where the developer intends to refactor further after the fix is applied.

In fact, if the fix was not suggested due to resulting in errors, you'd end up in a world where CodeFixes are almost never used because they're suggested far too little to be useful because they're being far too conservative about when they should be suggested.

penenkel commented 4 years ago

hm, well, I guess that makes kind of sense. Too bad.