Skip to content

[Typechecker] Reapply fix for SR-11298 #27639

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 12 commits into from
Oct 15, 2019
Merged

[Typechecker] Reapply fix for SR-11298 #27639

merged 12 commits into from
Oct 15, 2019

Conversation

theblixguy
Copy link
Collaborator

@theblixguy theblixguy commented Oct 12, 2019 •
edited
Loading

This re-applies the previously accepted fix for SR-11298.

  1. The first commit unreverts Revert "Merge pull request #27057 from theblixguy/unrevert/SR-11298" #27611
  2. The second commit applies a tweak so we don't incorrectly mark explicitly mutating methods in a class-constrained protocol extension as nonmutating.

Resolves SR-11298

theblixguy
theblixguy commented Oct 12, 2019
View reviewed changes
slavapestov
slavapestov reviewed Oct 12, 2019
View reviewed changes
@theblixguy
Copy link
Collaborator Author

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator Author

@swift-ci please test source compatibility

@theblixguy
Copy link
Collaborator Author

Debug source compat failure is an unrelated UPASS.

LucianoPAlmeida
LucianoPAlmeida reviewed Oct 13, 2019
View reviewed changes
jrose-apple
jrose-apple reviewed Oct 14, 2019
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple 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 sticking with this! I think this'll assuage the core team's concerns.

jrose-apple
jrose-apple reviewed Oct 14, 2019
View reviewed changes
@theblixguy
Copy link
Collaborator Author

@swift-ci please smoke test

@jrose-apple
Copy link
Contributor

Debug will fail again but just that one project

@swift-ci Please test source compatibility

@theblixguy
Copy link
Collaborator Author

theblixguy commented Oct 15, 2019 •
edited
Loading

/Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/test/NameBinding/name_lookup.swift:615:41: error: unexpected error produced: property 'x' references itself
01:16:43 struct PatternBindingWithTwoVars1 { var x = 3, y = x }

Weird... I have no idea how my changes could possibly cause this. Let me rebase and try again.

EDIT: Could be due to #27668 where this test case was last touched.

@theblixguy
Revert "Revert "Merge pull request #27057 from theblixguy/unrevert/SR…
70d77e9 
…-11298""

This reverts commit 7725818.
@theblixguy
[Typechecker] Do not incorrectly mark explicitly mutating methods as …
6ca121d 
…non-mutatingif we're in a class-constrained protocol extension
@theblixguy
[AST] Use getExtendedProtocolDecl() to check for protocol extension a…
6665e71 
…nd use getDeclaredInterfaceType() instead of getSelfTypeInContext() - both are cheaper
@theblixguy
[Test] Update fix-it range for test case and add comments
4473cb8
@theblixguy
[AST] Remove redundant 'return false' from 'hasValueSemantics()'
37b4020
@theblixguy
[AST] Remove old code in emitLetToVarNoteIfSimple() that prevented th…
e18a49a 
…e fix-it from being applied in some cases
@theblixguy
[Test] Check if the fix-its are being offered and being correctly app…
09067cf 
…lied
@theblixguy
[Typechecker] Add a comment to explain why we return SelfAccessKind::…
77e1626 
…Mutating if decl is in a class-constrained protocol extension
@theblixguy
[Test] Adds another test case
528558d
@theblixguy
[Changelog] Remove workaround for SR-11298
fe14b0f
@theblixguy
[Typechecker] Fix typo and update comment in SelfAccessKindRequest::e…
952a561 
…valuate
@theblixguy
[Test] Add more test cases
2033201
@theblixguy
Copy link
Collaborator Author

theblixguy commented Oct 15, 2019 •
edited
Loading

@swift-ci please smoke test

(will run source compat if this passes)

@theblixguy
Copy link
Collaborator Author

theblixguy commented Oct 15, 2019 •
edited
Loading

Validation order changed, this PR should fix that failing test: #27679

@theblixguy
Copy link
Collaborator Author

Alright, let’s try again:

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator Author

@swift-ci please test source compatibility

@theblixguy
Copy link
Collaborator Author

Great, all good (except that UPASS). Let's :shipit: and hope it doesn't get reverted again!

@theblixguy theblixguy merged commit 008374b into swiftlang:master Oct 15, 2019
@theblixguy theblixguy deleted the fix/sr-11298-third-times-a-charm branch October 15, 2019 23:05
@theblixguy theblixguy mentioned this pull request Oct 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavapestov slavapestov slavapestov left review comments

@LucianoPAlmeida LucianoPAlmeida LucianoPAlmeida left review comments

@jrose-apple jrose-apple jrose-apple 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
@theblixguy @jrose-apple @slavapestov @LucianoPAlmeida