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

[Sema] Emit a diagnostic when extending a protocol with a redundant requirement #20503

Merged
merged 12 commits into from Nov 16, 2018
Merged

[Sema] Emit a diagnostic when extending a protocol with a redundant requirement #20503

merged 12 commits into from Nov 16, 2018

Conversation

theblixguy
Copy link
Collaborator

@theblixguy theblixguy commented Nov 11, 2018 •
edited

This PR adds a change to emit a warning diagnostic when extending a protocol with a redundant requirement.

Resolves SR-9218.

@theblixguy theblixguy changed the title [AST] Emit a diagnostic if an extension contains a redundant requirement [AST] Emit a diagnostic when extending a protocol with a redundant requirement Nov 11, 2018
harlanhaskins
harlanhaskins requested changes Nov 12, 2018
View reviewed changes
lib/AST/TypeCheckRequests.cpp Outdated Show resolved Hide resolved
@harlanhaskins
Copy link
Collaborator

@rudkx Does this change look good to you?

@harlanhaskins
Copy link
Collaborator

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator Author

theblixguy commented Nov 13, 2018 •
edited

Seems like some tests related to SIL/SILOptimizer and IRGen are broken (Assertion failed: (Val && "isa<> used on a null pointer") in most of them). I wonder if it's because the owner and/or Requirement is nil and I am trying to get the type?

The test Constraints.rdar37790062 also needs to be updated because it produces the redundant requirement warning: requirement of 'Self.V.V' to 'A' is redundant in an extension of 'A'. I need to look into the test source to see if it's actually correct or a bug.

I think I have fixed now, running check swift no longer throws those errors. I'll push the changes now!

@xedin
Copy link
Member

xedin commented Nov 13, 2018

@swift-ci please smoke test

@harlanhaskins
Copy link
Collaborator

@xedin Think this is good to merge?

theblixguy and others added 2 commits November 15, 2018 18:41
@theblixguy
[sema] fix style
81f797d
@harlanhaskins @theblixguy
[test] Add another test case for redundant requirement
3faf202 
Co-Authored-By: theblixguy <suyashsrijan@outlook.com>
@theblixguy theblixguy changed the title [AST] Emit a diagnostic when extending a protocol with a redundant requirement [Sema] Emit a diagnostic when extending a protocol with a redundant requirement Nov 15, 2018
@harlanhaskins
Copy link
Collaborator

@swift-ci please test

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 55236b4

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - 55236b4

@theblixguy
[test] fix failing test
3dfff6f 
The test was failing because A has already been declared as a typealias.
@theblixguy
Copy link
Collaborator Author

@harlanhaskins can you run the tests again? Seems like A was already defined as a type alias in the test file, so the test failed because of invalid redeclaration. I have fixed it now!

@harlanhaskins
Copy link
Collaborator

Ah! My apologies.

@swift-ci please test

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - 3faf202

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 3faf202

@theblixguy
Copy link
Collaborator Author

All tests have passed 🎉

@harlanhaskins
Copy link
Collaborator

Thanks, @theblixguy!!

@harlanhaskins harlanhaskins merged commit 0d9f634 into apple:master Nov 16, 2018
@theblixguy theblixguy deleted the fix/SR-9218 branch November 16, 2018 05:37
@theblixguy theblixguy mentioned this pull request Nov 11, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xwu xwu xwu left review comments

@harlanhaskins harlanhaskins harlanhaskins approved these changes

@xedin xedin xedin approved these changes

@DougGregor DougGregor Awaiting requested review from DougGregor

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
5 participants
@theblixguy @harlanhaskins @xedin @swift-ci @xwu