Skip to content

Narrow StringOutput.OutputType to non-optional#338

Open
broken-circle wants to merge 1 commit into
swiftlang:mainfrom
broken-circle:StringOutput-optional
Open

Narrow StringOutput.OutputType to non-optional#338
broken-circle wants to merge 1 commit into
swiftlang:mainfrom
broken-circle:StringOutput-optional

Conversation

@broken-circle

Copy link
Copy Markdown
Member

StringOutput.output(from:) never returns an optional string, so the associated OutputType does not need to be optional. This PR narrows its type to String. This is a breaking API change, but one I think is worth landing before 1.0.

The current behavior is that the user always gets a non-optional String. Invalid byte sequences are replaced with the Unicode replacement character (U+FFFD), which is the behavior of String(decoding:as:) that output(from:) already uses via String(decodingBytes:as:). This also makes StringOutput consistent with DataOutput, whose OutputType is non-optional Data.

The optional type was introduced in iCharlesHu/Subprocess/main@7f9cea8 and brought over to this repository in the initial commit. I couldn't find a reason the type was optional. The type permits nil, but no code path produces it; if returning nil on invalid input is behavior we want, let me know and I'll look into it as a follow-up PR. String(validating:as:) would be the appropriate API, but it's gated on macOS 15+ (Subprocess supports macOS 13+), so the implementation will require availability-gated fallbacks.

`StringOutput.output(from:)` never returns an optional string, so the
associated `OutputType` does not need to be optional. Narrow its type
to `String`.
@broken-circle

broken-circle commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

@iCharlesHu, somewhat related question: the 1.0 beta release includes the deprecated .standardOutput/.standardError aliases on FileDescriptorOutput (the where Self == FileDescriptorOutput extension in Output.swift), each marked // TODO: remove for 1.0. Now that the beta has shipped with them present, is removal before 1.0 still the plan, or do they need to stay through 1.0 for source compatibility? I can open a follow-up PR to remove them if you want.

@jakepetroules

Copy link
Copy Markdown
Contributor

Personally I think String(decoding:as:) + non-Optional is the right default, if people really want to validate that the text doesn't contain invalid bytes they could always use bytes and do the parsing themselves.

@broken-circle

Copy link
Copy Markdown
Member Author

Exactly what I was thinking!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants