Resilience diagnostics #6669
Resilience diagnostics #6669
Conversation
|
@swift-ci Please test |
|
Build failed |
slavapestov
force-pushed
the
resilience-diagnostics
branch
from
16631b7 to
04c52ed
Compare
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test macOS |
|
Build failed |
|
Build failed |
Referencing private or internal declarations from an @inline(__always) body can lead to SIL verifier and linker failures, so now we diagnose and prohibit it. Mark the entity in question public, or if it is not part of user-visible API, internal and @_versioned. See <swiftlang/swift#6669>.
|
@swift-ci Please test |
|
Build failed |
slavapestov
force-pushed
the
resilience-diagnostics
branch
2 times, most recently
from
eb103c9 to
a46e17a
Compare
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test Linux |
|
Build failed |
There was a problem hiding this comment.
Glad to see diagnostics coming in! I'm a little uncomfortable to see this being defined in terms of ResilienceExpansion, though. I maintain that we're going to need AvailabilityContext as soon as we have versioned attributes, and I'd rather see ResilienceExpansion squashed and rebuilt from scratched than extended to match our current resilience model. (…because I don't feel like there is a natural way to do such an extension.)
| }; | ||
|
|
||
| /// If the body is able to be inlined into functions in other resilience | ||
| /// domains, this ensures that only sufficiently-conservative access patterns |
There was a problem hiding this comment.
Nitpick: "access patterns" isn't a Sema-level term.
There was a problem hiding this comment.
I just removed the comment since it got copy-pasted from getResilienceExpansion() on accident.
| @@ -4,18 +4,6 @@ | |||
| // RUN: %target-swift-frontend -O -primary-file %s %S/Inputs/devirt_access_helper.swift -I %t -emit-sil -sil-inline-threshold 1000 -sil-verify-all | %FileCheck -check-prefix=WHOLE-MODULE %s | |||
| // RUN: %target-swift-frontend -O -primary-file %s %S/Inputs/devirt_access_helper.swift -I %t -emit-sil -sil-inline-threshold 1000 -sil-verify-all | %FileCheck -check-prefix=PRIMARY-FILE %s | |||
|
|
|||
| import devirt_access_other_module | |||
There was a problem hiding this comment.
I don't think it's a good idea to just delete part of the test. Maybe run it under explicit non-resilient mode?
There was a problem hiding this comment.
The "explicit non-resilient" mode would just disable the new diagnostics. I'd be OK with making them conditional if we really had to, but I think basically this test was just exercising that the SIL optimizer doesn't blow up in a case that no longer type checks.
| (DeclName, unsigned)) | ||
|
|
||
| ERROR(resilience_decl_unavailable, | ||
| none, "%0 %1 is %select{internal|private}2 and cannot be referenced " |
There was a problem hiding this comment.
This leaves out fileprivate. Can you use Accessibility as the argument here, even if public and open don't make sense?
| ERROR(versioned_attr_with_explicit_accessibility, | ||
| none, "'@_versioned' attribute can only be applied to internal " | ||
| "declarations, but %0 is %select{private|fileprivate|internal|public}1", | ||
| (Identifier, unsigned)) |
There was a problem hiding this comment.
This should use Accessibility as the argument kind, and I suggest also capitalizing "INTERNAL" and "PUBLIC" (and "OPEN"?) to show that they should never occur.
Neither of the following is supported:
func g<T>(_: T) {
_ = {
struct S {}
}
}
struct G<T> {
let fn = {
struct S {}
}
}
Even though nested generic types are supported, the second example
is more like a type inside a function than a type inside a type,
because 'S' has no parent type.
Technically this is source-breaking but since neither SILGen nor
IRGen knew how to generate code for these I doubt anything worked.
slavapestov
force-pushed
the
resilience-diagnostics
branch
from
a46e17a to
0438ce0
Compare
…rguments model Piggybacks some resilience diagnostics onto the availability checking code. Public and versioned functions with inlineable bodies can only reference other public and internal entities, since the SIL code for the function body is serialized and stored as part of the module. This includes @_transparent functions, @_inlineable functions, accessors for @_inlineable storage, @inline(__always) functions, and in Swift 4 mode, default argument expressions. The new checks are a source-breaking change, however we don't guarantee source compatibility for underscored attributes. The new ABI and tests for the default argument model will come in subsequent commits.
slavapestov
force-pushed
the
resilience-diagnostics
branch
from
0438ce0 to
d65d1d2
Compare
|
@jrose-apple I plan on re-visiting the whole ResilienceExpansion thing soon. I wanted to prototype the semantics and get some tests in first, though. I made the other fixes you suggested -- are you OK with this getting merged? |
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test |
|
Build failed |
|
Yeah, I guess it's more important to start diagnosing things right now. I'll admit part of my discomfort is that ResilienceExpansion doesn't naturally make sense to me, even though it's such a simple enum. But that part shouldn't block you. |
rdar://problem/29164469
rdar://problem/23522157