Fix long-standing soundness hole in rethrows checking

[slavapestov]

A rethrows function would still be considered rethrows if one of
its argument functions unconditionally throws. This meant that we
would accept the following, even though it was invalid:

enum MyError : Error {
  case bad
}
func rethrowsViaClosure(_ fn: () throws -> ()) rethrows {
  try fn()
}
func invalidRethrows(_ fn: () throws -> ()) rethrows {
  try rethrowsViaClosure { throw MyError.bad }
}

invalidRethrows()

The problem was simple. When visiting a function argument in
rethrows analysis, we would always just drop the result of
the classification walk on the floor, which meant that we
returned ThrowingKind::RethrowingOnly instead of possibly
returning ThrowingKind::Throws.

Since the Dispatch overlay was relying on this hole in the
implementation of DispatchQueue.sync, I also added a
@_rethrowsUnchecked attribute to bypass rethrows checking
entirely for rare cases like this.

Fixes https://bugs.swift.org/browse/SR-680 / rdar://problem/27868617.

[slavapestov]

@swift-ci Please smoke test

[DougGregor]

Amazing. Thanks!

[slavapestov]

@swift-ci Please test

[slavapestov]

@swift-ci Please test source compatibility

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - da2b0cd

[slavapestov]

apple/swift-corelibs-foundation#2983
@swift-ci Please test Linux

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - da2b0cd

[slavapestov]

apple/swift-corelibs-libdispatch#558
apple/swift-corelibs-foundation#2983
@swift-ci Please test Linux

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - da2b0cd

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - da2b0cd

[xwu]

The methods changed in Dispatch and Foundation seem like very reasonable things for end users to do also. While surely rare, my understanding of rethrows was that it was always meant to allow some amount of reprocessing of the error, as long as one only throws if the given predicate throws.

Is there no less invasive change to the compiler which wouldn't prohibit these legitimate uses without depending on an underscored attribute?

[slavapestov]

@xwu They're reasonable things to do, but the workaround people used was kind of silly and just emergent behavior from this bug. You could do this:

func f(_ fn: () throws -> ()) rethrows {
  try fn()
}

func g(_ fn: () throws -> ()) rethrows {
  f { throw foo }
}

But not this:

func f(_ fn: () throws -> ()) rethrows {
  throw foo
}

If the bug hadn't existed in the first place, nobody would be relying on it the way they are today.

The reason that the bug is bad is that you can introduce a runtime crash without realizing it through incorrect usage of rethrows. An analogous situation would be if the compiler accepted an invalid conversion that might sometimes crash due to some weird edge case in the constraint solver, and people started relying on it instead of using as! explicitly. That would also be undesirable, even though unsafe type coercion is surely something that people need to do.

Instead of an underscored attribute, we could instead formalize it by introducing a new rethrows(unsafe) spelling in the function signature.

[slavapestov]

Looks like GRDB in the source compat suite relies on this bug also:

  /Users/buildnode/jenkins/workspace/swift-PR-source-compat-suite-debug/swift-source-compat-suite/project_cache/GRDB.swift/GRDB/Core/DatabaseQueue.swift:167:20: error: call can throw, but the error is not handled; a function declared 'rethrows' may only throw if its parameter does
        return try writer.sync { db in
                   ^

In this case though, it doesn't appear they're doing anything unsafe. Instead, they're calling a throws function that should really be rethrows.

[slavapestov]

@xwu I wrote up a pitch for rethrows(unsafe) -- feedback appreciated: https://forums.swift.org/t/pitch-fix-rethrows-checking-and-add-rethrows-unsafe/44863

[grynspan]

It would be valuable to land this attribute before the checker changes so that code that relies on the current behaviour (such as DispatchQueue) has an opportunity to migrate.

[grynspan]

Instead of an underscored attribute, we could instead formalize it by introducing a new rethrows(unsafe) spelling in the function signature.

This would make sense to me—that, or some scoped bridging mechanism to allow an engineer to say "I'm going to call an intermediate function that can't throw an error itself (e.g. because it's @convention(c)), but which may need to catch an error thrown by the passed closure. Trust me." Which is of course what the new attribute does, but having a with-like function might be better from a readability standpoint (and would allow us to limit the scope of such behaviour to a specific subsection of the rethrowing function's body rather than the whole body.)

If nothing else, we could reserve the @_rethrowsUnchecked attribute for this with-like function only and tell everyone else to call it and not use the attribute directly.

Edit: I commented in the rethrows(unchecked) pitch as well.