Skip to content

[SR-12425] Improving diagnostics for key path application on non-convertible root type #31192

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 6 commits into from
Apr 23, 2020
Merged

[SR-12425] Improving diagnostics for key path application on non-convertible root type #31192

merged 6 commits into from
Apr 23, 2020

Conversation

LucianoPAlmeida
Copy link
Contributor

@LucianoPAlmeida LucianoPAlmeida commented Apr 22, 2020 •
edited by xedin
Loading

Follow up to #31140
This improves diagnostics when applying a key path with a root type that cannot be applied to an instance base that has another non-convertible type.

cc @xedin @hamishknight

Resolves: SR-12425
Resolves: rdar://problem/62200986

@xedin xedin self-requested a review April 22, 2020 17:40
xedin
xedin requested changes Apr 22, 2020
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 good, thank you! I have left a couple of comments inline which I think need to be addressed before we merge.

hamishknight
hamishknight reviewed Apr 22, 2020
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.

Looks great!

xedin
xedin reviewed Apr 22, 2020
View reviewed changes
@LucianoPAlmeida
Copy link
Contributor Author

Right, all adjusted I think :)

xedin
xedin approved these changes Apr 22, 2020
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.

Perfect, thank you!

@xedin
Copy link
Contributor

xedin commented Apr 22, 2020

@swift-ci please smoke test

AnthonyLatsis
AnthonyLatsis approved these changes Apr 23, 2020
View reviewed changes
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis left a comment

Choose a reason for hiding this comment

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

Just some really small nits you may want to consider.

@AnthonyLatsis
Copy link
Collaborator

The macOS failure is unrelated, so let us try again.

@AnthonyLatsis
Copy link
Collaborator

@swift-ci please smoke test

@xedin xedin merged commit 288e286 into swiftlang:master Apr 23, 2020
@xedin
Copy link
Contributor

xedin commented Apr 23, 2020

@LucianoPAlmeida Could you please cherry-pick this to 5.3 branch as a simple commit?

@LucianoPAlmeida
Copy link
Contributor Author

@LucianoPAlmeida Could you please cherry-pick this to 5.3 branch as a simple commit?

Sure! :)

@LucianoPAlmeida LucianoPAlmeida mentioned this pull request Apr 23, 2020
Merged
@LucianoPAlmeida LucianoPAlmeida mentioned this pull request Apr 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xedin xedin xedin approved these changes

@AnthonyLatsis AnthonyLatsis AnthonyLatsis approved these changes

@hamishknight hamishknight hamishknight approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LucianoPAlmeida @xedin @AnthonyLatsis @hamishknight