Skip to content

[Sema] Report unused newValue when getter is accessed by member reference#14256

Closed
KingOfBrian wants to merge 2 commits intoswiftlang:swift-4.1-branchfrom
KingOfBrian:bugfix/SR-964-As-Member
Closed

[Sema] Report unused newValue when getter is accessed by member reference#14256
KingOfBrian wants to merge 2 commits intoswiftlang:swift-4.1-branchfrom
KingOfBrian:bugfix/SR-964-As-Member

Conversation

@KingOfBrian
Copy link
Copy Markdown
Contributor

This PR fixes a missed scenario in an earlier fix for SR-964 which warns if a getter is accessed when the newValue parameter of a setter is not used. The previous fix, disappointingly misses the case of variables inside of classes or structs and only works on global variables. I experienced a bit of unit-test tunnel vision, and didn't test the scenario I was hoping to fix 😬.

I made this PR against 4.1 in hopes that it could land in the next release, even though I'm guessing it is a bit of a long shot!

Really ™️ Resolves SR-964.

@milseman milseman requested review from lattner and xedin February 19, 2018 00:05
@milseman
Copy link
Copy Markdown
Member

Much of this is pretty old code. @lattner or @xedin, do you think you can review this change? Do you think this is important enough to nominate for Swift-4.1?

@milseman
Copy link
Copy Markdown
Member

@swift-ci please test

@swift-ci
Copy link
Copy Markdown
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 63519a3

@swift-ci
Copy link
Copy Markdown
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 63519a3

@milseman
Copy link
Copy Markdown
Member

@swift-ci please test

@swift-ci
Copy link
Copy Markdown
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 63519a3

@swift-ci
Copy link
Copy Markdown
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 63519a3

@tkremenek
Copy link
Copy Markdown
Member

tkremenek commented Mar 10, 2018

This change should be proposed on master first before being pulled into a release branch. Release branches do not forward to master.

Please propose this change on master first! Thank you.

@tkremenek tkremenek closed this Mar 10, 2018
@tkremenek
Copy link
Copy Markdown
Member

@KingOfBrian sorry for not re-directing sooner. I may be mistaken but this does not look already on master. Please create a new PR against master to review the change there. It is very late to take this change for 4.1, but this looks promising!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants