[c++-interop] For failed imports in ClangImporter, cache them regardless. #41173

plotfi · 2022-02-03T02:34:41Z

As per SR-14137 this caches entries in ImportedDecls even when the
import failed. The use of Optional<NullablePtr> is employed here
as a better way to track this than a raw nullptr.

Also have to mention I did this based on Thomas's PR #36747.

This in theory should help us better handle complex templates and
dependant types.

plotfi · 2022-02-03T02:35:01Z

@zoecarver @hyp @CodaFi

lib/ClangImporter/ImportDecl.cpp

zoecarver · 2022-02-03T21:18:13Z

Storing a Optional<NullablePtr> in the map of imported decls doesn't make sense. That already has two failures modes: 1) does the key exists? 2) Is the value null?

What you want is to return an Optional<NullablePtr> from importDeclCached. That way you can communicate both: 1) is there a cached thing here and 2) is that thing null?

I think you can just take Thomas' patch as-is and try to land it. IIRC there were some test failures, so the work in landing this is just debugging the test failures and landing it.

plotfi · 2022-02-03T22:12:10Z

What you want is to return an Optional<NullablePtr> from importDeclCached. That way you can communicate both: 1) is there a cached thing here and 2) is that thing null?

Updated patch, but no optionals at all. I can update importDeclCached to provide a tri-state through Optional<Decl *> where None means nothing was found and nullptr/non-nullptr means it either found a cached import-failure or a succeeded imported decl. IIRC Thomas' patch did not do this (it just returns None if there was a cached nullptr), which is why I think this confused me. I'll ping this PR once I rerun the test suite on this newest version.

plotfi · 2022-02-04T02:35:01Z

Storing a Optional<NullablePtr> in the map of imported decls doesn't make sense. That already has two failures modes: 1) does the key exists? 2) Is the value null?

What you want is to return an Optional<NullablePtr> from importDeclCached. That way you can communicate both: 1) is there a cached thing here and 2) is that thing null?

I think you can just take Thomas' patch as-is and try to land it. IIRC there were some test failures, so the work in landing this is just debugging the test failures and landing it.

Updated to provide the additional information about prior import failures from importDeclCached. I ran a ninja check-swift and I don't see any additional test failures.

lib/ClangImporter/ImportDecl.cpp

zoecarver · 2022-02-04T20:39:21Z

lib/ClangImporter/ImportDecl.cpp

