new_ret_no_self false positives #3338
Conversation
|
|
||
| impl TupleReturnerOk { | ||
| // should not trigger lint | ||
| pub fn new() -> (Self, u32) { unimplemented!(); } |
There was a problem hiding this comment.
((Self, u32), u32) Will also trigger the lint. I'm not sure at all how strict we should be. I mean it seems perfectly reasonable to allow (Option<Self>, u32) or at least Option<(Self, u32)>. This might get out of hand quickly.
If you want to cover a lot of cases, you can call walk on the return type (giving you an iterator) and iterate over everything.
That would still cause false positives for struct Foo(pub Bar); impl Bar { fn new() -> Foo { ... } } but we need to restrict ourselves somewhere I guess.
There was a problem hiding this comment.
Thank you for the tip, that sounds like a much more manageable approach. I pushed the commit below just to capture my progress, but I'll go back and try rewriting this to use walk.
|
The suggestion to use |
|
I've added test cases which I believe cover all of the issues from #3313, and removed a clippy allow annotation within the clippy code base which should not be necessary anymore (we were returning Assuming we don't want to make the lint more strict I think this is good to go. Let me know if you see anything that can be improved. |
|
Any thoughts on why the travis build failed? I can't make out the reason looking at the logs. |
|
I restarted the job. |
|
For completeness (#3338 (comment)) test cases for |
|
@flip1995 Thanks for the feedback. I've added those test cases. Let me know if you see any additional issues. |
|
LGTM now! bors r+ |
3338: new_ret_no_self false positives r=flip1995 a=JoshMcguigan ~~WORK IN PROGRESS~~ I plan to fix all of the false positives in #3313 in this PR, but I wanted to open it now to start gathering feedback. In this first commit, I've updated the lint to allow tuple return types as long as `Self` shows up at least once, in any position of the tuple. I believe this is the broadest possible interpretation of what should be allowed for tuple return types, but I would certainly be okay making the lint more strict. fixes #3313 Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>
|
bors r- |
Canceled |
|
It seems that bors also needs a bors r+ |
3338: new_ret_no_self false positives r=flip1995 a=JoshMcguigan ~~WORK IN PROGRESS~~ I plan to fix all of the false positives in #3313 in this PR, but I wanted to open it now to start gathering feedback. In this first commit, I've updated the lint to allow tuple return types as long as `Self` shows up at least once, in any position of the tuple. I believe this is the broadest possible interpretation of what should be allowed for tuple return types, but I would certainly be okay making the lint more strict. fixes #3313 Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>
new_ret_no_self will still fail on the clippy packaged with rust 1.31.0. there was a fix to recursively walk types so that Result<Self, E> wouldn't fail, but that didn't make it into the stable release yet. rust-lang/rust-clippy#3338
The lint was changed to be more lenient than the documentation implies in PR rust-lang#3338. Related issue rust-lang#3313
WORK IN PROGRESSI plan to fix all of the false positives in #3313 in this PR, but I wanted to open it now to start gathering feedback.
In this first commit, I've updated the lint to allow tuple return types as long as
Selfshows up at least once, in any position of the tuple. I believe this is the broadest possible interpretation of what should be allowed for tuple return types, but I would certainly be okay making the lint more strict.fixes #3313