Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCS1047 "Remove Async suffix" is incorrectly reported for IAsyncEnumerable #1084

Closed
ovska opened this issue May 2, 2023 · 3 comments · Fixed by #1091
Closed

RCS1047 "Remove Async suffix" is incorrectly reported for IAsyncEnumerable #1084

ovska opened this issue May 2, 2023 · 3 comments · Fixed by #1091

Comments

@ovska
Copy link
Contributor

ovska commented May 2, 2023

Original title: RCS1047 "Remove Async suffix" is incorrectly reported for IAsyncEnumerables

Product and Version Used:
Roslynator 4.3.0 on Visual Studio 2022 17.5.5 64bit

Steps to Reproduce:

public static class Test
{
    public static AsyncEnumerable EnumerateAsync() // <- reported here
    {
        return new AsyncEnumerable();
    }
}

public readonly struct AsyncEnumerable
{
    public IAsyncEnumerator<int> GetAsyncEnumerator(CancellationToken cancellationToken = default)
    {
        throw new NotImplementedException();
    }
}

Actual Behavior:
The EnumerateAsync-method's Async-suffix is faded, and roslynator suggests to remove the suffix. If the returned type implements IAsyncEnumerable<>, the analyzer isn't triggered. This might be intended or an oversight, and luckily it's not a big deal if it won't be fixed.

Expected Behavior:
The analyzer could check if the type implements GetAsyncEnumerator directly. This is another can of worms too, though, since the enumerator isn't required to implement the interface either, as long as it has Current and MoveNextAsync().


My reasoning for foregoing the interface is to avoid the enumerable/enumerator accidentally being used with
LINQ or boxed to be used anywhere except the callsite. As for the suffix, there are synchronous overloads in the same class, and I wanted to separate the ones returning an async enumerable.

@ovska
Copy link
Contributor Author

ovska commented May 7, 2023

The bug seems to also affect a lot more cases (e.g. everything except a direct IAsyncEnumerable<>).

I made a try at it, and it ends up really complex (even ignoring the fact that the enumerator might not implement IAsyncEnumerator<>). Involves a lot of nested loops, walking the inheritance chain, and checking interfaces and type parameters. It might not be worth the benefit, let me know what you think.
Link: ovska@d6b4d95

Examples of also failing cases of valid IAsyncEnumerables:

public static class Test
{
    public static T EnumerateGenericAsync<T>() where T : IAsyncEnumerable<T>
    {
        return default!;
    }

    public static Base EnumerateBaseAsync()
    {
        return default!;
    }

    public static Inherited EnumerateInheritedAsync()
    {
        return default!;
    }

    public static T EnumerateBaseOfTAsync<T>() where T : Base
    {
        return default!;
    }

    public static T EnumerateInheritedOfTAsync<T>() where T : IAsyncEnumerable<T>
    {
        return default!;
    }
}

public class Inherited : Base
{
}

public class Base : IAsyncEnumerable<string>
{
    public IAsyncEnumerator<string> GetAsyncEnumerator(CancellationToken cancellationToken = default) => default!;
}

@ovska ovska changed the title RCS1047 "Remove Async suffix" is reported for duck-typed IAsyncEnumerable RCS1047 "Remove Async suffix" is incorrectly reported for IAsyncEnumerable May 7, 2023
@josefpihrt
Copy link
Collaborator

Hi,

Do you want to do a PR?

If yes, then following methods might help you:

  • extension method ContainsMember<IMethodSymbol> will check if a type contains method GetAsyncEnumerator.
  • method GetPossiblyAwaitableType will extract type parameter if necessary.
  • extension method Implements(true) will check if the type implements IAsyncEnumerable<T>.

@ovska
Copy link
Contributor Author

ovska commented May 24, 2023

Sure thing. I think I'll probably just do a fix for types implementing IAsyncEnumerable<T> via interface/inheritance or constraint. The problem of duck typing is much too broad for this case, I think. For example, SymbolUtility.IsAwaitable doesn't recognize a GetAwaiter extension method which returns a non-standard task awaiter type, which nonetheless is usable with await in C# 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants