Skip to content

Replace rethrowing map with generic typed throws #69771

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 13, 2024
Merged

Replace rethrowing map with generic typed throws #69771

merged 7 commits into from
Jan 13, 2024

Conversation

DougGregor
Copy link
Member

@DougGregor DougGregor commented Nov 10, 2023 •
edited
Loading

Adopt typed throws for the map operation to propagate thrown error
types through the map API. The new map has a signature like this:

  public func map<T, E>(
    _ transform: (Element) throws(E) -> T
  ) throws(E) -> [T]

This is done in a manner that is backward compatible for almost all cases:

  • The new typed-throws entrypoint is @_alwaysEmitIntoClient so it can back-deploy all the way back.
  • The old rethrows entrypoint is left in place, using @usableFromInline with internal so it is part of the ABI but not the public interface, @_silgen_name so it has the right name in the binary, and a new name __rethrows_map so it's otherwise out of the way. The old entrypoint is implemented in terms of the new one.

Note that the implementation details for the existential collection "box" classes rely on method overriding of _map operations, and the types are frozen, so we don't get to change their signatures. However, these are only implementations, not API: the actual API map functions can be upgraded to typed throws.

Note that this code makes use of a known hole in rethrows checking to allow calling between rethrows and typed throws. We'll need to do something about this for source-compatibility reasons, but I'll follow up with that separately.

@DougGregor DougGregor requested a review from a team as a code owner November 10, 2023 06:32
@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

Note that we expect some tests to fail here, because I haven't updated the expected output in a number of places.

@DougGregor
Copy link
Member Author

(Only the last two commits are actually new)

@DougGregor DougGregor mentioned this pull request Nov 10, 2023
Merged
21 tasks
@stephentyrone
Copy link
Contributor

stephentyrone commented Nov 10, 2023 •
edited
Loading

Generally we provide these legacy ABI entrypoints via @available(*, unavailable) (sometimes with a source name like _legacy_se04xx_map and @_silgen_name to the right symbol) rather than internal @usableFromInline @_disfavoredOverload. Both ways seem fine to me, but is there any reason for the difference here?

@DougGregor
Copy link
Member Author

DougGregor commented Nov 10, 2023 •
edited
Loading

Generally we provide these legacy ABI entrypoints via @available(*, unavailable) (sometimes with a source name like _legacy_se04xx_map and @_silgen_name to the right symbol) rather than internal @usableFromInline @_disfavoredOverload. Both ways seem fine to me, but is there any reason for the difference here?

I prefer @usableFromInline internal to @available(*, unavailable) because it's more hidden from the language model. More importantly, we're looking to delete the bodies of unavailable functions to save on code size, so one shouldn't use the latter for code that's still supposed to work.

The @_disfavoredOverload hack is a new experiment, partly out of laziness of not wanting to look up the SIL names for the existing symbols. I think I've decided that I don't really like it much, and have gone back to double-unscored, silgen_name'd versions for the ABI-only entrypoints.

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

glessard
glessard reviewed Nov 10, 2023
View reviewed changes
glessard
glessard reviewed Nov 10, 2023
View reviewed changes
@DougGregor
Copy link
Member Author

How fun. It's very crashy on Intel, yet fine on Apple Silicon. Time for some low-level debugging.

@tshortli
Copy link
Contributor

tshortli commented Nov 11, 2023 •
edited
Loading

Generally we provide these legacy ABI entrypoints via @available(*, unavailable) (sometimes with a source name like _legacy_se04xx_map and @_silgen_name to the right symbol) rather than internal @usableFromInline @_disfavoredOverload. Both ways seem fine to me, but is there any reason for the difference here?

We will need to be careful about this pattern going forward because one of the features I've been working on that was recently enabled on main compiles @available(*, unavailable) code either down to stubs that trap in the case of a library evolution enabled module, or completely omits the unavailable code entirely for non-resilient modules. This is to save code size in libraries with a lot of unavailable code. Semantically, @available(*, unavailable) means to the compiler that "this declaration has never been reachable" so it's not really appropriate for stubs needed for ABI compatibility. I personally think the @usableFromInline internal pattern for compatibility stubs is a better one anyways because it doesn't allow clients to even name the stub declaration.

@stephentyrone
Copy link
Contributor

stephentyrone commented Nov 11, 2023 •
edited
Loading

That’s a massively ABI breaking change that will need to go through evolution, even if it is desirable. I agree that’s probably what unavailable should have meant, but in practice it’s not what it ever has meant.

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

x86 miscompile is addressed by #69819

@DougGregor DougGregor enabled auto-merge November 13, 2023 19:51
@DougGregor DougGregor disabled auto-merge November 13, 2023 19:51
@DougGregor
Copy link
Member Author

A couple more cleanups peeled off this and put into #69824

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please build toolchain

@stephentyrone
Copy link
Contributor

Do we understand where the large changes in the two MapReduce benchmarks come from?

@eeckstein
Copy link
Contributor

It's probably because function signature optimization is disabled for indirect throws (Though I didn't verify that assumption)

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please smoke test macOS

@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@swift-ci please benchmark

@DougGregor
Copy link
Member Author

I've rebased this PR and am re-running all of the tests, now that SE-0413 has been accepted and typed throws has been enabled by default

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Replace rethrowing map with generic typed throws
9e4e9ea 
Adopt typed throws for the `map` operation to propagate thrown error
types through the `map` API. This is done in a manner that is backward
compatible for almost all cases:

* The new typed-throws entrypoint is `@_alwaysEmitIntoClient` so it
can back-deploy all the way back.
* The old `rethrows` entrypoint is left in place, using
`@usableFromInline` with `internal` so it is part of the ABI but not
the public interface, and `@_disfavoredOverload` so the type checker
avoids it while building the standard library itself. The old
entrypoint is implemented in terms of the new one.

Note that the implementation details for the existential collection
"box" classes rely on method overriding of `_map` operations, and the
types are frozen, so  we don't get to change their signatures. However,
these are only implementations, not API: the actual API `map`
functions can be upgraded to typed throws.

Note that this code makes use of a known hole in `rethrows` checking
to allow calling between `rethrows` and typed throws. We'll need to do
something about this for source-compatibility reasons, but I'll follow
up with that separately.
@DougGregor
Improve ABI-only rethrowing map shims based on code review
af79dd8 
Replace the hackish use of `@_disfavoredOverload` with the more principled
use of `@_silgen_name` for the entrypoint we are maintaining, then rename
these functions in source to `__rethrows_map` to indicate what they're for.

While here, make them `throws` instead of `rethrows`. The ABI is the
same, and `throws` allows us do avoid to do/catch tricks with rethrows
functions.
@DougGregor
[Existential collections] Switch internal _map rethrows methods to th…
304ce05 
…rows

These class methods are internal, but because they are overridden and are
part of a `@usableFromInline`, `@_fixed_layout` class, we they can't be
moved over to typed throws without breaking ABI. However, they are
only ever called from typed-throws functions, which already need a
do...catch dance to downcast the error itself. Make them `throws`
instead, which is ABI-compatible, but eliminates the need for
do...catch hackery in the function itself.
@DougGregor
Update tests for map switching to typed throws
f316c5f
@DougGregor
Avoid use of typed throws in SIL opaque values test
61579af 
SIL opaque values do not yet support typed throws, so avoid the API that
uses them.
@DougGregor
[Debug info] Map the thrown error type into context when generating d…
d21c649 
…ebug info
@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@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

swiftlang/swift-package-manager#7249

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

The swift-testing failure involves the C++ standard library; nothing to do with typed throws. Time to merge!

@DougGregor DougGregor merged commit 2557183 into swiftlang:main Jan 13, 2024
@DougGregor DougGregor deleted the typed-throws-map branch January 13, 2024 04:17
@tshortli tshortli mentioned this pull request Feb 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glessard glessard glessard left review comments

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@slavapestov slavapestov Awaiting requested review from slavapestov

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

@rintaro rintaro Awaiting requested review from rintaro rintaro is a code owner

@hamishknight hamishknight Awaiting requested review from hamishknight hamishknight is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

@adrian-prantl adrian-prantl Awaiting requested review from adrian-prantl adrian-prantl is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DougGregor @stephentyrone @tshortli @eeckstein @glessard