Skip to content

[Constraint solver] Favor more-specialized overload among two generics. #14499

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
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
10 changes: 4 additions & 6 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ set_property(GLOBAL PROPERTY JOB_POOLS local_jobs=${localhost_logical_cores})
# Put linking in that category
set_property(GLOBAL PROPERTY JOB_POOL_LINK local_jobs)

ENABLE_LANGUAGE(C)

# First include general CMake utilities.
include(SwiftUtils)
include(CheckSymbolExists)
Expand Down Expand Up @@ -750,10 +748,10 @@ if(swift_build_android AND NOT "${SWIFT_ANDROID_NDK_PATH}" STREQUAL "")

set(SWIFT_ANDROID_PREBUILT_PATH
"${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-${SWIFT_ANDROID_NDK_GCC_VERSION}/prebuilt/${_swift_android_prebuilt_suffix}")

# Resolve the correct linker based on the file name of CMAKE_LINKER (being 'ld' or 'ld.gold' the options)
get_filename_component(SWIFT_ANDROID_LINKER_NAME "${CMAKE_LINKER}" NAME)
set(SWIFT_SDK_ANDROID_ARCH_armv7_LINKER
set(SWIFT_SDK_ANDROID_ARCH_armv7_LINKER
"${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-${SWIFT_ANDROID_NDK_GCC_VERSION}/prebuilt/${_swift_android_prebuilt_suffix}/bin/arm-linux-androideabi-${SWIFT_ANDROID_LINKER_NAME}")

configure_sdk_unix(ANDROID "Android" "android" "android" "armv7" "armv7-none-linux-androideabi" "${SWIFT_ANDROID_SDK_PATH}")
Expand Down Expand Up @@ -881,7 +879,7 @@ endif()
###############
#
# We have to include stdlib/ before tools/.
# Do not move add_subdirectory(stdlib) after add_subdirectory(tools)!
# Do not move add_subdirectory(stdlib) after add_subdirectory(tools)!
#
# We must include stdlib/ before tools/ because stdlib/CMakeLists.txt
# declares the swift-stdlib-* set of targets. These targets will then
Expand All @@ -900,7 +898,7 @@ add_subdirectory(stdlib)
if(SWIFT_INCLUDE_TOOLS)
add_subdirectory(include)
add_subdirectory(lib)

# Always include this after including stdlib/!
# Refer to the large comment above the add_subdirectory(stdlib) call.
# https://bugs.swift.org/browse/SR-5975
Expand Down
10 changes: 5 additions & 5 deletions cmake/modules/AddSwift.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ function(_add_variant_c_compile_flags)
else()
list(APPEND result "-DNDEBUG")
endif()

if(SWIFT_ENABLE_RUNTIME_FUNCTION_COUNTERS)
list(APPEND result "-DSWIFT_ENABLE_RUNTIME_FUNCTION_COUNTERS")
endif()
Expand Down Expand Up @@ -320,7 +320,7 @@ function(_add_variant_swift_compile_flags
if (SWIFT_ENABLE_GUARANTEED_NORMAL_ARGUMENTS)
list(APPEND result "-Xfrontend" "-enable-guaranteed-normal-arguments")
endif()

if(SWIFT_ENABLE_RUNTIME_FUNCTION_COUNTERS)
list(APPEND result "-D" "SWIFT_ENABLE_RUNTIME_FUNCTION_COUNTERS")
endif()
Expand Down Expand Up @@ -1605,7 +1605,7 @@ function(add_swift_library name)
if(SWIFTLIB_IS_SDK_OVERLAY)
list(APPEND swiftlib_swift_compile_flags_all "-Fsystem" "${SWIFT_SDK_${sdk}_PATH}/System/Library/PrivateFrameworks/")
endif()

if("${sdk}" STREQUAL "IOS_SIMULATOR")
if("${name}" STREQUAL "swiftMediaPlayer")
message("DISABLING AUTOLINK FOR swiftMediaPlayer")
Expand Down Expand Up @@ -1854,7 +1854,7 @@ function(add_swift_library name)
LIBRARY DESTINATION lib${LLVM_LIBDIR_SUFFIX}
RUNTIME DESTINATION bin)
swift_is_installing_component(dev is_installing)

if(NOT is_installing)
set_property(GLOBAL APPEND PROPERTY SWIFT_BUILDTREE_EXPORTS ${name})
else()
Expand Down Expand Up @@ -2177,7 +2177,7 @@ function(add_swift_host_tool executable)

swift_is_installing_component(${ADDSWIFTHOSTTOOL_SWIFT_COMPONENT}
is_installing)

if(NOT is_installing)
set_property(GLOBAL APPEND PROPERTY SWIFT_BUILDTREE_EXPORTS ${executable})
else()
Expand Down
23 changes: 23 additions & 0 deletions lib/Sema/ConstraintSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1389,6 +1389,29 @@ void ConstraintSystem::addOverloadSet(Type boundType,
return;
}

// Performance hack: if there are two generic overloads, and one is
// more specialized than the other, prefer the more-specialized one.
if (!favoredChoice && choices.size() == 2 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is great! I'm trying to implement similar thing as well but instead of hard-coding this to 2 overloads only, maybe it would be better to try and group overload choices based on declaration context and rank them in groups so at the end there are N favored choices in the set?... Another idea might be to move from favoring to making it a ranking criteria since we erase worse intermediate solutions...

Copy link
Member Author

Choose a reason for hiding this comment

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

You're absolutely right that we could generalize this. In general, if an one overload is more specialized than another, we should try it first. We could probably encode this information in an tree of disjunctions using favored constraints... but it would probably be better in the long run to compute the lattice describing the relationships and have the solver take an appropriate path through the lattice.

As for moving it to ranking criteria, that won't prune as efficiently, because we often still explore those other options. I'd rather

choices[0].isDecl() && choices[1].isDecl() &&
isa<AbstractFunctionDecl>(choices[0].getDecl()) &&
cast<AbstractFunctionDecl>(choices[0].getDecl())->isGeneric() &&
isa<AbstractFunctionDecl>(choices[1].getDecl()) &&
cast<AbstractFunctionDecl>(choices[1].getDecl())->isGeneric()) {
switch (TC.compareDeclarations(DC, choices[0].getDecl(),
choices[1].getDecl())) {
case Comparison::Better:
favoredChoice = const_cast<OverloadChoice *>(&choices[0]);
break;

case Comparison::Worse:
favoredChoice = const_cast<OverloadChoice *>(&choices[1]);
break;

case Comparison::Unordered:
break;
}
}

SmallVector<Constraint *, 4> overloads;

// As we do for other favored constraints, if a favored overload has been
Expand Down
13 changes: 9 additions & 4 deletions test/Compatibility/enum_cases.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,18 @@ enum G_E<T> {
let arr: [String] = []
let _ = arr.map(E.foo) // Ok
let _ = arr.map(E.bar) // Ok
let _ = arr.map(E.two) // expected-error {{cannot convert value of type '(Int, Int) -> E' to expected argument type '(String) -> _'}}
let _ = arr.map(E.tuple) // expected-error {{cannot convert value of type '((x: Int, y: Int)) -> E' to expected argument type '(String) -> _'}}
let _ = arr.map(E.two) // expected-error {{cannot invoke 'map' with an argument list of type '((Int, Int) -> E)'}}
// expected-note@-1{{expected an argument list of type '((Self.Element) throws -> T)'}}
let _ = arr.map(E.tuple) // expected-error {{cannot invoke 'map' with an argument list of type '(((x: Int, y: Int)) -> E)'}}
// expected-note@-1{{expected an argument list of type '((Self.Element) throws -> T)'}}

let _ = arr.map(G_E<String>.foo) // Ok
let _ = arr.map(G_E<String>.bar) // Ok
let _ = arr.map(G_E<String>.two) // expected-error {{cannot convert value of type '(String, String) -> G_E<String>' to expected argument type '(String) -> _'}}
let _ = arr.map(G_E<Int>.tuple) // expected-error {{cannot convert value of type '((x: Int, y: Int)) -> G_E<Int>' to expected argument type '(String) -> _'}}
let _ = arr.map(G_E<String>.two) // expected-error {{cannot invoke 'map' with an argument list of type '((String, String) -> G_E<String>)'}}
// expected-note@-1{{expected an argument list of type '((Self.Element) throws -> T)'}}

let _ = arr.map(G_E<Int>.tuple) // expected-error {{cannot invoke 'map' with an argument list of type '(((x: Int, y: Int)) -> G_E<Int>)'}}
// expected-note@-1{{expected an argument list of type '((Self.Element) throws -> T)'}}

let _ = E.foo("hello") // expected-error {{missing argument label 'bar:' in call}}
let _ = E.bar("hello") // Ok
Expand Down
13 changes: 9 additions & 4 deletions test/Constraints/enum_cases.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,18 @@ enum G_E<T> {
let arr: [String] = []
let _ = arr.map(E.foo) // Ok
let _ = arr.map(E.bar) // Ok
let _ = arr.map(E.two) // expected-error {{cannot convert value of type '(Int, Int) -> E' to expected argument type '(String) -> _'}}
let _ = arr.map(E.tuple) // expected-error {{cannot convert value of type '((x: Int, y: Int)) -> E' to expected argument type '(String) -> _'}}
let _ = arr.map(E.two) // expected-error {{cannot invoke 'map' with an argument list of type '((Int, Int) -> E)'}}
// expected-note@-1{{expected an argument list of type '((Self.Element) throws -> T)'}}

let _ = arr.map(E.tuple) // expected-error {{cannot invoke 'map' with an argument list of type '(((x: Int, y: Int)) -> E)'}}
// expected-note@-1{{expected an argument list of type '((Self.Element) throws -> T)'}}

let _ = arr.map(G_E<String>.foo) // Ok
let _ = arr.map(G_E<String>.bar) // Ok
let _ = arr.map(G_E<String>.two) // expected-error {{cannot convert value of type '(String, String) -> G_E<String>' to expected argument type '(String) -> _'}}
let _ = arr.map(G_E<Int>.tuple) // expected-error {{cannot convert value of type '((x: Int, y: Int)) -> G_E<Int>' to expected argument type '(String) -> _'}}
let _ = arr.map(G_E<String>.two) // expected-error {{cannot invoke 'map' with an argument list of type '((String, String) -> G_E<String>)'}}
// expected-note@-1{{expected an argument list of type '((Self.Element) throws -> T)'}}
let _ = arr.map(G_E<Int>.tuple) // expected-error {{cannot invoke 'map' with an argument list of type '(((x: Int, y: Int)) -> G_E<Int>)'}}
// expected-note@-1{{expected an argument list of type '((Self.Element) throws -> T)'}}

let _ = E.foo("hello") // expected-error {{missing argument label 'bar:' in call}}
let _ = E.bar("hello") // Ok
Expand Down
12 changes: 10 additions & 2 deletions test/Constraints/optional.swift
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,22 @@ func test8(_ x : AnyObject?) {


// Partial ordering with optionals
func test9_helper<T>(_ x: T) -> Int { }
func test9_helper<T>(_ x: T?) -> Double { }
func test9_helper<T: P>(_ x: T) -> Int { }
func test9_helper<T: P>(_ x: T?) -> Double { }

func test9_helper2<T>(_ x: T) -> Int { }
func test9_helper2<T>(_ x: T?) -> Double { }

func test9(_ i: Int, io: Int?) {
let result = test9_helper(i)
var _: Int = result
let result2 = test9_helper(io)
let _: Double = result2

let result3 = test9_helper2(i)
var _: Double = result3
let result4 = test9_helper2(io)
let _: Double = result4
}

protocol P { }
Expand Down
3 changes: 1 addition & 2 deletions test/SwiftSyntax/DeserializeFile.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import StdlibUnittest
import Foundation
import SwiftSyntax
import SwiftLang

func getInput(_ file: String) -> URL {
var result = URL(fileURLWithPath: #file)
Expand All @@ -20,7 +19,7 @@ var DecodeTests = TestSuite("DecodeSyntax")

DecodeTests.test("Basic") {
expectDoesNotThrow({
let content = try SwiftLang.parse(getInput("visitor.swift"))
let content = try SourceFileSyntax.encodeSourceFileSyntax(getInput("visitor.swift"))
let source = try String(contentsOf: getInput("visitor.swift"))
let parsed = try SourceFileSyntax.decodeSourceFileSyntax(content)
expectEqual("\(parsed)", source)
Expand Down
5 changes: 2 additions & 3 deletions test/SwiftSyntax/ParseFile.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
// REQUIRES: executable_test
// REQUIRES: OS=macosx
// REQUIRES: objc_interop
// REQUIRES: rdar36740859

import Foundation
import StdlibUnittest
import SwiftSyntax
import SwiftLang

var ParseFile = TestSuite("ParseFile")

Expand All @@ -31,8 +31,7 @@ ParseFile.test("ParseSingleFile") {
let currentFile = URL(fileURLWithPath: #file)
expectDoesNotThrow({
let currentFileContents = try String(contentsOf: currentFile)
let parsed = try SourceFileSyntax.decodeSourceFileSyntax(try
SwiftLang.parse(currentFile))
let parsed = try SourceFileSyntax.parse(currentFile)
expectEqual("\(parsed)", currentFileContents)
})
}
Expand Down
7 changes: 4 additions & 3 deletions test/SwiftSyntax/VisitorTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
// REQUIRES: OS=macosx
// REQUIRES: objc_interop

// FIXME: This test fails occassionally in CI with invalid json.
// REQUIRES: disabled

import StdlibUnittest
import Foundation
import SwiftSyntax
import SwiftLang

func getInput(_ file: String) -> URL {
var result = URL(fileURLWithPath: #file)
Expand All @@ -27,8 +29,7 @@ VisitorTests.test("Basic") {
}
}
expectDoesNotThrow({
let parsed = try SourceFileSyntax.decodeSourceFileSyntax(try
SwiftLang.parse(getInput("visitor.swift")))
let parsed = try SourceFileSyntax.parse(getInput("visitor.swift"))
let counter = FuncCounter()
let hashBefore = parsed.hashValue
counter.visit(parsed)
Expand Down
6 changes: 2 additions & 4 deletions test/lit.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,6 @@ else:
test_resource_dir = os.path.join(config.swift_lib_dir, 'swift')
resource_dir_opt = ""
stdlib_resource_dir_opt = resource_dir_opt
sourcekitd_framework_dir = config.swift_lib_dir
lit_config.note('Using resource dir: ' + test_resource_dir)

# Parse the variant triple.
Expand Down Expand Up @@ -671,13 +670,12 @@ if run_vendor == 'apple':
(run_cpu, run_os, run_vers, clang_mcp_opt))

config.target_build_swift = (
"%s %s %s -F %s -Xlinker -rpath -Xlinker %s %s %s %s %s -F %s -Xlinker -rpath -Xlinker %s"
"%s %s %s -F %s -Xlinker -rpath -Xlinker %s %s %s %s %s"
% (xcrun_prefix, config.swiftc, target_options,
extra_frameworks_dir, extra_frameworks_dir,
sdk_overlay_linker_opt, config.swift_test_options,
config.swift_driver_test_options,
swift_execution_tests_extra_flags, sourcekitd_framework_dir,
sourcekitd_framework_dir))
swift_execution_tests_extra_flags))
config.target_run = ""

if 'interpret' in lit_config.params:
Expand Down
23 changes: 2 additions & 21 deletions tools/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,14 +1,3 @@
include(CheckIncludeFiles)
check_include_files("xpc/xpc.h" HAVE_XPC_H)

swift_is_installing_component(sourcekit-inproc SOURCEKIT_INSTALLING_INPROC)

if(HAVE_XPC_H AND SWIFT_BUILD_SOURCEKIT AND NOT SOURCEKIT_INSTALLING_INPROC)
set(BUILD_SOURCEKIT_XPC_SERVICE TRUE)
else()
set(BUILD_SOURCEKIT_XPC_SERVICE FALSE)
endif()

# Add generated libSyntax headers to global dependencies.
list(APPEND LLVM_COMMON_DEPENDS swift-syntax-generated-headers)

Expand All @@ -27,27 +16,19 @@ add_swift_tool_subdirectory(swift-api-digester)
add_swift_tool_subdirectory(swift-syntax-test)
add_swift_tool_subdirectory(swift-refactor)

if(SWIFT_BUILD_STDLIB AND SWIFT_BUILD_SDK_OVERLAY)
set(BUILD_FOUNDATION TRUE)
else()
set(BUILD_FOUNDATION FALSE)
endif()

if(SWIFT_BUILD_SOURCEKIT)
add_swift_tool_subdirectory(SourceKit)
if(BUILD_SOURCEKIT_XPC_SERVICE AND BUILD_FOUNDATION)
add_subdirectory(SwiftSourceKitClient)
endif()
endif()


if(SWIFT_HOST_VARIANT STREQUAL "macosx")
# Only build Darwin-specific tools when deploying to OS X.
add_swift_tool_subdirectory(swift-stdlib-tool)

# SwiftSyntax depends on both the standard library (because it's a
# Swift module), and the SDK overlays (because it depends on Foundation).
# Ensure we only build SwiftSyntax when we're building both.
if(BUILD_FOUNDATION)
if(SWIFT_BUILD_STDLIB AND SWIFT_BUILD_SDK_OVERLAY)
add_subdirectory(SwiftSyntax)
endif()
endif()
Expand Down
2 changes: 2 additions & 0 deletions tools/SourceKit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ configure_file(
set(SOURCEKIT_DEPLOYMENT_OS "${SWIFT_HOST_VARIANT}")
set(SOURCEKIT_DEPLOYMENT_TARGET "${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_DEPLOYMENT_VERSION}")

swift_is_installing_component(sourcekit-inproc SOURCEKIT_INSTALLING_INPROC)

if("${CMAKE_SYSTEM_NAME}" STREQUAL "Darwin" AND NOT CMAKE_CROSSCOMPILING)
set(CMAKE_OSX_SYSROOT "${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_PATH}")
set(CMAKE_OSX_ARCHITECTURES "${SWIFT_HOST_VARIANT_ARCH}")
Expand Down
2 changes: 1 addition & 1 deletion tools/SourceKit/tools/sourcekitd/bin/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
add_subdirectory(InProc)
if(HAVE_XPC_H)
if (HAVE_XPC_H)
add_subdirectory(XPC)
endif()
4 changes: 4 additions & 0 deletions tools/SourceKit/tools/sourcekitd/lib/API/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
include(CheckIncludeFiles)

check_include_files("xpc/xpc.h" HAVE_XPC_H)

# If we were going to build for APPLE but don't have XPC, just build inproc.
if(APPLE AND NOT HAVE_XPC_H)
set(SWIFT_SOURCEKIT_USE_INPROC_LIBRARY TRUE)
Expand Down
18 changes: 0 additions & 18 deletions tools/SwiftSourceKitClient/CMakeLists.txt

This file was deleted.

29 changes: 0 additions & 29 deletions tools/SwiftSourceKitClient/SourceKitdClient.swift

This file was deleted.

Loading