[cxx-interop] Shared references are considered safe

[Xazax-hun]

This patch makes sure we don't get warnings in strict memory safe mode when using shared references. Those types are reference counted so we are unlikely to run into lifetime errors.

rdar://151039766

[egorzhdan]

Would this also make SWIFT_IMMORTAL_REFERENCE and SWIFT_UNSAFE_REFERENCE types safe in Swift?

[Xazax-hun]

Would this also make SWIFT_IMMORTAL_REFERENCE and SWIFT_UNSAFE_REFERENCE types safe in Swift?

SWIFT_IMMORTAL_REFERENCE yes, SWIFT_UNSAFE_REFERENCE no. The latter is explicitly marked as unsafe which takes precedence over the rule in this PR. Do you want me to change SWIFT_IMMORTAL_REFERENCE?

[fahadnayyar]

I think just checking if __attribute__((swift_attr("import_reference"))) is on the decl won't fix the issue for inherited SWIFT_SHARED_REFERENCE types.

In a follow-up patch, could you please check if the decl is a foreign reference type using CxxRecordSemantics like this:

  auto semanticsKind = evaluateOrDefault(
      importerImpl->SwiftContext.evaluator,
      CxxRecordSemantics({decl, importerImpl->SwiftContext, importerImpl,
                          /* shouldDiagnoseLifetimeOperations */ false}),
      {});
  return semanticsKind == CxxRecordSemanticsKind::Reference;

You can also take a look at importer::recordHasReferenceSemantics for reference.

[Xazax-hun]

Thanks, this is a good point! I will update the PR and add a test.

[Xazax-hun]

I ended up going in a slightly different direction. We did not have Swift's ASTContext and the importer available in this query, and it is used from Decl.cpp which should never require an importer. Also, it is sufficient to do this query once, when we make the decision how to import a type. Instead, I started to pass in the Swift declaration which gives us a really cheap way to check if we imported something as a shared reference (we just check if it was imported as a class).

[fahadnayyar]

I'm approving this, but I'd like to add a request to consider inherited SWIFT_SHARED_REFERENCE types in a follow-up patch.

[egorzhdan]

Hmm, could we just have bool isClass here, or do you anticipate that we'll have a more elaborate heuristic in the future?

[Xazax-hun]

In some call sites we don't need/have this information. It could be a std::optional<bool> isClass; though. I don't have strong opinions here.

[egorzhdan]

What's the call site that doesn't have this information?

[Xazax-hun]

This descriptor is used for two queries: IsSafeUseOfCxxDecl and ClangDeclExplicitSafety.

The former is used to rename methods to "_nameUnsafe" and we don't have a Swift decl when we make the naming decision. But the former query does not need this information. I could clean this up by having 2 separate descriptors for the 2 queries.

But more importantly, ClangDeclExplicitSafety will recursively check for the safety of the base classes. While we iterate through the Clang bases we do not have the corresponding imported Swift types for those base classes.

But this is not problematic because:

• Either the derived class was a shared reference, in that case we do not care about the bases and do an early return.
• Or the derived class was not a shared reference, in that case a base being a shared reference does not prevent us from considering this type as unsafe since this is not actually reference counted by Swift.

[egorzhdan]

Ah I see, I didn't realize this is used for two different requests. Thanks for explaining! Yeah, I would vote for splitting it up into two distinct descriptors.

[Xazax-hun]

Done.

[Xazax-hun]

@swift-ci please smoke test

[egorzhdan]

LGTM, thanks!