[TypeChecker] Adjust Double<->CGFloat conversion to always preserve its location #59762
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before these changes filtering would not favor some members because their function types have `escaping` bit set when argument function type never has it. Double/CGFloat implicit conversion is one of such cases where `CGFloat.init` overloads have `escaping` bit, which leads to solver checking more overloads then it should. Note that the difference in effects such as `async` is going to be handled by scoring and ranking after partial solution is produced, so overloads that differ only in `async` or `throws` are still going to be solved for as part of "favored" set.
…ts location Unfortunately current approach of making a conversion independent of location doesn't work when conversion is required for multiple arguments to the same call because solver expects that either there are no Double<->CGFloat conversions, or one of them has already been applied which is not the case. The reason why locations weren't preserved in the first place is due to how a solution is applied to AST - AST is mutated first and then, if there are any conversions, they are applied to the already mutated version of original AST. This creates a problem for Double<->CGFloat which depends on an overload choice of injected call and it's impossible to find it based on the mutated AST. But it turns out that this is only an issue in two specific cases - conversions against contextual type and after optional injection. This situations could be mitigated by dropping parts of the locator which are unimportant for the Double<->CGFloat conversion - anchor in case of contextual and `OptionalPayload` element(s) in case of optional injection. Resolves: swiftlang#59374
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
hborla
approved these changes
Jun 29, 2022
This was referenced Jun 29, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unfortunately current approach of making a conversion independent of location
doesn't work when conversion is required for multiple arguments to the
same call because solver expects that either there are no Double<->CGFloat
conversions, or one of them has already been applied which is not the case.
The reason why locations weren't preserved in the first place is due to
how a solution is applied to AST - AST is mutated first and then, if there
are any conversions, they are applied to the already mutated version of
original AST. This creates a problem for Double<->CGFloat which depends
on an overload choice of injected call and it's impossible to find it based
on the mutated AST. But it turns out that this is only an issue in two
specific cases - conversions against contextual type and after optional injection.
This situations could be mitigated by dropping parts of the locator which are
unimportant for the Double<->CGFloat conversion - anchor in case of contextual
and
OptionalPayloadelement(s) in case of optional injection.Resolves: #59374