Skip to content

[🍒][cxx-interop] Use translation unit scope when doing lookups; disambiguate math functions. #67020

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.

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 12 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion lib/ClangImporter/ClangAdapter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ clang::TypedefNameDecl *importer::findSwiftNewtype(const clang::NamedDecl *decl,
clang::LookupResult lookupResult(clangSema, notificationName,
clang::SourceLocation(),
clang::Sema::LookupOrdinaryName);
if (!clangSema.LookupName(lookupResult, nullptr))
if (!clangSema.LookupName(lookupResult, clangSema.TUScope))
return nullptr;
auto nsDecl = lookupResult.getAsSingle<clang::TypedefNameDecl>();
if (!nsDecl)
Expand Down
2 changes: 1 addition & 1 deletion lib/ClangImporter/ClangImporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2712,7 +2712,7 @@ ClangImporter::Implementation::lookupTypedef(clang::DeclarationName name) {
clang::SourceLocation(),
clang::Sema::LookupOrdinaryName);

if (sema.LookupName(lookupResult, /*scope=*/nullptr)) {
if (sema.LookupName(lookupResult, sema.TUScope)) {
for (auto decl : lookupResult) {
if (auto typedefDecl =
dyn_cast<clang::TypedefNameDecl>(decl->getUnderlyingDecl()))
Expand Down
32 changes: 25 additions & 7 deletions lib/ClangImporter/ImportDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3197,19 +3197,30 @@ namespace {
// presence in the C++ standard library will cause overloading
// ambiguities or other type checking errors in Swift.
auto isAlternativeCStdlibFunctionFromTextualHeader =
[](const clang::FunctionDecl *d) -> bool {
[this](const clang::FunctionDecl *d) -> bool {
// stdlib.h might be a textual header in libc++'s module map.
// in this case, check for known ambiguous functions by their name
// instead of checking if they come from the `std` module.
if (!d->getDeclName().isIdentifier())
return false;
return d->getName() == "abs" || d->getName() == "div";
if (d->getName() == "abs" || d->getName() == "div")
return true;
if (Impl.SwiftContext.LangOpts.Target.isOSDarwin())
return d->getName() == "strstr" || d->getName() == "sin" ||
d->getName() == "cos" || d->getName() == "exit";
return false;
};
if (decl->getOwningModule() &&
(decl->getOwningModule()
->getTopLevelModule()
->getFullModuleName() == "std" ||
isAlternativeCStdlibFunctionFromTextualHeader(decl))) {
auto topLevelModuleEq =
[](const clang::FunctionDecl *d, StringRef n) -> bool {
return d->getOwningModule() &&
d->getOwningModule()
->getTopLevelModule()
->getFullModuleName() == n;
};
if (topLevelModuleEq(decl, "std")) {
if (isAlternativeCStdlibFunctionFromTextualHeader(decl)) {
return nullptr;
}
auto filename =
Impl.getClangPreprocessor().getSourceManager().getFilename(
decl->getLocation());
Expand All @@ -3218,6 +3229,13 @@ namespace {
return nullptr;
}
}
// Use the exit function from _SwiftConcurrency.h as it is platform
// agnostic.
if ((topLevelModuleEq(decl, "Darwin") ||
topLevelModuleEq(decl, "SwiftGlibc")) &&
decl->getDeclName().isIdentifier() && decl->getName() == "exit") {
return nullptr;
}
Comment on lines +3232 to +3238
Copy link
Contributor

Choose a reason for hiding this comment

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

@zoecarver: is there some reason for not doing this check like !topLevelModuleEq(decl, "SwiftConcurrencyShims") && decl->getDeclName().isIdentifier() && decl->getName() == "exit" instead? Hardcoding SwiftConcurrencyShims is bad, but at least it is just one module name, and not two. I found out about this because SwiftGlibc is the name of the overlaid module provided by Swift, but what if the system provides its own modulemap for the system headers and it is called differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to import user-defined overloaded or nested exit functions.

}

auto dc =
Expand Down
4 changes: 3 additions & 1 deletion lib/ClangImporter/ImportMacro.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,8 @@ static Optional<std::pair<llvm::APSInt, Type>>
}

// Macro identifier.
// TODO: for some reason when in C++ mode, "hasMacroDefinition" is often
// false: rdar://110071334
} else if (token.is(clang::tok::identifier) &&
token.getIdentifierInfo()->hasMacroDefinition()) {

Expand Down Expand Up @@ -422,7 +424,7 @@ static ValueDecl *importMacro(ClangImporter::Implementation &impl,
auto diagState = impl.getClangSema().DelayedDiagnostics.push(diagPool);
auto parsedType = impl.getClangSema().getTypeName(identifier,
clang::SourceLocation(),
/*scope*/nullptr);
impl.getClangSema().TUScope);
impl.getClangSema().DelayedDiagnostics.popWithoutEmitting(diagState);

if (parsedType && diagPool.empty()) {
Expand Down
2 changes: 2 additions & 0 deletions test/ClangImporter/cf.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// RUN: %target-swift-frontend -disable-objc-attr-requires-foundation-module -typecheck -verify -import-cf-types -I %S/Inputs/custom-modules %s

// RUN: %target-swift-frontend -disable-objc-attr-requires-foundation-module -typecheck -verify -import-cf-types -enable-experimental-cxx-interop -I %S/Inputs/custom-modules %s

// REQUIRES: objc_interop

import CoreCooling
Expand Down
3 changes: 3 additions & 0 deletions test/ClangImporter/macros.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -enable-objc-interop -typecheck -verify %s

// Most of these don't pass: rdar://110071334
// %target-swift-frontend(mock-sdk: %clang-importer-sdk) -enable-experimental-cxx-interop -enable-objc-interop -typecheck -verify %s

@_exported import macros

func circle_area(_ radius: CDouble) -> CDouble {
Expand Down
6 changes: 6 additions & 0 deletions test/Interop/Cxx/objc-correctness/Inputs/module.modulemap
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,9 @@ module CxxClassWithNSStringInit [extern_c] {
module NSOptionsMangling {
header "NSOptionsMangling.h"
}

module NSNofiticationBridging {
header "nsnotification-bridging.h"
requires objc
requires cplusplus
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#import <Foundation/Foundation.h>

extern NSString * const SpaceShipNotification;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// RUN: %target-swift-ide-test -print-module -module-to-print=NSNofiticationBridging -I %S/Inputs -source-filename=x -enable-experimental-cxx-interop -enable-objc-interop | %FileCheck %s

// REQUIRES: objc_interop

// CHECK: import Foundation

// CHECK: let SpaceShipNotification: String
22 changes: 10 additions & 12 deletions test/Interop/Cxx/stdlib/avoid-import-cxx-math.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,17 @@
import CxxStdlib

func test() {
let x: Float = 1.0
let x: Double = 1.0
let y: Double = 2.0

// Note: we dispatch `pow(Float,Double)`
// to ensure we don't pick up the
// C++ stdlib `pow` function template.
// The `pow` function is still reexported
// from Darwin via CxxStdlib, so there are
// matching overloads that can be found still.
// Note: the error is different on Glibc instead
// of Darwin, so do not check the exact error.
let _ = CxxStdlib.pow(x, y) // expected-error {{}}
let _ = pow(x, y)

let _ = CxxStdlib.abs(x) // expected-error {{module 'CxxStdlib' has no member named 'abs'}}
let _ = CxxStdlib.div(x) // expected-error {{module 'CxxStdlib' has no member named 'div'}}
let _ = abs(x)
// https://github.com/apple/swift/issues/67006
// let _ = div(42, 2)
let _ = sin(x)
let _ = cos(x)
let _ = strstr("a", "aaa")

exit(0)
}

This file was deleted.

39 changes: 39 additions & 0 deletions test/Interop/Cxx/stdlib/use-swift-concurrency.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// RUN: %target-run-simple-swift(-I %S/Inputs -Xfrontend -enable-experimental-cxx-interop -Xfrontend -disable-availability-checking %import-libdispatch -parse-as-library)
//
// REQUIRES: executable_test
// REQUIRES: concurrency
// REQUIRES: libdispatch
// REQUIRES: concurrency_runtime

import StdlibUnittest

import CxxStdlib
import Cxx

import _Concurrency
import Dispatch

@main struct Main {
static func main() async {
var ConcurrencyTestSuite = TestSuite("Concurrency")

ConcurrencyTestSuite.test("Task.sleep") {
let start = DispatchTime.now()
await Task.sleep(100_000_000)
let stop = DispatchTime.now()
expectTrue(stop >= (start + .nanoseconds(100_000_000)))
}

ConcurrencyTestSuite.test("Task.sleep (non-blocking)") {
let task = detach {
std.string("Hello, Swift!")
}

await Task.sleep(100_000_000)
expectEqual(await task.get(), "Hello, Swift!")
}

await runAllTestsAsync()
}
}