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

Override Stream.ReadAsync/WriteAsync #33789

Closed
terrajobst opened this issue Mar 19, 2020 · 17 comments
Closed

Override Stream.ReadAsync/WriteAsync #33789

terrajobst opened this issue Mar 19, 2020 · 17 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@terrajobst
Copy link
Member

The rule should flag types derived from Stream that override BeginRead/EndRead or BeginWrite/EndWrite but that don't override ReadAsync or WriteAsync. And it should flag types derived from Stream that override the array-based ReadAsync or WriteAsync but that don't override the Memory-based overloads of the same name. (Potentially the same should be done for the Span-based overloads, but as the array-based Read and Write methods are abstract and thus must be overridden, it's harder to say whether those should be or not.)

Category: Performance

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@jeffhandley jeffhandley added this to To do in Roslyn Analyzers Mar 20, 2020
@jeffhandley jeffhandley added this to the Future milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Medium
  • Fixer: Medium

@stephentoub
Copy link
Member

Fixer: Medium

I don't think there's anything meaningful a fixer could do here.

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2020
@carlossanlop
Copy link
Member

I don't think there's anything meaningful a fixer could do here.

@stephentoub maybe the fixer could offer empty overrides for those methods, similar to what CS0534 does for abstract classes:

image

@carlossanlop
Copy link
Member

@terrajobst why should the span-based Read/Write overrides not be suggested as part of this analyzer?

Suggested category: Performance
Suggested severity: Info
Suggested milestone: 6.0

Pending to decide in API review:

  • If we want a code fixer that behaves like CS0534 or not.
  • If we want to also require the span-based Read/Write to get overriden.

Suggested analyzer behavior:

class MyStream : Stream
{
    // After adding the four BeginRead/BeginWrite EndRead/EndWrite overrides below,
    // the analyzer will trigger a warning suggesting to also override:
    // - The array-based ReadAsync/WriteAsync methods
    // - The memory-based ReadAsync/WriteAsync methods
    // - The span-based Read/Write methods
    public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, object state)
    {
        return ...;
    }

    public override int EndRead(IAsyncResult asyncResult)
    {
        return ...;
    }

    public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, object state)
    {
        return ...;
    }

    public override void EndWrite(IAsyncResult asyncResult)
    {
        ...;
    }

    // Suggested to override
    public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
    {
        return ...;
    }

    // Suggested to override
    public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
    {
        return ...;
    }

    // Suggested to override
    public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
    {
        return ...;
    }

    // Suggested to override
    public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)
    {
        return ...;
    }

    // Suggested to override
    public override int Read(Span<byte> buffer)
    {
        return ...;
    }

    // Suggested to override
    public override void Write(ReadOnlySpan<byte> buffer)
    {
        ...;
    }

    // Everything else below was automatically added by CS0534 because they were all abstract

    public override bool CanRead { get; }
    public override bool CanSeek { get; }
    public override bool CanWrite { get; }
    public override long Length { get; }
    public override long Position { get; set; }

    public override void Flush()
    {
        throw new NotImplementedException();
    }

    // This is one of the methods Immo mentioned
    public override int Read(byte[] buffer, int offset, int count)
    {
        throw new NotImplementedException();
    }

    public override long Seek(long offset, SeekOrigin origin)
    {
        throw new NotImplementedException();
    }

    public override void SetLength(long value)
    {
        throw new NotImplementedException();
    }

    // This is one of the methods Immo mentioned
    public override void Write(byte[] buffer, int offset, int count)
    {
        throw new NotImplementedException();
    }
}

@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 20, 2020
@carlossanlop carlossanlop moved this from To do to Ready for review in Roslyn Analyzers Nov 20, 2020
@carlossanlop carlossanlop modified the milestones: Future, 6.0.0 Nov 20, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 24, 2020
@terrajobst
Copy link
Member Author

terrajobst commented Nov 24, 2020

