[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? |
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.
There was a problem hiding this comment.
That is what we used to do, but caching makes it quite difficult to do this correctly.
Consider:
template <typename, typename> struct TTake2 {};
template <typename T> struct PassThru {};
struct IsUnsafe { int *p; };
struct HasUnsafe : TTake2<PassThru<HasUnsafe>, IsUnsafe> {};
using AlsoUnsafe = PassThru<HasUnsafe>;Upon visual inspection, we can see that HasUnsafe should be unsafe because it inherits from TTake2<PassThru<HasUnsafe>, IsUnsafe>, which is unsafe because its second template type parameter is unsafe. But we might not know that while we're checking the safety of the first template parameter, PassThru<HasUnsafe>.
When we look through PassThru<HasUnsafe> and arrive at its template parameter, we need to break the cycle and assume that safety is unspecified for the time being. We will later discover that HasUnsafe is actually unsafe via its second template parameter. However, if we are caching intermediate results, then we will incorrectly remember that PassThru<HasUnsafe> is unspecified, which is incorrect because it has an unsafe template parameter.
It's possible to make this work with conditional caching, but it is not clear to me that the added complexity is worth it.
|
Also unrelated: |
|
@swift-ci please test windows platform |
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