Skip to content

[cxx-interop] Pull over fix from 8e7766b and apply to strstr, sin, an… #66896

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 8 commits into from
Jun 29, 2023
Merged
32 changes: 25 additions & 7 deletions lib/ClangImporter/ImportDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3200,19 +3200,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 @@ -3221,6 +3232,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;
}
}

auto dc =
Expand Down
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'}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was getting errors like this:

Darwin.pow:1:13: note: diagnostic produced elsewhere: candidate expects value of type 'Double' for parameter #1 (got 'Float')
public func pow(_: Double, _: Double) -> Double

So I figured we could just test the positive case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also testing the positive case seems more direct in terms of making sure the thing we want to work works. It also is a bit less fragile and more portable.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect example of why we should test the positive case: div wasn't available on linux even before this patch because it's in stdlib.h. Issue filed.

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()
}
}