Skip to content
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

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

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
Collaborator

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
Collaborator

@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!

lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
@LucianoPAlmeida
Copy link
Collaborator 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
Collaborator Author

Right, this is ready :) @hamishknight @xedin

theblixguy
theblixguy reviewed Jun 15, 2020
View reviewed changes
include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
hamishknight
hamishknight reviewed Jun 15, 2020
View reviewed changes
include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.cpp Show resolved Hide resolved
@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
Copy link
Collaborator 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
Collaborator Author

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

hamishknight
hamishknight reviewed Jun 22, 2020
View reviewed changes
Copy link
Collaborator

@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

lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.h Outdated Show resolved Hide resolved
lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved
@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
hamishknight
hamishknight reviewed Jun 25, 2020
View reviewed changes
lib/Sema/CSDiagnostics.cpp Outdated Show resolved Hide resolved
lib/Sema/CSDiagnostics.h Show resolved Hide resolved
lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved
hamishknight
hamishknight approved these changes Jun 25, 2020
View reviewed changes
Copy link
Collaborator

@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
Collaborator 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
Member

@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!

lib/Sema/CSDiagnostics.h Outdated Show resolved Hide resolved
@LucianoPAlmeida
Copy link
Collaborator Author

@swift-ci please smoke test

@LucianoPAlmeida
Copy link
Collaborator 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
Member

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
Member

xedin commented Jun 25, 2020

@swift-ci please smoke test Windows platform

@LucianoPAlmeida LucianoPAlmeida merged commit 43fd786 into apple:master Jun 25, 2020
@LucianoPAlmeida
Copy link
Collaborator 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.

None yet
4 participants
@LucianoPAlmeida @xedin @theblixguy @hamishknight