Skip to content

[Sema] Diagnose function coercion ambiguity #63904

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Mar 16, 2023
Merged

[Sema] Diagnose function coercion ambiguity #63904

merged 8 commits into from
Mar 16, 2023

Conversation

@LucianoPAlmeida
Copy link
Contributor

@LucianoPAlmeida LucianoPAlmeida commented Feb 25, 2023 •
edited
Loading

In previous PR was handling only failures located in argument mismatch, but ambiguity of overload in coerce subexpr can come from other fixes as well e.g. contextual mismatch, not only located in FunctionArgument. So is a more general approach look for the overload subexpr.

Resolves #63834
@LucianoPAlmeida
Copy link
Contributor Author

LucianoPAlmeida commented Feb 25, 2023

@swift-ci Please smoke test
AnthonyLatsis
AnthonyLatsis reviewed Feb 25, 2023
View reviewed changes
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you!
@LucianoPAlmeida LucianoPAlmeida force-pushed the ambiguous-overload branch 4 times, most recently from 7e88e66 to 33b8fde Compare February 25, 2023 02:28
@LucianoPAlmeida
Copy link
Contributor Author

LucianoPAlmeida commented Feb 25, 2023

@swift-ci Please smoke test
AnthonyLatsis
AnthonyLatsis reviewed Feb 25, 2023
View reviewed changes
AnthonyLatsis
AnthonyLatsis reviewed Feb 25, 2023
View reviewed changes
@AnthonyLatsis
Copy link
Collaborator

AnthonyLatsis commented Feb 25, 2023

@swift-ci please smoke test
@LucianoPAlmeida
Copy link
Contributor Author

LucianoPAlmeida commented Feb 25, 2023

@swift-ci Please smoke test
AnthonyLatsis
AnthonyLatsis reviewed Feb 26, 2023
View reviewed changes
@LucianoPAlmeida LucianoPAlmeida force-pushed the ambiguous-overload branch 2 times, most recently from c37eaa4 to b5e184b Compare February 26, 2023 17:23
hamishknight
hamishknight reviewed Feb 27, 2023
View reviewed changes
hamishknight
hamishknight reviewed Feb 28, 2023
View reviewed changes
Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

Looking good! Just one more comment, and a suggestion for potential future work that could be done here
hamishknight
hamishknight approved these changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

LGTM! I'll let @xedin have the final say though
xedin
xedin reviewed Feb 28, 2023
View reviewed changes
Copy link
Contributor

@xedin xedin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This feels like a workaround for the fact that fixes don't have correct locators for some specific situations where the problem is with structure of the types being coerced, should be adjust locator at the point of fix instead?
@LucianoPAlmeida
Copy link
Contributor Author

LucianoPAlmeida commented Feb 28, 2023

You mean record the fix anchored on the coerce subexpr in those cases?
Or maybe still record fix on the coerce expr but with additional path elt that we can use to simplify down to the subexpr?
@LucianoPAlmeida LucianoPAlmeida changed the title [Sema] Handle coerce subexpr overload in getCalleeLocator [Sema] Diagnose function coercion ambiguity Mar 11, 2023
@LucianoPAlmeida
Copy link
Contributor Author

LucianoPAlmeida commented Mar 11, 2023

@swift-ci Please smoke test MacOS
@xedin xedin self-requested a review March 11, 2023 20:21
@LucianoPAlmeida
Copy link
Contributor Author

LucianoPAlmeida commented Mar 12, 2023

@swift-ci Please smoke test MacOS Platform
xedin
xedin reviewed Mar 13, 2023
View reviewed changes
@LucianoPAlmeida LucianoPAlmeida requested a review from xedin March 14, 2023 01:24
xedin
xedin reviewed Mar 14, 2023
View reviewed changes
xedin
xedin reviewed Mar 15, 2023
View reviewed changes
@LucianoPAlmeida LucianoPAlmeida force-pushed the ambiguous-overload branch 2 times, most recently from 837a109 to af584bd Compare March 15, 2023 02:04
@LucianoPAlmeida LucianoPAlmeida requested a review from xedin March 15, 2023 02:47
xedin
xedin approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!
xedin
xedin reviewed Mar 15, 2023
View reviewed changes
@LucianoPAlmeida
Copy link
Contributor Author

LucianoPAlmeida commented Mar 15, 2023

@swift-ci Please test
@LucianoPAlmeida
Copy link
Contributor Author

LucianoPAlmeida commented Mar 15, 2023

@swift-ci Please test MacOS Platform
@LucianoPAlmeida LucianoPAlmeida merged commit 4d981e3 into swiftlang:main Mar 16, 2023
@LucianoPAlmeida LucianoPAlmeida deleted the ambiguous-overload branch March 16, 2023 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@LucianoPAlmeida @AnthonyLatsis @xedin @hamishknight