Skip to content
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

[TypeChecker] Implement Double <-> CGFloat implicit conversion #34401

Merged
merged 45 commits into from
Mar 31, 2021

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Oct 23, 2020

Implement an implicit conversion between Double and CGFloat types via
an implicit initializer call.

Resolves: rdar://problem/70592377

@xedin xedin requested a review from hborla October 23, 2020 00:59
@xedin
Copy link
Contributor Author

xedin commented Oct 23, 2020

@swift-ci please test source compatibility

@xedin
Copy link
Contributor Author

xedin commented Oct 23, 2020

Debug build was a UPASS

@xedin xedin force-pushed the implicit-cgfloat-conversion branch from ee8542a to 51aa211 Compare October 26, 2020 19:07
@xwu
Copy link
Collaborator

xwu commented Oct 26, 2020

This looks fantastic in terms of improving the usability of CGFloat.

To the extent that it touches upon the Swift language itself, and in particular type relationships and one of the original tentpole design decisions not to have implicit numeric conversions, I hope we'll have a chance to discuss and review this through the Swift Evolution process.

If presented as a sort of built-in ABI-stable retroactive typealias-equivalent, it would be pretty much a no-brainer, but if presented as an "implicit conversion," what's implied (to some, at least) would be a much more radical departure from Swift's design.

@xedin
Copy link
Contributor Author

xedin commented Oct 26, 2020

Thanks, @xwu! At this point I'm mostly trying to figure out whether it's feasible and what the performance impact is going to be. This would definitely be pitched and discussed on Swift Evolution if this experiment is a success.

@xedin
Copy link
Contributor Author

xedin commented Oct 27, 2020

@swift-ci please build toolchain macOS

@xedin xedin force-pushed the implicit-cgfloat-conversion branch from 51aa211 to c47feea Compare October 28, 2020 03:06
@xedin
Copy link
Contributor Author

xedin commented Oct 28, 2020

@swift-ci please build toolchain macOS

@swift-ci
Copy link
Contributor

macOS Toolchain
Download Toolchain
Git Sha - c47feeacc3a5408c63ab4fc39dc9d50d00fccd42

Install command
tar -zxf swift-PR-34401-738-osx.tar.gz --directory ~/

@xedin xedin added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Oct 30, 2020
@xedin xedin force-pushed the implicit-cgfloat-conversion branch from c47feea to 2573c7d Compare October 31, 2020 07:35
@xedin
Copy link
Contributor Author

xedin commented Oct 31, 2020

@swift-ci please build toolchain

@swift-ci
Copy link
Contributor

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 2573c7d6dc075fa12e3bd349745efcb3c19dce4e

Install command
tar zxf swift-PR-34401-477-ubuntu16.04.tar.gz
More info

@swift-ci
Copy link
Contributor

macOS Toolchain
Download Toolchain
Git Sha - 2573c7d6dc075fa12e3bd349745efcb3c19dce4e

Install command
tar -zxf swift-PR-34401-745-osx.tar.gz --directory ~/

@xedin xedin force-pushed the implicit-cgfloat-conversion branch from 2573c7d to bbfb1d9 Compare November 12, 2020 01:13
@xedin
Copy link
Contributor Author

xedin commented Nov 12, 2020

@swift-ci please test source compatibility

@xedin xedin force-pushed the implicit-cgfloat-conversion branch from bbfb1d9 to 0d5de0e Compare November 14, 2020 06:18
@xedin
Copy link
Contributor Author

xedin commented Nov 14, 2020

@swift-ci please test source compatibility

1 similar comment
@xedin
Copy link
Contributor Author

xedin commented Nov 16, 2020

@swift-ci please test source compatibility

@xedin xedin force-pushed the implicit-cgfloat-conversion branch 2 times, most recently from 0fddd92 to b54012d Compare November 17, 2020 00:38
@xedin
Copy link
Contributor Author

xedin commented Nov 17, 2020

@swift-ci please test source compatibility

@xedin
Copy link
Contributor Author

xedin commented Nov 17, 2020

@swift-ci please build toolchain

@swift-ci
Copy link
Contributor

macOS Toolchain
Download Toolchain
Git Sha - b54012d38118cb84e3a496ccf39794d6b0eae3d3

Install command
tar -zxf swift-PR-34401-774-osx.tar.gz --directory ~/

@xedin xedin force-pushed the implicit-cgfloat-conversion branch from b54012d to ad047cd Compare January 7, 2021 17:41
@xedin
Copy link
Contributor Author

xedin commented Jan 7, 2021

@swift-ci please test source compatibility

