Skip to content

[CS] Consolidate logic forming locators to callees #24826

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 4 commits into from
May 16, 2019

Conversation

hamishknight
Copy link
Contributor

@hamishknight hamishknight commented May 16, 2019

This PR adds ConstraintSystem::getCalleeLocator, which forms a locator that describes the callee of a given expression. This function is then used to replace various places where this logic is duplicated, and in doing so improves a couple of diagnostics due to catching cases that were previously missed. Most notably, it's now more lenient when forming a ConstructorMember locator for an apply. Previously such a locator was only formed for an apply of a TypeExpr, however now it's formed for an apply of an expr of AnyMetatypeType. This better matches the logic in simplifyApplicableFnConstraint and allows us to better diagnose things like type(of: x)(...).

getCalleeLocator is then used when checking if we can diagnose an ambiguity with fixes, which allows us to have fixes anchored to both the apply and to its function expr (where the former could be looking at an argument of the apply, and the latter could be looking at a requirement of the callee).

Finally, this PR adds a convenience getDeclOrNull function to OverloadChoice, and fixes a simple compiler crasher. Given how straightforward these changes were, I didn't feel it was worth splitting them into a separate PR.

Resolves SR-10694 and SR-10696.

This commit adds `ConstraintSystem::getCalleeLocator`, which forms a
locator that describes the callee of a given expression. This function
is then used to replace various places where this logic is duplicated.

This commit also changes the conditions under which a ConstructorMember
callee locator is formed. Previously it was formed for a CallExpr with a
TypeExpr function expr. However, now such a locator is formed if the
function expr is of AnyMetatypeType. This allows it to be more lenient
with invalid code, as well as work with DotSelfExpr.

Resolves SR-10694.
Currently we check that the fixes share the same anchor, however this
doesn't account for the case where, given an ApplyExpr, one fix is
anchored on its function expr, and another is anchored on the apply
itself (because the former fix might be looking at the callee's
requirements, and the latter fix might be looking at an argument of
the call).

This commit changes the logic such that we check that fixes share the
same callee locator, which covers the above case. In addition, now
that we have the callee locator, we can use this to find the overload
directly.
This helps clean up a bunch of call sites.
@hamishknight
Copy link
Contributor Author

cc. @xedin

@xedin xedin self-requested a review May 16, 2019 18:11
Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

Nice!

@slavapestov
Copy link
Contributor

@swift-ci Please smoke test

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.

Great, thanks! That's exactly the refactoring I wanted to do when I had some time, I have also talked about this with @sl, we have to many of these checks already in CSDiagnostics.

@xedin xedin merged commit a20188b into swiftlang:master May 16, 2019
@sl
Copy link
Contributor

sl commented May 16, 2019

Thanks for pointing this out @xedin. This looks awesome! Glad to see this ended up getting cleaned up even sooner than expected 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants