-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Obsolete NamedPipeClientStream constructor with isConnected parameter (SYSLIB0063) #120328
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
base: main
Are you sure you want to change the base?
Conversation
… (SYSLIB0063) Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Tagging subscribers to this area: @dotnet/area-system-io |
src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR obsoletes the NamedPipeClientStream
constructor that takes a bool isConnected
parameter (as it's effectively meaningless) and introduces a new constructor without this parameter. The isConnected
parameter only ever had one valid value (true
), making it redundant and confusing.
Key changes:
- Introduces diagnostic ID
SYSLIB0063
for the obsolete constructor - Adds a new constructor without the
isConnected
parameter that always sets the connection state toConnected
- Updates existing tests to use the new constructor or suppress the obsolete warning where necessary
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs | Adds the new constructor and marks the old one as obsolete with SYSLIB0063 |
src/libraries/System.IO.Pipes/ref/System.IO.Pipes.cs | Updates reference assembly with new constructor and obsolete attribute |
src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj | Links Obsoletions.cs to the project |
src/libraries/Common/src/System/Obsoletions.cs | Defines SYSLIB0063 diagnostic ID and message |
docs/project/list-of-diagnostics.md | Documents the new SYSLIB0063 diagnostic |
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs | Adds tests for the new constructor and updates existing tests with pragma warnings |
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CrossProcess.cs | Updates to use new constructor |
src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs | Updates to use new constructor |
…lidate tests Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
…ion" This reverts commit 071c67f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see Copilot following our obsoletion instructions!
public NamedPipeClientStream(PipeDirection direction, bool isAsync, bool isConnected, SafePipeHandle safePipeHandle) | ||
: this(direction, isAsync, safePipeHandle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to file a breaking change for that? This can break any exist code I guess depending on isConnected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only symptom is that on Connect[Async]
we will stop throwing an unintended DirectoryNotFoundException and instead we will throw an InvalidOperationException because it will now be assumed that the pipe client is connected.
I would prefer not to file the breaking change because this behavior was not useful at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please add a test that creates a server and client named pipe, then re-creates the client named pipe from the existing client handle, then calls connect and assert that it throws InvalidOperationException. Test the new constructor, and the obsoleted constructor with both true/false for isConnected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Added test ConnectOnPipeFromExistingHandle_Throws_InvalidOperationException
that validates the behavior for the new constructor and the obsolete constructor with both isConnected=true and isConnected=false. All three cases now throw InvalidOperationException when Connect is called, confirming that isConnected is ignored. Commit d5e8c89.
…andle Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, please address this feedback.
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Done. Applied all feedback with commit 8f4a2d1:
|
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateClient.cs
Outdated
Show resolved
Hide resolved
Remove lambdas
✅ Implementation Complete: Obsolete NamedPipeClientStream constructor with isConnected parameter (SYSLIB0063)
Summary of Changes
This PR implements the approved API proposal to obsolete the
NamedPipeClientStream
constructor that takes abool isConnected
parameter, and introduces a new constructor without it.Changes Made:
NamedPipeClientStream(PipeDirection, bool, SafePipeHandle)
that always sets state to Connected[Obsolete]
and[EditorBrowsable(EditorBrowsableState.Never)]
attributesAPI Changes:
Compatibility:
isConnected
parameter is now ignored by delegating to the new constructorOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.