Video

  • We should scope it to cases is Stream (and not a derived type)
  • The rule should flag types that override
    • ReadAsync(byte[]) but that don't override the overload with Memory<T>
    • WriteAsync(byte[]) but that don't override the overload with Memory<T>
  • On by default with severity info
  • Perf category
  • No fixer needed, the default behavior of auto completing override or Ctrl + . Generate overrides is sufficient

@Joe4evr
Copy link
Contributor

Joe4evr commented Nov 26, 2020

Re: A different analyzer that suggests to override certain virtuals in more general cases: I've heard the term "soft-abstract" once or twice to refer to those type of members, so wouldn't it make sense to have that analyzer trigger on a SoftAbstractAttribute marker?

@carlossanlop carlossanlop added the help wanted [up-for-grabs] Good issue for external contributors label Nov 30, 2020
@carlossanlop
Copy link
Member

@Joe4evr feel free to write an analyzer proposal in a separate issue for the general case you mentioned.

@carlossanlop
Copy link
Member

@Mrnikbobjeff if this is one of the analyzers for which you already had an implementation, let me know so I can assign it to you. Otherwise, it's up for grabs.

@buyaa-n buyaa-n moved this from Ready for review to .NET 6.0 in Roslyn Analyzers Nov 30, 2020
@NewellClark
Copy link
Contributor

NewellClark commented Jan 17, 2021

I would like to be assigned to this issue. I have an implementation almost completed. I should have a pr by Monday.

@danmoseley
Copy link
Member

It won't let me assign you for some reason but consider it yours!

@NewellClark
Copy link
Contributor

@danmosemsft thank you. I have a working analyzer, but I've found some violations in dotnet/runtime, which I'm working on fixing. Do you want me to wait until all violations in runtime are fixed? Or should I go ahead and submit the analyzer now?

@carlossanlop
Copy link
Member

I've found some violations in dotnet/runtime
Do you want me to wait until all violations in runtime are fixed? Or should I go ahead and submit the analyzer now?

@NewellClark do you mean your new analyzer is finding cases in the dotnet/runtime code? If that's the case, please share a PR with the flagged dotnet/runtime cases fixed. Also please share a PR with your new analyzer code. It would be great if we can review both things in parallel.

@NewellClark
Copy link
Contributor

NewellClark commented Jan 21, 2021

I've found some violations in dotnet/runtime
Do you want me to wait until all violations in runtime are fixed? Or should I go ahead and submit the analyzer now?

do you mean your new analyzer is finding cases in the dotnet/runtime code?

@carlossanlop Yes, I've found seven violations. I've already submitted two PRs. One of the violations (WebSocketHttpListenerDuplexStream) would be complicated to spanify and additionally is only used internally. I'll provide more details about that one when I submit the analyzer PR. Sorry it's taken me awhile to respond; one of the fix-PRs took longer to get up than I expected.

@carlossanlop
Copy link
Member

Thanks for helping with this, @NewellClark . I see you are already getting feedback in the runtime PR. I look forward to reviewing your analyzer PR.

@NewellClark
Copy link
Contributor

Would it be possible to have this issue linked to this pr?

@jeffhandley
Copy link
Member

@NewellClark Since the PR is in a different repo, I don't think GitHub will mark it as a linked PR. Cross-referencing as you've done in the PR description and the issue comment here matches what we generally do though.

@carlossanlop carlossanlop moved this from .NET 6.0 to In progress in Roslyn Analyzers Feb 4, 2021
@adamsitnik adamsitnik modified the milestones: 6.0.0, 7.0.0 Jul 22, 2021
@wzchua
Copy link
Contributor

wzchua commented Jul 24, 2021

Is the issue considered done with dotnet/roslyn-analyzers#4726 merged?

@jeffhandley jeffhandley modified the milestones: 7.0.0, 6.0.0 Jul 26, 2021
System.IO - Streams automation moved this from To do to Done Jul 26, 2021
.NET Analyzers (vNext) automation moved this from To do to Done Jul 26, 2021
@jeffhandley jeffhandley moved this from In progress to Done in Roslyn Analyzers Jul 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer help wanted [up-for-grabs] Good issue for external contributors
Projects
Development

No branches or pull requests

10 participants