Warn if the getter is used, but the setter argument is not used#11465
Warn if the getter is used, but the setter argument is not used#11465jrose-apple merged 6 commits intoswiftlang:masterfrom
Conversation
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please smoke test |
|
@shahmishal is @swift-ci doing ok? It seems to bear a grudge against @KingOfBrian |
|
Build failed |
|
Build failed |
|
Looks like one of the checks isn't reporting, and I don't think the two failures are related to this change. Really excited for this warning and would love to get it it 4.1 ! |
|
CC @gottesmm @shahmishal that failure is the futimens one. In case you're still trying to track it down: https://ci.swift.org/job/swift-PR-osx/86/console |
|
@swift-ci please test OS X Platform |
|
Is something blocking this from being merged? I'd love to see it on 4.1 as well 🎉 |
|
Did this ever get reviewed? |
|
CC @jrose-apple. If you have time, could you review this change? |
jrose-apple
left a comment
jrose-apple
left a comment
There was a problem hiding this comment.
The implementation feels a bit funny to me, since that map is normally for stored properties, but I can see how it works nicely with the existing tracking. So my only remaining concern is with the phrasing of the diagnostic.
| (Identifier)) | ||
| WARNING(unused_setter_newvalue, none, | ||
| "setter argument %0 was never used, but the getter was accessed; " | ||
| "consider using %0 or removing %1", |
There was a problem hiding this comment.
I think the "removing" phrasing is confusing. How about something like "setter argument %0 was never used; did you mean to use it instead of accessing the property's current value"?
Ideally, we'd put the warning on one of the uses of the getter, and maybe even have a fix-it if it's just one use, but that's probably more work than you want to do right now.
|
Well, that’s not... quite a rebase. |
|
Hah! was hoping to fix this before anyone noticed. Me and git didn't get along last night. I think my initial rebase was off... and things escalated. I'll clean it up shortly! |
KingOfBrian
force-pushed
the
bugfix/SR-964
branch
from
93d1462 to
f9c84db
Compare
KingOfBrian
force-pushed
the
bugfix/SR-964
branch
from
f9c84db to
4f20f67
Compare
|
Thanks for the feedback @jrose-apple -- I added the fixit, and moved where the error was reported. I track the first |
|
|
||
| /// This is a mapping from a VarDecl property getter to the expression that | ||
| /// used it. | ||
| llvm::SmallDenseMap<VarDecl*, DeclRefExpr*> GetterDeclUsageMap; |
There was a problem hiding this comment.
This map will only ever have one entry, right?
There was a problem hiding this comment.
Yea -- sort of funny, I woke up this morning and that was literally my first thought. I converted it to a pair, but I'm not sure if that's a normal C++ construct to use. Would that make sense to you?
There was a problem hiding this comment.
I think I would just use two instance variables, and not worry about keeping them tied together. You also set each half at a different point in execution anyway.
(Also, may as well make them const.)
| var suspiciousSetter: String { | ||
| get { return "" } | ||
| set { | ||
| print(suspiciousSetter) // expected-warning {{setter argument 'newValue' was never used; did you mean to use it instead of accessing the property's current value?}} expected-note {{do you want to replace 'suspiciousSetter' with 'newValue'?}} |
There was a problem hiding this comment.
If you're going to have the note, I think the "did you mean" part belongs in the note, rather than describing the fix-it. Also, you can test fix-its with the expected-* syntax! Use {{1-2=foo}} syntax after the diagnostic text to mean "replace columns 1 to 2 with 'foo'".
There was a problem hiding this comment.
Yea, that makes sense. I thought a note was more appropriate since the fixit isn't always the correct course of action. I can just do the warning if you think that makes more sense.
Otherwise, does this language look good?
warning: setter argument 'newValue' was never used, but the property was accessed
note: did you mean to use 'newValue' instead of accessing the property's current value?
There was a problem hiding this comment.
Sure, that seems fine, and I agree that the separate note is a good idea.
|
@swift-ci Please test |
|
Build failed |
7 participants
This PR generates a warning if a setters getter is used, but the setter argument is not used. This is a common bug that occurs when a custom setter is implemented, but the getter is accidentally used instead of the setter argument.
This PR also removes some code that added method parameters to the checker. It was only used to warn about var parameters that were not mutated. Since var parameter support was dropped in 3.0, this was just busy work.
This was a rebase off of this PR that wasn't cooperating with CI.
Resolves SR-964.