Implementation-only import checking for conformances #23808
Conversation
lib/Sema/TypeCheckAccess.cpp
Outdated
|
|
||
| SubstitutionMap subConformanceSubs = | ||
| concreteConf->getSubstitutions(SF.getParentModule()); | ||
| visitSubstitutionMap(subConformanceSubs); |
There was a problem hiding this comment.
This recursive bit was tricky; it's to handle cases like
public struct ConditionalGenericStruct<T> {}
extension ConditionalGenericStruct: NormalProto where T: NormalProto {
public typealias Assoc = Int
}
public func testConditionalGeneric(_: NormalProtoAssocHolder<ConditionalGenericStruct<NormalStruct>>) {}
// expected-error@-1 {{cannot use conformance of 'NormalStruct' to 'NormalProto' here; 'BADLibrary' has been imported as '@_implementationOnly'}}Am I doing this right?
There was a problem hiding this comment.
I think you need to visit the associated conformances too; imagine you have:
protocol Q {}
protocol P {
associatedtype T : Q
}
struct S1 : P {
typealias T = S2
}
Let's say that S2 : Q is implementation only. I think your current check will miss that. Note that checking associated conformances should subsume your check.
There was a problem hiding this comment.
Also I'm worried about infinite recursion here. @DougGregor might have a better idea of the 'canonical' way to visit everything you need exactly once.
There was a problem hiding this comment.
I check associated conformances when doing conformance checking; this section is type signature checking. But I didn't think about infinite recursion in either case, so I'll need to pay more attention there.
There was a problem hiding this comment.
Okay, I think this is okay because substitution conformances always bottom out, even though associated conformances do not. And I don't need to check the associated conformances of my associated conformances because you can always check a conformance on its own while assuming the others are valid and then move on to the next one.
| bool treatUsableFromInlineAsPublic); | ||
| /// Walks a Type to find all NominalTypes, BoundGenericTypes, and | ||
| /// TypeAliasTypes. | ||
| class TypeDeclFinder : public TypeWalker { |
There was a problem hiding this comment.
Maybe these should live in their own header file?
There was a problem hiding this comment.
Yeah, if anything the original AccessScopeChecker should just sink into TypeCheckAccess.cpp. It's not reusable after all.
There was a problem hiding this comment.
I'll come back and fix this in a separate PR, since it doesn't strictly need to go into 5.1.
| /// the generic type. | ||
| class SimpleTypeDeclFinder : public TypeDeclFinder { | ||
| /// The function to call when a ComponentIdentTypeRepr is seen. | ||
| llvm::function_ref<Action(const TypeDecl *)> Callback; |
There was a problem hiding this comment.
I would feel more confident if this was an std::function since then you don't have to be sure to keep the closure alive somewhere else
There was a problem hiding this comment.
:-/ std::function has a definite cost, though, and doesn't inline away nicely. I'd rather keep this as is.
lib/Sema/TypeCheckAccess.cpp
Outdated
|
|
||
| SubstitutionMap subConformanceSubs = | ||
| concreteConf->getSubstitutions(SF.getParentModule()); | ||
| visitSubstitutionMap(subConformanceSubs); |
There was a problem hiding this comment.
I think you need to visit the associated conformances too; imagine you have:
protocol Q {}
protocol P {
associatedtype T : Q
}
struct S1 : P {
typealias T = S2
}
Let's say that S2 : Q is implementation only. I think your current check will miss that. Note that checking associated conformances should subsume your check.
lib/Sema/TypeCheckAccess.cpp
Outdated
|
|
||
| SubstitutionMap subConformanceSubs = | ||
| concreteConf->getSubstitutions(SF.getParentModule()); | ||
| visitSubstitutionMap(subConformanceSubs); |
There was a problem hiding this comment.
Also I'm worried about infinite recursion here. @DougGregor might have a better idea of the 'canonical' way to visit everything you need exactly once.
lib/Sema/TypeCheckAccess.cpp
Outdated
| // | ||
| // We still don't want to do this if we found issues with the TypeRepr, | ||
| // though, because that would result in some issues being reported twice. | ||
| if (!foundAnyIssues && type) { |
There was a problem hiding this comment.
The code might be easier to follow with an early return here.
lib/Sema/TypeCheckAccess.cpp
Outdated
| if (accessScope.isPublic()) | ||
| return false; | ||
|
|
||
| // Is this a stored property in a non-resilent struct or class? |
There was a problem hiding this comment.
Nit: "resilent" has a typo.
lib/Sema/TypeCheckAccess.cpp
Outdated
| auto *parentNominal = dyn_cast<NominalTypeDecl>(property->getDeclContext()); | ||
| if (!parentNominal || parentNominal->isResilient()) | ||
| return true; | ||
| return shouldSkipChecking(parentNominal); |
There was a problem hiding this comment.
Recursing here is clever, but I'm not happy with how it makes the code read:
- I had to stop and double-check that skipping the recursive call for everything but stored properties was an intentional behavior. I don't think it's a bug, but it looks like a bug.
- You don't really need the full generality of recursion because a
NominalTypeDeclwill always hit one of the first twoifs, so it makes it look like this might recurse up through the ancestor contexts, even though it actually doesn't.
There are a couple of ways you could address the first point, but in light of the second, I'd suggest extracting an isPublicOrUsableFromInline() helper function and calling that instead of recursing:
static bool isPublicOrUsableFromInline(const ValueDecl *VD) {
return VD->getFormalAccessScope(nullptr,
/*treatUsableFromInlineAsPublic*/true)
.isPublic();
}
static bool shouldSkipChecking(const ValueDecl *VD) {
// Is this part of the module's API or ABI?
if (isPublicOrUsableFromInline(VD))
return false;
// Is this a stored property in a non-resilient struct or class which is
// part of the module's ABI or API?
auto *property = dyn_cast<VarDecl>(VD);
if (!property || !property->hasStorage() || property->isStatic())
return true;
auto *parentNominal = dyn_cast<NominalTypeDecl>(property->getDeclContext());
if (!parentNominal || parentNominal->isResilient())
return true;
return !isPublicOrUsableFromInline(parentNominal);
}There was a problem hiding this comment.
@slavapestov We do keep running into that helper. Do we want an isFormallyABI or something?
lib/Sema/TypeCheckAccess.cpp
Outdated
| if (!parentNominal || parentNominal->isResilient()) | ||
| return true; | ||
| return shouldSkipChecking(parentNominal); | ||
| }; |
lib/Sema/TypeCheckAccess.cpp
Outdated
| return; | ||
| // Only check individual variables if we didn't check an enclosing | ||
| // TypedPattern. | ||
| if (seenVars.count(theVar) || theVar->isInvalid()) |
There was a problem hiding this comment.
It's really unfortunate that Pattern::forEachNode() doesn't let you skip recursing into the children of a node, but I guess using an ASTWalker instead would be even more painful.
There was a problem hiding this comment.
Yeah, and it's honestly not going to be a very deep tree in practice.
| public var (testBadTypeTuple1, testBadTypeTuple2): (BadStruct?, BadClass?) = (nil, nil) | ||
| // expected-error@-1 {{cannot use 'BadStruct' here; 'BADLibrary' has been imported as '@_implementationOnly'}} | ||
| // expected-error@-2 {{cannot use 'BadClass' here; 'BADLibrary' has been imported as '@_implementationOnly'}} | ||
| public var (testBadTypeTuplePartlyInferred1, testBadTypeTuplePartlyInferred2): (Optional, Optional) = (Optional<Int>.none, Optional<BadStruct>.none) // expected-error {{cannot use 'BadStruct' here; 'BADLibrary' has been imported as '@_implementationOnly'}} |
There was a problem hiding this comment.
Note: checkTypedPattern() checks the last VarDecl, which is the one with the bad type in this test case.
There was a problem hiding this comment.
I guess I should try some more orders, yeah.
| private var computedIsOkay: BadStruct? { return nil } // okay | ||
| private static var staticIsOkay: BadStruct? // okay | ||
| @usableFromInline internal var computedUFIIsNot: BadStruct? { return nil } // expected-error {{cannot use 'BadStruct' here; 'BADLibrary' has been imported as '@_implementationOnly'}} | ||
| } |
There was a problem hiding this comment.
The other test file includes test cases with structural types, but this one doesn't. Is that okay? I'm not sure what this file is trying to test.
There was a problem hiding this comment.
This file tests the exception for non-frozen structs and classes when library evolution is enabled. It assumes everything works otherwise; it's just testing the frozen/non-frozen distinction.
There was a problem hiding this comment.
A comment would make that clearer.
jrose-apple
force-pushed
the
conformance-conformance
branch
from
ecdedbd
to
9c60cae
Compare
|
@swift-ci Please test source compatibility |
jrose-apple
force-pushed
the
conformance-conformance
branch
from
9c60cae
to
1d0c380
Compare
|
Okay, I think this is ready. I removed a bunch of duplication from the last commit and now things aren't perfect but they're at least not terribly bad. What test cases am I missing? Leaving marked as a draft because it still slurps in #23912, so I have to do one more rebase. I'll do a follow-up PR that harmonizes John's and my terminology, since right now half of the stuff says "exportability" and the other half says "implementation-only" or "implementationOnly"; and another one that renames AccessScopeChecker.h to TypeDeclFinder.h (and pulls the access control walking out) as requested by Slava. @swift-ci Please test |
|
@swift-ci Please test compiler performance |
|
@swift-ci Please test source compatibility |
|
Build failed |
jrose-apple
changed the title
jrose-apple
requested review from
beccadax and
slavapestov
and removed request for
beccadax
|
@swift-ci Please test compiler performance |
|
Build failed |
|
Failed in, um, the very thing I'm testing. Maybe I accidentally lost some changes when rebasing? |
These also create a dependency on the implementation module, even if both the type and the protocol are public. As John puts it, a conformance is basically a declaration that we name as part of another declaration. More rdar://problem/48991061
Okay, strictly we're checking the "signature conformances" of a newly- declared conformance, but that wouldn't have fit on one line. This is making sure that we don't have a requirement on an associated type (or on the conforming type) that can only be satisfied using a conformance in an implementation-only import.
This will be used in the next commit for checking conformance use in inlinable function bodies, but for now it's No Functionality Change.
This includes both the types and the values (generic functions, etc) used in the inlinable code. We get some effectively duplicate diagnostics at this point because of this, but we can deal with that at a future date. Last part of rdar://problem/48991061
jrose-apple
force-pushed
the
conformance-conformance
branch
from
1d0c380
to
7006aa0
Compare
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test macOS |
| } | ||
| extension PublicInferredAssociatedTypeImpl: PublicInferredAssociatedType {} // expected-error {{cannot use conformance of 'NormalStruct' to 'NormalProto' in associated type 'Self.Assoc' (inferred as 'NormalStruct'); 'BADLibrary' has been imported as '@_implementationOnly'}} | ||
| extension PublicInferredAssociatedTypeImpl: UFIInferredAssociatedType {} // expected-error {{cannot use conformance of 'NormalStruct' to 'NormalProto' in associated type 'Self.Assoc' (inferred as 'NormalStruct'); 'BADLibrary' has been imported as '@_implementationOnly'}} | ||
| extension PublicInferredAssociatedTypeImpl: InternalInferredAssociatedType {} // okay |
There was a problem hiding this comment.
What error would we emit for something like this?
public struct PublicExplicitAssociatedTypeImpl {
public typealias Assoc = NormalStruct
public func takesAssoc(_: Assoc) {}
}
extension PublicExplicitAssociatedTypeImpl: PublicInferredAssociatedType {}There was a problem hiding this comment.
Hm, should look the same, but worth adding as a test case. Thanks!
| // for the type, and therefore we could skip this extra work? | ||
| if (!foundAnyIssues && type) { | ||
| type.walk(SimpleTypeDeclFinder([&](const TypeDecl *typeDecl) { | ||
| // Note that if we have a type, we can't skip checking it even if the |
There was a problem hiding this comment.
In theory we could also skip walking the type if our source file doesn't import anything as implementation-only, but that might be too much conditionalizing for little gain.
There was a problem hiding this comment.
Yeah, John had his part skipping. Preliminary "test compiler performance" didn't show any significant difference, but it's probably worth following up here to make sure that really is the case.
| } | ||
|
|
||
| Action visitNominalType(NominalType *ty) override { | ||
| visitTypeDecl(ty->getDecl()); |
There was a problem hiding this comment.
@AnthonyLatsis's PR adds where clauses for types without generic parameter lists, so you should just duplicate the bound generic type logic here; the nominal type might add a conformance that's not there on the parent type.
We could also skip visiting the parent type as an optimization but I don't know how.
(I'd really like to merge NominalType/BoundGenericType into a single type, remove the parent type concept altogether, and store the SubstitutionMap directly inside the type...)
There was a problem hiding this comment.
Uh, hm. Nominal types don't carry that information, so unless the current DeclContext is now relevant I don't see how the current logic will work with that. Are you sure that's something that can come up in this part of the code? (as opposed to being checked at the FuncDecl or whatever when we walk the requirements)
| @@ -0,0 +1,263 @@ | |||
| // RUN: %empty-directory(%t) | |||
There was a problem hiding this comment.
Can you add some tests where a base class has an implementation-only conformance to a protocol, and the derived class appears in a substitution map somewhere against a conformance requirement to the protocol? This is where you get an InheritedProtocolConformance and we should make sure that works too.
There was a problem hiding this comment.
I think I had them in the other file (the non-inlinable tests) but I'll add some here too.
|
I'm going to merge this as is so I can start PR testing for the 5.1 branch, but I'll make these additional changes along with the general follow-up cleanup work. |
…rmance Implementation-only import checking for conformances (cherry picked from commit 6ea773e)
Continues building on John's #23702 and my #23722 to start checking the use of conformances. These also create a dependency on the implementation module, even if both the type and the protocol are public. As John puts it, a conformance is basically a declaration that we name as part of another declaration.
More rdar://problem/48991061