@xedin xedin force-pushed the implicit-cgfloat-conversion branch from a556030 to ffc3e4f Compare January 16, 2021 00:48
xedin added 15 commits March 17, 2021 00:18
This makes it possible to pick `CGFloat` function/operator overloads even
in the presence of literals, otherwise non-default literal score gets in
a way and solver ends up producing a lot more solutions with implicit
Double<->CGFloat conversion than it can disambiguate, so it's better to
just preserve the old behavior and pick `CGFloat` concrete overloads when
appropriate.
… arguments

If argument is a floating-point literal, with newly introduced implicit
Double<->CGFloat conversion, sometimes it's better to choose a concrete
function/operator overload on `CGFloat` even if it's not a default literal
type e.g. `let _: CGFloat = min(1.0, log(<CGFloat value>))` shouldn't form
solutions with `Double` arguments since it would result in multiple
implicit conversions, it's better to use a `CGFloat` type for the arguments.
…ce of Double<->CGFloat conversion

Not all of the unary operators have `CGFloat` overloads,
so in order to preserve previous behavior (and overall
best solution) with implicit Double<->CGFloat conversion
we need to allow  attempting generic operators for such cases.

```swift
let _: CGFloat = -.pi / 2
```

`-` doesn't have `CGFloat` overload (which might be an oversight),
so in order to preserve type-checking behavior solver can't be
allowed to pick `-(Double) -> Double` based on overload of `/`,
the best possible solution would be with `/` as `(CGFloat, CGFloat) -> CGFloat`
and `-` on a `FloatingPoint` protocol.
…al type

This wasn't a problem before since locator wasn't really used by
`ExprRewritter:coerceToType` but with Double<->CGFloat conversion
it needs the locator to be anchored at a rewritten expression instead
of the original one to form a correct implicit initializer call.
…t conversions

Change the conversion rule to favor any number of widenings (CGFloat -> Double)
over even a single narrowing conversion and if there is no way to avoid narrowing
(due to contextual requirements) attempt it as late as possible (the deeper in
the AST that conversion is located the higher its score).

This is a generally better rule when it comes to rounding and information
loss that would result from narrowing conversions.
…onversion

Doing so resulted in performance impact due to the number of partial
solutions increase.
Just like generic overloads, `shrink` should always avoid any solutions
with implicit conversions.

Reducing disjunction domains becaused on solutions with implicit
conversions could have negative performance impact due to the
increase in total number of solutions that have to be examined
as a result.
@xedin xedin force-pushed the implicit-cgfloat-conversion branch from 52607fb to 0e6198d Compare March 17, 2021 07:19
@xedin
Copy link
Contributor Author

xedin commented Mar 17, 2021

@swift-ci please test source compatibility

Copy link
Collaborator

@xwu xwu left a comment

Choose a reason for hiding this comment

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

I'm excited about this! Just some nits...

include/swift/AST/Types.h Outdated Show resolved Hide resolved
include/swift/Basic/LangOptions.h Outdated Show resolved Hide resolved
include/swift/Sema/Constraint.h Outdated Show resolved Hide resolved
include/swift/Sema/ConstraintSystem.h Outdated Show resolved Hide resolved
include/swift/Sema/ConstraintSystem.h Outdated Show resolved Hide resolved
lib/Sema/CSGen.cpp Outdated Show resolved Hide resolved
lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved
lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved
lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved
lib/Sema/CSStep.cpp Outdated Show resolved Hide resolved
@xedin xedin marked this pull request as ready for review March 29, 2021 17:46
@xedin
Copy link
Contributor Author

xedin commented Mar 29, 2021

@swift-ci please test source compatibility

@xedin
Copy link
Contributor Author

xedin commented Mar 29, 2021

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Mar 29, 2021

Windows failure:

Failed Tests (1):
  Swift(windows-x86_64) :: Concurrency/Runtime/async_task_detached.swift

@xedin
Copy link
Contributor Author

xedin commented Mar 30, 2021

@swift-ci please test source compatibility release

@xedin
Copy link
Contributor Author

xedin commented Mar 31, 2021

Release suite failed with 05:38:56 Caused: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@79e5e9aa:macOS-21": Remote call on macOS-21 failed. The channel is closing down or has closed down. Since SE-0307 got accepted I'm going to land this PR.

@xedin xedin merged commit f00c578 into swiftlang:main Mar 31, 2021
@xedin xedin changed the title [DNM][TypeChecker] Implement Double <-> CGFloat implicit conversion [TypeChecker] Implement Double <-> CGFloat implicit conversion Mar 31, 2021
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - a559ca8

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - a559ca8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants