[cxx-interop] Do not eagerly import non-field Clang members #86048
+510
−407
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
getClangOwningModule() no longer takes a parameter named `MI` nor does it return an optional (it just returns a poitner).
This refactoring restructures the code in a couple of ways: - move all the "conformToXXXIfNeeded()" functions into a single function and remove them from the ClangDerivedConformances.h interface - unify the check for isInStdNamespace() for the C++ stdlib types - unify the name comparisons into a single llvm::StringSwitch - unify redundant nullptr assert()s, and upgrade them to ASSERT() - move the known stdlib type name-checking logic out of the "conformToCxxSTDLIBTYPEIfNeeded()" functions, and rename them to drop the "IfNeeded" suffix. This patch also introduces a local CxxStdType enum class that enumerates (and thus documents) all of the known (and thus supported) types from the C++ stdlib types. The ultimate goal is to organize the code such that by the time we call "conformToCxxSTDLIBTYPE()" we are already sure that the type is (or at least claims to be, based on its name and DeclContext) the C++ stdlib type we think it is, rather than interleaving such checks with the conformance synthesis logic. The main observable difference should be the logging: now, we will no longer have PrettyStackTraceDecls logging for C++ stdlib types like "conforming to CxxVector" unless we are already certain we are looking at a relevant type, e.g., std::vector. Otherwise, the functional behavior of this code should be the same as before.
This is the first patch of a series of patches intended to shift lookup toward using Clang lookup, to avoid Swift lookup's dependency on eager Clang member importation. The lookupCxxTypeMember() helper function replaces the previous helper function, lookupNestedClangTypeDecl(). Functionally, the main difference is that lookupCxxTypeMember() uses clang::Sema::LookupQualifiedName(), which takes cares of issues like inheritance, ambiguity, etc. Meanwhile, lookupNestedClangTypeDecl() only used clang::DeclContext::lookup(), which is limited to the lexical context, and does not work with inheritance. The isIterator() function is renamed to hasIteratorConcept() to better reflect what it is checking, and still uses clang::DeclContext::lookup() to preserve the original behavior and to avoid requiring its callers to supply a clang::Sema instance.
This is slightly easier to read.
Instead of looking up the *already imported* Swift members of std::set, this patch shifts the conformance synthesis logic to look up the equivalent members from Clang, and *then* imports them to Swift types to satisfy the CxxSet conformance. Doing so removes the logic's dependency on eagerly importing such members.
Instead of looking up the *imported* first_type and second_type Swift type aliases, this patch shifts the conformance to look up these typedefs from Clang, and *then* imports them to Swift types to satisfy the CxxPair conformance. Doing so removes the conformance's dependency on eagerly importing such typedefs.
Instead of looking up the *imported* Swift type aliases, this patch shifts the conformance to look up these typedefs from Clang, and *then* imports them to Swift types to satisfy CxxVector conformance. Doing so removes the conformance's dependency on eagerly importing such typedefs. This patch also drops a conformance check that RawIterator conforms to UnsafeCxxRandomAccessIterator. It shouldn't be necessary, because we are looking at std::vector, which we assume comes from a conforming stdlib. Even if it were necessary, it's not clear that this is the right place and strategy for doing conformance checking. Besides we are fairly inconsistent about checking other associated types. Let's be optimistic about conformance for now (-:
Instead of looking up the *imported* Swift type aliases, this patch shifts the conformance to look up these typedefs from Clang, and *then* imports them to Swift types to satisfy CxxVector conformance. Doing so removes the conformance's dependency on eagerly importing such typedefs. There is one remaining dependency on eager imports, which is the unavailability patching we do for the subscript operator. This patch also drops a conformance check that the iterators conform to the our Cxx iterator protocols. It shouldn't be necessary, because we are looking at std::map, which we assume comes from a conforming stdlib.
This is no longer needed now that we are looking up the insert function via clang::Sema:::LookupQualifiedName
Instead of looking up the *imported* Swift type aliases, this patch shifts the conformance to look up these typedefs from Clang, and *then* imports them to Swift types to satisfy CxxSpan conformance. Doing so removes the conformance's dependency on eagerly importing such typedefs.
Instead of looking up the Wrapped type from the type of the computed variable .pointee, this patch shifts the conformance logic to look up the value_type typedef from Clang, and uses the imported version of that to to satsify the CxxOptional conformance. Doing so removes the conformance's dependency on eagerly importing the .pointee method, and makes the conformance logic consistent with the rest of the CxxStdlib.
Skipping these members makes importing structs much lazier, and greatly reduces the chance we run into spurious template instantiation errors. rdar://145238539&152542137
There isn't a very good reason _not_ to import the return type when we already import the argument types, and can occasionally lead to the return type being misinterpreted as void when our heuristics are wrong. Even if we're not binding the return value in Swift, we still need to, say, call its destructor. rdar://139225705
Contributor
Author
|
@swift-ci please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Skipping these members makes importing structs much lazier, and greatly
reduces the chance we run into spurious template instantiation errors.
It also allows us to always import the return type of functions.