[cxx-interop] Do not import template type arguments #85379
Conversation
j-hui
requested review from
Xazax-hun,
beccadax,
egorzhdan,
hnrklssn,
ian-twilightcoder and
susmonteiro
as code owners
|
Could you elaborate on the differences between this and the previous patches? |
Checking this upon import may overlook annotations that explicitly mark a type as safe (or unsafe). Instead, we consolidate this logic in the ClangDeclExplicitSafety request. rdar://163196609
Prior to this patch, we eagerly imported template type arguments. A consequence of doing so is that we over-instantiate (potentially unused) type arguments, which causes unnecessary errors. The only apparent use we have for these type arguments are to check whether they are unsafe, so that we can mark the instantiated type as unsafe as well... even if the instantiated type does not use its unsafe template type argument at all. Using un-instantiatable types in template type arguments is supported in C++. The test case included in this patch validates this behavior, for both missing member and incomplete type errors. Note, also, that as of this patch, dumping the module interface of CxxModule actually causes swift-ide-test to emit compiler errors, since it tries to instantiate the invalid types MissingMember<Empty> and IncompleteField<Incomplete>. However, these errors are simply swallowed by swift-ide-test, so they should be harmless, though we should probably get rid of them entirely in future work. rdar://145238539
j-hui
force-pushed
the
dont-import-template-type-arguments-round-2
branch
from
c24d10f to
3255ffb
Compare
|
This patch re-lands the patches reverted in #85296. Note that there were no known issues with that patch. The main difference in this patch is that it eliminates the |
|
@swift-ci please test |
Was it this request that caused the cycle mentioned in #85296, or was it some other patch that has now been fixed? |
lib/ClangImporter/ClangImporter.cpp
Outdated
| } | ||
| auto req = ClangDeclExplicitSafety({recordDecl, false}); | ||
| if (evaluator.hasActiveRequest(req)) | ||
| // Normally, using hasActiveRequest() to avoid cycles is an anti-pattern |
There was a problem hiding this comment.
It’s still an anti-pattern and nothing else outside of some old hacks in lib/AST and lib/Sema calls it today. Did you get a chance to investigate the possibility of plumbing a DenseSet<…> visited through here instead?
There was a problem hiding this comment.
It would be good if the doc comment for hasActiveRequest mentioned this and explained what the issue with it is, so that new contributors are guided in the right direction. Right now it's really unclear that it's not meant to be used.
There was a problem hiding this comment.
Did you get a chance to investigate the possibility of plumbing a
DenseSet<…> visitedthrough here instead?
I did, but that ended up causing more problems of its own, e.g., whether/how to cache the DenseSet<...> that now needs to be part of the request descriptor, etc.
I've rewritten the request to be non-recursive.
It would be good if the doc comment for hasActiveRequest mentioned this and explained what the issue with it is, so that new contributors are guided in the right direction. Right now it's really unclear that it's not meant to be used.
I agree. I've added a comment.
There was a problem hiding this comment.
I did, but that ended up causing more problems of its own, e.g., whether/how to cache the DenseSet<...> that now needs to be part of the request descriptor, etc.
I've rewritten the request to be non-recursive.
Rewriting it to be non-recursive is what I meant, not passing the visited set as a parameter to the request. Sorry for the confusion. Thanks!
Doing so reduces opportunities for caching, but it's not obvious to me that we benefit from such fine-grained caching. The benefit here is that we also avoid the pitfalls of incremental caching, as illustrated by the added test case. Finally, we eliminate the use of hasActiveRequest(), which is an anti-pattern.
|
@swift-ci please test |
…icitSafetyDescriptor Keep the naming convention consistent; this isn't specific to Cxx
|
@swift-ci please test |
| // If desc.decl doesn't have a definition, safety is unspecified. | ||
| return ExplicitSafety::Unspecified; |
There was a problem hiding this comment.
If a decl has a field that has unspecified safety, and another field that is explicitly unsafe, is ExplicitSafety::Unspecified the right value to return?
There was a problem hiding this comment.
This early return only triggers when decl == desc.decl, i.e., when the "origin" decl of the request lacks a definition.
Here, if decl is from a field, it should not equal desc.decl.
Seems unrelated |
|
@swift-ci please test windows platform |
| stack.push_back(desc.decl); | ||
| seen.insert(desc.decl); | ||
| while (!stack.empty()) { | ||
| const clang::Decl *decl = stack.back(); |
There was a problem hiding this comment.
Could we end up visiting the same decl multiple time across different requests?
I wonder if we should also cache the responses explicitly for all the decls that we visited and computed safety for.
5 participants
Prior to this patch, we eagerly imported template type arguments.
A consequence of doing so is that we over-instantiate (potentially
unused) type arguments, which causes unnecessary errors. This patch
skips importing those types.
These types were previously used to determine whether an imported
type is unsafe. This patch moves that check into the request that
checks for Clang decl safety.
rdar://145238539
rdar://163196609