S4457 flags the compliant solution as uncompliant

[pa...@paulomorgado.info]

Hi,

If one makes the changes in https://rules.sonarsource.com/csharp/RSPEC-4457, SkipLinesAsync will still be flag as uncompliant.

[pa...@paulomorgado.info]

OOPS!

My refactoring added await.

Nevertheless, https://blog.stephencleary.com/2016/12/eliding-async-await.html

[amaur...@sonarsource.com]

Hi,

Could you explain what you think is wrong? What shall I be looking in the article you quoted?

[pa...@paulomorgado.info]

Hi,

My refactoring tool had refactored the non compliant code into:

public static async Task SkipLinesAsync(this TextReader reader, int linesToSkip)
{
    if (reader == null) { throw new ArgumentNullException(nameof(reader)); }
    if (linesToSkip < 0) { throw new ArgumentOutOfRangeException(nameof(linesToSkip)); }

    return await reader.SkipLinesInternalAsync(linesToSkip);
}

private static async Task SkipLinesInternalAsync(this TextReader reader, int linesToSkip)
{
    for (var i = 0; i < linesToSkip; ++i)
    {
        var line = await reader.ReadLineAsync().ConfigureAwait(false);
        if (line == null) { break; }
    }
}

And that's still non compliant.

If SkipLinesAsync is immediately awaited there's no significant difference between the two versions in the case of validations (synchronous part) throwing.

However, if an exception is thrown from the asynchronous part, the compliant code will loose the information of SkipLinesAsync and will throw as if SkipLinesInternalAsync was directly assigned.

[Amaury Leve]

Hi Paulo,

Your code is indeed non-compliant because the issue won't be fired at the moment you expect. Look at the code below to understand why.

static class Program
    {
        static void Main(string[] args)
        {
            var test1 = SkipLinesAsync1(null, 1);
            test1.GetAwaiter().GetResult(); // throws here - only when awaited
 
            var test2 = SkipLinesAsync2(null, 1); // throws here - this is what you would expect
            test2.GetAwaiter().GetResult();
 
            Console.ReadKey();
        }
 
        public static async Task SkipLinesAsync1(this TextReader reader, int linesToSkip)

        {
            if (reader == null)
            {
                throw new ArgumentNullException(nameof(reader));
            }
            if (linesToSkip < 0)
            {
                throw new ArgumentOutOfRangeException(nameof(linesToSkip));
            }

 
            await reader.SkipLinesInternalAsync(linesToSkip);
        }
 
        public static Task SkipLinesAsync2(this TextReader reader, int linesToSkip)

        {
            if (reader == null)
            {
                throw new ArgumentNullException(nameof(reader));
            }
            if (linesToSkip < 0)
            {
                throw new ArgumentOutOfRangeException(nameof(linesToSkip));
            }

 
            return reader.SkipLinesInternalAsync(linesToSkip);
        }

 
        private static async Task SkipLinesInternalAsync(this TextReader reader, int linesToSkip)
        {
            for (var i = 0; i < linesToSkip; ++i)
            {
                var line = await reader.ReadLineAsync().ConfigureAwait(false);
                if (line == null)
                { break; }
            }
        }
    }

Does it make sense?

Cheers,

Amaury

--
You received this message because you are subscribed to the Google Groups "SonarLint" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarlint+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarlint/c659eb9f-9408-42ef-875c-da374277dafd%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Amaury Levé | SonarSource

Software Developer - .Net Team

http://sonarsource.com

Are you using SonarLint in your IDE? 

[pa...@paulomorgado.info]

Yes. But it comes with some stack trace erasure.

[Amaury Leve]

Hi,

I agree with you that when your async method is always awaited directly the behavior seems correct. Although there are 2 main points you might want to keep in mind. The first one is that if someone else calls this method and doesn't await it directly they will end up with a wrong behavior. The second point is that when you follow Microsoft TAP guidelines they recommend to return a failed task rather than throwing exception.

If you do think the rule is not relevant for your usage you have the possibility to disable it on your ruleset/quality profile. If you think the rule is valuable but some of the issues raised shouldn't be fixed in your situation, you have the possibility to mark it as FP or Won't Fix on SonarQube/SonarCloud.

All of that being said I acknowledge that most of the time you will await directly an async method and so it might be worth for us to reconsider the severity of this rule.

Cheers,

Amaury

On Thu, May 17, 2018 at 12:34 AM <pa...@paulomorgado.info> wrote:

Yes. But it comes with some stack trace erasure.

--

You received this message because you are subscribed to the Google Groups "SonarLint" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarlint+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarlint/b2fbb921-0cec-46db-a01c-39eb4565d108%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.