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

Adopt typed throws in AsyncIteratorProtocol and AsyncSequence #70635

Merged
merged 22 commits into from Jan 29, 2024

Conversation

DougGregor
Copy link
Member

@DougGregor DougGregor commented Dec 27, 2023

Introduce a new associated type Failure into the two protocols involved
in async sequences, which represents the type thrown when the sequence
fails. Introduce a defaulted _next(_:) operations that is isolated to the
given actor (or nil) and either throws Failure or produces the next
element of the sequence. Provide a default implementation of next(_:)
in terms of next() that force-casts the thrown error to the Failure type,
and a default implementation in terms of next() in terms of next(_:) that
lets new async iterators only define the new entrypoint.

Drop @rethrows from AsyncSequence and AsyncIteratorProtocol;
this unofficial feature is on its way out.

Introduce special associated type inference logic for the Failure
type of an AsyncIteratorProtocol conformance when there is no
specific next(_:) witness. This inference logic looks at the
witness for next():

  • If next() throws nothing, Failure is inferred to Never.
  • If next() throws, Failure is inferred to any Error.
  • If next() rethrows, Failure is inferred to T.Failure, where
    T is the first type parameter with a conformance to either
    AsyncSequence or AsyncIteratorProtocol.

The default implementation and the inference rule, together, allow
existing async sequences to continue working as before. Switch the
contract between the compiler and library for the async for loop to
use next(_:) rather than next() when targeting a new-enough
deployment target.

Give AsyncSequence and AsyncIteratorProtocol primary associated
types for the element and failure types, which will allow them to be
used more generally with existential and opaque types, i.e.,
any AsyncSequence<Image, NetworkError>.

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

Comment on lines 1068 to 1079
// If there is a generic parameter named Failure, don't try to use next()
// to infer Failure.
if (auto genericSig = dc->getGenericSignatureOfContext()) {
for (auto gp : genericSig.getGenericParams()) {
// Packs cannot witness associated type requirements.
if (gp->isParameterPack())
continue;

if (gp->getName() == assocType->getName())
return llvm::None;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very sketchy and I want to try eliminating it.

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

2 similar comments
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please build toolchain

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please build toolchain

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

<unknown>:0: error: 'nextElement()' is only available in macOS 9999 or newer
<unknown>:0: error: 'nextElement()' is only available in macOS 9999 or newer

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

…fects

Our decision procedure for what type of error is thrown during
iteration of an async sequence was effectively duplicated between the
checking for the next/nextElement call (which is synthesized) and
separate logic for the async for..in loop. The latter is strictly
redundant, so switch to relying only on the former.

This is staging for customizing the next/nextElement call further.
… on availability

This allows us to not break backward deployment
…cking

Some values (such as `undef`) might not be associated with a function.
This isn't done very well, and is expected to be replaced, but it
allows us to avoid bogus Sendable diagnostics for iteration over
async sequences.
Use an optional isolated parameter to this new `next(_:)` overload to
keep it on the same actor as the caller, and pass `#isolation` when
desugaring the async for..in loop. This keeps async iteration loops on
the same actor, allowing non-Sendable values to be used with many
async sequences.
…latMap`

Allow `AsyncSequence.flatMap` to be defined with "incorrect" availability,
meaning that the function can refer to the `Failure` associated type
in its where clause even though the function is back-deployed to
before the `Failure` associated type was introduced.

We believe this is safe, and that this notion can be generalized to any
use of an associated type in a same-type constraint of a function
(yes, it sounds weird), but for now introduce a narrower hack to see
how things work in practice and whether it addresses all of the
source-compatibility concerns we've uncovered.
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@swift-ci please build toolchain

@DougGregor
Copy link
Member Author

@swift-ci please build toolchain Windows

@DougGregor
Copy link
Member Author

@swift-ci please smoke test Linux

2 similar comments
@DougGregor
Copy link
Member Author

@swift-ci please smoke test Linux

@DougGregor
Copy link
Member Author

@swift-ci please smoke test Linux

@DougGregor
Copy link
Member Author

@swift-ci please test windows

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through the whole thing so far, not much to add but looking good, heroic effort Doug :)

ArgumentList *nextArgs;
if (nextFn && nextFn->getParameters()->size() == 1) {
auto isolationArg =
new (ctx) CurrentContextIsolationExpr(stmt->getForLoc(), Type());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I like this name for it 👍

@@ -86,13 +86,40 @@ import Swift
/// checking for cancellation as described above, or in `deinit` if it's
/// a reference type.
@available(SwiftStdlib 5.1, *)
@rethrows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goodbye 👋 😊

/// required to maintain backward compatibility with existing async iterators.
@available(SwiftStdlib 5.11, *)
@inlinable
public mutating func next(_ actor: isolated (any Actor)?) async throws(Failure) -> Element? {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we going to do the "conform to just one of them" dance the same as with executors, or different trickery?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're relying on the deprecation of the old one to help here, at least from Swift 6 on

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey, so not the the "implement just one of them" -- might be for the best, that was causing a lot of pain in executor rollout, probably not worth it.

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@swift-ci please build toolchain macOS

@DougGregor
Copy link
Member Author

@swift-ci please build toolchain Windows

@DougGregor
Copy link
Member Author

Lots of spurious failures, so rerunning a number of jobs. Yesterday was a rough day to be a CI system.

@DougGregor
Copy link
Member Author

@DougGregor
Copy link
Member Author

I'm going to speculatively merge this to unblock more testing. We can back out the merge or change things depending on how the review goes.

@DougGregor DougGregor merged commit 0cc5297 into apple:main Jan 29, 2024
8 checks passed
@DougGregor DougGregor deleted the async-sequence-typed-throws branch January 29, 2024 19:51
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.

None yet

2 participants