-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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.
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
[Sema] Report unused newValue when getter is accessed in custom setter #21983
[Sema] Report unused newValue when getter is accessed in custom setter #21983
Conversation
print(suspiciousSetter) // expected-warning {{setter argument 'newValue' was never used, but the property was accessed}} expected-note {{did you mean to use 'newValue' instead of accessing the property's current value?}} {{15-31=newValue}} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add one for classes too? I've got extra paranoia now that we let the first time through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, good thought - I also added an extension test. Unfortunately, I did the tests in the github editor since I don't have a working development environment right now! Friday I handed in my work laptop and wont get a new laptop till Monday. The tests were pretty easy so I expect them to pass in CI, but if there's any issues, I can pick it up early next week.
@swift-ci Please test |
Build failed |
Build failed |
@BrianKing-Pear This still missed a case but now I've forgotten what it is and the builds have been cleared. I'll run the tests again. @swift-ci Please smoke test |
Hey @jrose-apple -- Can you kick CI again? I pushed up a fix. New job and computer this week, so unfortunately I don't have the bandwidth to get this branch built locally and really tested, but it is compiling as expected. If it fails, I'll kick a build tonight and get things ironed out. |
@swift-ci Please test |
Build failed |
Build failed |
Hey @jrose-apple -- can you kick the tests again? The failure said something about being tested upstream or something. I forget the exact wording and the failure links are dead now, but I don't think it's anything to do with these changes. |
@swift-ci Please test macOS |
Thanks, Brian! |
🎉 - Thanks for your help @jrose-apple |
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 😬.Really ™️ Resolves SR-964.