Skip to content

[ClangImporter] Import NSNotificationName constants as nonisolated #71419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 8, 2024
Merged

[ClangImporter] Import NSNotificationName constants as nonisolated #71419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
269f30f
[ClangImporter] Add a way to determine whether type is NSNotification…
xedin Feb 6, 2024
5f0a9c4
[ClangImporter] Import NSNotificationName constants as `nonisolated`
xedin Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/ClangImporter/ClangAdapter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,13 @@ bool importer::isNSString(clang::QualType qt) {
return qt.getTypePtrOrNull() && isNSString(qt.getTypePtrOrNull());
}

bool importer::isNSNotificationName(clang::QualType type) {
if (auto *typealias = type->getAs<clang::TypedefType>()) {
return typealias->getDecl()->getName() == "NSNotificationName";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a slight inconsistency with other parts of ClangImporter here—they only treat NSNotifcationName as special if it also (according to retrieveNewTypeAttr()) has a clang::SwiftNewTypeAttr attached. But I think this will only make a difference if the SDK has a broken declaration of NSNotificationName, so it's probably harmless.

}
return false;
}

bool importer::isNSNotificationGlobal(const clang::NamedDecl *decl) {
// Looking for: extern NSString *fooNotification;

Expand Down
3 changes: 3 additions & 0 deletions lib/ClangImporter/ClangAdapter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ clang::TypedefNameDecl *findSwiftNewtype(const clang::NamedDecl *decl,
bool isNSString(const clang::Type *);
bool isNSString(clang::QualType);

/// Wehther the passed type is `NSNotificationName` typealias
bool isNSNotificationName(clang::QualType);

/// Whether the given declaration was exported from Swift.
///
/// Note that this only checks the immediate declaration being passed.
Expand Down
17 changes: 17 additions & 0 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8138,6 +8138,23 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) {
/*isUnchecked=*/true);
}
}

// Special handling of `NSNotificationName` static immutable properties.
//
// These constants could be used with observer APIs from a different isolation
// context, so it's more convenient to import them as `nonisolated` unless
// they are explicitly isolated to a MainActor.
if (!seenMainActorAttr) {
auto *DC = MappedDecl->getDeclContext();
if (DC->isTypeContext() && isa<VarDecl>(MappedDecl)) {
auto *mappedVar = cast<VarDecl>(MappedDecl);
if (mappedVar->isStatic() && mappedVar->isLet() &&
isNSNotificationName(cast<clang::ValueDecl>(ClangDecl)->getType())) {
MappedDecl->getAttrs().add(new (SwiftContext) NonisolatedAttr(
/*unsafe=*/false, /*implicit=*/true));
}
}
}
}

static bool isUsingMacroName(clang::SourceManager &SM,
Expand Down
45 changes: 45 additions & 0 deletions test/Concurrency/import_nsnotificationname_constants_as_nonisolated.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// RUN: %empty-directory(%t/src)
// RUN: %empty-directory(%t/sdk)
// RUN: split-file %s %t/src

// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck %t/src/main.swift \
// RUN: -import-objc-header %t/src/Test.h \
// RUN: -strict-concurrency=complete \
// RUN: -disable-availability-checking \
// RUN: -module-name main -I %t -verify

// REQUIRES: objc_interop
// REQUIRES: concurrency

//--- Test.h
#define SWIFT_MAIN_ACTOR __attribute__((swift_attr("@MainActor")))

#pragma clang assume_nonnull begin

@import Foundation;

SWIFT_MAIN_ACTOR
@interface Test : NSObject
@end
extern NSNotificationName const TestDidTrigger __attribute__((swift_name("Test.didTrigger")));

SWIFT_MAIN_ACTOR
extern NSNotificationName const TestIsolatedTrigger __attribute__((swift_name("Test.isolatedTrigger")));

#pragma clang assume_nonnull end

//--- main.swift

func testAsync() async {
print(Test.didTrigger) // Ok (property is nonisolated)
print(Test.isolatedTrigger)
// expected-warning@-1 {{expression is 'async' but is not marked with 'await'; this is an error in Swift 6}}
// expected-note@-2 {{property access is 'async'}}
}

@MainActor
func testMainActor() {
print(Test.didTrigger) // Ok
print(Test.isolatedTrigger) // Ok
}