[cxx-interop] Use Clang lookup for type members when deriving Clang conformances #85996
+474
−400
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.
Contributor
Author
|
@swift-ci please test |
j-hui
force-pushed
the
clang-lookups-for-clang-conformances
branch
from
c664a65 to
7c779a1
Compare
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.
j-hui
force-pushed
the
clang-lookups-for-clang-conformances
branch
from
7c779a1 to
6ed9efb
Compare
Contributor
Author
|
@swift-ci please test |
1 similar comment
Contributor
Author
|
@swift-ci please test |
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.
j-hui
force-pushed
the
clang-lookups-for-clang-conformances
branch
from
c4d1535 to
b636336
Compare
Contributor
Author
|
@swift-ci please test |
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.
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.
This patch set refactors the conformance logic in ClangDerivedConformances.cpp to use primarily use Clang lookups to find the member typedefs and functions needed to synthesize the typealiases used as associated types for each conformance. Doing so removes its reliance on the eager member importing behavior of the SwiftDeclConverter, and brings us a step closer toward making it lazier.
In particular, for each of the standard library types, the conformance logic is rewritten to uniformly follow this structure:
This greatly simplifies and unifies the control flow of each conformance routine.
Any eager conformance-checking logic is removed here, e.g., that a
std::vector'siteratorconforms toCxxIterator. This further simplifies the derived conformance control flow. We optimistically assume that if a type is namedstd::CONTAINERthen it conforms to the standard. This conformance-checking was not being done consistently for each requirement, and it's not clear what value it brought, since we do not emit any diagnostics to warn the user that we are appear to be importing some non-standard-conformingstd-type. In any case, checking that associated types conform to requirements should be (and maybe already is?) lazier, and performed only when we are checking conformance for the CxxStdlib protocol we're currently synthesizing.Patching is performed on
std::optional,std::span, andstd::map. In the first two cases, we are add a constructor, whereas in the latter case, we are marking the imported subscript method as unavailable so that theCxxDictionarycan provide a safer overload.This refactoring doesn't yet address the Clang derived conformances for
Sequence,Iterator, andConvertibleToBool, which are performed on every type. As of this refactoring still rely on eager import behavior, though only on eagerly imported operators.None of this refactoring is intended to change any behavior, which is why I have not added tests.