Skip to content

[SR-5688] [Sema] Handle key path component base type on MemberAccessOnOptionalBaseFailure #32376

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 22 commits into from
Jun 25, 2020
Merged

[SR-5688] [Sema] Handle key path component base type on MemberAccessOnOptionalBaseFailure #32376

merged 22 commits into from
Jun 25, 2020

Conversation

LucianoPAlmeida
Copy link
Contributor

The fix was being recorded but it was failing to handle the key path component type.
This just handles the KeyPathComponent locator.

Resolves SR-5688.

hamishknight
hamishknight reviewed Jun 14, 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.

Thanks for the fix!

@LucianoPAlmeida LucianoPAlmeida requested a review from gregomni June 14, 2020 19:53
hamishknight
hamishknight reviewed Jun 14, 2020
View reviewed changes
@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-5688-key-path-optional branch from 136733c to fa11612 Compare June 15, 2020 02:18
@LucianoPAlmeida
Copy link
Contributor Author

@hamishknight @xedin There are still some tests failing I have to look, so I'll ping you when this is ready for a review :)

@LucianoPAlmeida
Copy link
Contributor Author

Right, this is ready :) @hamishknight @xedin

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-5688-key-path-optional branch from 5c5f63d to 4e3f54d Compare June 15, 2020 10:19
theblixguy
theblixguy reviewed Jun 15, 2020
View reviewed changes
hamishknight
hamishknight reviewed Jun 15, 2020
View reviewed changes
@LucianoPAlmeida
[CSDiagnostics] Adjusting MemberAccessOnOptionalBaseFailure to be abl…
6bba692 
…e to handle key path component member base types
@LucianoPAlmeida
[tests] Adding regression tests for SR-5688
e7b1058
@LucianoPAlmeida
[CSDiagnostics] Adjusting source range to diagnose/insert the fixes i…
d44d809 
…n correct location
@LucianoPAlmeida
[tests] Adjusting regression tests to handle the fixits
69409c1
@LucianoPAlmeida
[AST] Creating an helper getSourceRange function for KeyPathExpr::Com…
96267b0 
…ponent
@LucianoPAlmeida
[ConstraintSystem] Store member base type when recording UnwrapOption…
93c49fc 
…alBase fix
@LucianoPAlmeida
[AST] Creating a new diagnostic note for removing optional from writt…
ae6b971 
…en type
@LucianoPAlmeida
[CSDiagnostics] Adjusting logic around MemberAccessOnOptionalBaseFail…
a05d597 
…ure to emit the correct diagnostics and fixes
@LucianoPAlmeida
[tests] Adjusting regression tests to add subscript and key path root…
6ebfd83 
… cases with respective diagnostics
@LucianoPAlmeida
[Diagnostics] Adjusting message to mention base type
2f31e85
@LucianoPAlmeida
[CSDiagnostics] Better naming for method/variable that represents bas…
4df6287 
…e source range
@LucianoPAlmeida
[CSDiagnostics] Adjusting to use the stored base member only when mem…
ca6eab3 
…ber is a key path component.
@LucianoPAlmeida
[Diagnostics] Adjusting minor typos and code
a5028f0
@LucianoPAlmeida
[AST] Adjusting keypath root diagnostic note message for use unwrappe…
9caaf21 
…d type
@LucianoPAlmeida
[CSDiagnostics] Adjusments in MemberAccessOnOptionalBaseFailure diagn…
7b34558 
…ostics as per suggestion
@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-5688-key-path-optional branch from d60f4f7 to 316abfc Compare June 20, 2020 03:09
@LucianoPAlmeida
Copy link
Contributor Author

@hamishknight Thank you for the review
Sorry for taking too long to answer, but I've addressed all things.
So this is ready for review again :)
cc @xedin

@LucianoPAlmeida
Copy link
Contributor Author

Right, now tests are happy so this is ready for review :)

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-5688-key-path-optional branch from c90927b to 74f7651 Compare June 21, 2020 17:14
hamishknight
hamishknight reviewed Jun 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! Just a couple more comments

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-5688-key-path-optional branch from a2d1817 to 8fe2a61 Compare June 24, 2020 12:48
hamishknight
hamishknight reviewed Jun 24, 2020
View reviewed changes
@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-5688-key-path-optional branch 2 times, most recently from 624c9a9 to 8fd3f02 Compare June 24, 2020 19:01
@LucianoPAlmeida
[CSSimplify] Attempt to diagnose InsertExplicitCall for optional func…
7f57de4 
…tion return types when possible on simplifyOptionalObjectConstraint
@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-5688-key-path-optional branch from 8fd3f02 to 7f57de4 Compare June 25, 2020 01:21
hamishknight
hamishknight reviewed Jun 25, 2020
View reviewed changes
@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-5688-key-path-optional branch from c2060c6 to 3a961c8 Compare June 25, 2020 13:07
hamishknight
hamishknight approved these changes Jun 25, 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.

Thanks again! I'll let @xedin have the final say, but this looks good to me!

Please make sure to do a squash-and-merge when merging to ensure the git history is kept clean (congrats on commit access btw!).

@LucianoPAlmeida
Copy link
Contributor Author

Please make sure to do a squash-and-merge when merging to ensure the git history is kept clean (congrats on commit access btw!).

Sure, Thank you for the review @hamishknight :) I've learned some nice new things here \o/

xedin
xedin approved these changes Jun 25, 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.

I have left a minor comment otherwise LGTM!

@LucianoPAlmeida
Copy link
Contributor Author

@swift-ci please smoke test

@LucianoPAlmeida
Copy link
Contributor Author

@xedin The windows build didn't trigger or it just didn't show on the github UI like that one in the other PR?

@xedin
Copy link
Contributor

xedin commented Jun 25, 2020

Looks like it just didn't report the status - https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-windows/4504/

@xedin
Copy link
Contributor

xedin commented Jun 25, 2020

@swift-ci please smoke test Windows platform

@LucianoPAlmeida LucianoPAlmeida merged commit 43fd786 into swiftlang:master Jun 25, 2020
@LucianoPAlmeida
Copy link
Contributor Author

Squashed and merged, Thank you @hamishknight @xedin :)

@LucianoPAlmeida LucianoPAlmeida deleted the SR-5688-key-path-optional branch June 25, 2020 23:08
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

@theblixguy theblixguy theblixguy left review comments

@hamishknight hamishknight hamishknight approved these changes

@gregomni gregomni Awaiting requested review from gregomni

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 @theblixguy @hamishknight