+  auto cached = importDeclCached(contextDecl, version, useCanonicalDecl);
+  if (cached.hasValue() && cached.getValue()) {
+    return cached.getValue();
+  } else if (cached.hasValue()) {


ClangImporter/attr-swift_name.swift fails if we return null from here because the diagnostics code for swift_name_circular_context_import doesn't get hit to append the warning+notes:

/Users/plotfi/local/S/swift/test/ClangImporter/Inputs/custom-modules/ObjCIRExtras.h:78:52: warning: cycle detected while resolving 'MutuallyCircularNameA' in swift_name attribute for 'MutuallyCircularNameB' SWIFT_NAME(MutuallyCircularNameA.Inner) @interface MutuallyCircularNameB : NSObject @end /Users/plotfi/local/S/swift/test/ClangImporter/Inputs/custom-modules/ObjCIRExtras.h:77:52: note: while resolving 'MutuallyCircularNameB' in swift_name attribute for 'MutuallyCircularNameA' SWIFT_NAME(MutuallyCircularNameB.Inner) @interface MutuallyCircularNameA : NSObject @end /Users/plotfi/local/S/swift/test/ClangImporter/Inputs/custom-modules/ObjCIRExtras.h:78:52: note: please report this issue to the owners of 'ObjCIRExtras' SWIFT_NAME(MutuallyCircularNameA.Inner) @interface MutuallyCircularNameB : NSObject @end

Since this function will return nullptr from this point regardless, do we want to bail before emitting these warnings?

zoecarver · 2022-02-04T20:41:15Z

Updated patch, but no optionals at all. I can update importDeclCached to provide a tri-state through Optional<Decl *> where None means nothing was found and nullptr/non-nullptr means it either found a cached import-failure or a succeeded imported decl. IIRC Thomas' patch did not do this (it just returns None if there was a cached nullptr), which is why I think this confused me. I'll ping this PR once I rerun the test suite on this newest version.

Yeah makes sense. In this case, you can return Optional.some(nullptr) when we've cached a nullptr. Make sense?

zoecarver · 2022-02-04T20:41:31Z

This looks good, though. Thanks again, Puyan!

plotfi · 2022-02-07T21:24:24Z

@zoecarver How does this look? Do you approve of the test case modification? From debugging the flow of the importer with the nullptr caching I believe the change in number of swift_name_circular_context_import warnings is due to the caching.

plotfi · 2022-02-09T23:21:48Z

@zoecarver Do you think this patch needs more work? I've done some investigation on the swift_name_circular_context_import warnings and I am not sure the exact same ordering of warnings can be printed out with the added caching.

zoecarver · 2022-02-15T22:59:03Z

lib/ClangImporter/ImportDecl.cpp

@@ -3417,7 +3417,8 @@ namespace {
      auto alreadyImportedResult =
          Impl.ImportedDecls.find({decl->getCanonicalDecl(), getVersion()});
      if (alreadyImportedResult != Impl.ImportedDecls.end())
-        return alreadyImportedResult->second;
+        if (alreadyImportedResult->second)
+          return alreadyImportedResult->second;


We can't return nullptr here?

Maybe we can re-factor this to use Impl.importDeclCached?

lib/ClangImporter/ImportDecl.cpp

zoecarver · 2022-02-15T23:03:39Z

Do you approve of the test case modification?

I'll take a closer look, but it seems fine to me.

zoecarver · 2022-02-15T23:03:45Z

@swift-ci please test.

zoecarver · 2022-02-15T23:11:48Z

By the way this patch looks great. Seems like it doesn't touch most visitors which is awesome!

Thanks again for working on it.

plotfi · 2022-02-19T10:04:11Z

By the way this patch looks great. Seems like it doesn't touch most visitors which is awesome!

Thanks again for working on it.

@zoecarver I addressed your comments. For some of them I just altered isAcceptableResult to do the thing that makes most sense when a nullptr entry is found (if its not an AccessorDecl return false).

lib/ClangImporter/ImportDecl.cpp

…ess. As per SR-14137 this caches entries in ImportedDecls even when the import failed. Also have to mention I did this based on Thomas's PR swiftlang#36747. This should help us better handle complex templates and dependant types.

drodriguez · 2022-03-09T23:15:39Z

@swift-ci please smoke test

drodriguez · 2022-03-10T01:22:18Z

@swift-ci please test Linux platform

plotfi · 2022-03-10T18:23:06Z

@hyp @zoecarver Tests passing, ready to merge unless you all have more review comments.

hyp

LGTM, thanks

plotfi commented Feb 3, 2022

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Outdated Show resolved Hide resolved

plotfi commented Feb 3, 2022

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Show resolved Hide resolved

plotfi force-pushed the cxx-interop-cache-failed-imports-plotfi branch from 61e2ddb to 8c8a71d Compare February 3, 2022 22:05

zoecarver reviewed Feb 4, 2022

View reviewed changes

plotfi force-pushed the cxx-interop-cache-failed-imports-plotfi branch 2 times, most recently from 4d0401e to 69ace9b Compare February 7, 2022 06:34

zoecarver reviewed Feb 15, 2022

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Show resolved Hide resolved

zoecarver reviewed Feb 15, 2022

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Outdated Show resolved Hide resolved

zoecarver reviewed Feb 15, 2022

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Outdated Show resolved Hide resolved

plotfi force-pushed the cxx-interop-cache-failed-imports-plotfi branch 2 times, most recently from 691459d to f6c3181 Compare February 19, 2022 10:00

hyp reviewed Feb 23, 2022

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Outdated Show resolved Hide resolved

zoecarver assigned hyp Feb 28, 2022

plotfi force-pushed the cxx-interop-cache-failed-imports-plotfi branch from f6c3181 to 9faff45 Compare March 9, 2022 23:13

hyp approved these changes Mar 10, 2022

View reviewed changes

drodriguez merged commit 09cdd36 into swiftlang:main Mar 10, 2022

[c++-interop] For failed imports in ClangImporter, cache them regardless. #41173

[c++-interop] For failed imports in ClangImporter, cache them regardless. #41173

Uh oh!

Conversation

plotfi commented Feb 3, 2022

Uh oh!

plotfi commented Feb 3, 2022

Uh oh!

Uh oh!

Uh oh!

zoecarver commented Feb 3, 2022

Uh oh!

plotfi commented Feb 3, 2022

Uh oh!

plotfi commented Feb 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoecarver Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

plotfi Feb 6, 2022

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Feb 4, 2022

Uh oh!

zoecarver commented Feb 4, 2022

Uh oh!

plotfi commented Feb 7, 2022

Uh oh!

plotfi commented Feb 9, 2022

Uh oh!

zoecarver Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

zoecarver Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoecarver commented Feb 15, 2022

Uh oh!

zoecarver commented Feb 15, 2022

Uh oh!

zoecarver commented Feb 15, 2022

Uh oh!

plotfi commented Feb 19, 2022

Uh oh!

Uh oh!

drodriguez commented Mar 9, 2022

Uh oh!

drodriguez commented Mar 10, 2022

Uh oh!

plotfi commented Mar 10, 2022

Uh oh!

hyp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!