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.

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

Writing out a &Box<T> type #1480

Closed
oli-obk opened this issue Jan 27, 2017 · 17 comments
Closed

Writing out a &Box<T> type #1480

oli-obk opened this issue Jan 27, 2017 · 17 comments
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Jan 27, 2017

any &Box<T> can also be a &T, and &T is much more general.

I don't see any use in ever writing &Box<T>

@oli-obk oli-obk added L-unnecessary Lint: Warn about unnecessary code good-first-issue These issues are a good way to get started with Clippy A-lint Area: New lints T-middle Type: Probably requires verifiying types labels Jan 27, 2017
@scott-linder
Copy link

I would like to give this a shot. Would this fit in the types mod, maybe under types::TypePass? Also, does this apply to mutable references to Box<T>?

@scott-linder
Copy link

I went ahead and tried this on my own at scott-linder/rust-clippy@9afc8c933. I hope this is in the ballpark, and I look forward to feedback on it.

@Manishearth
Copy link
Member

Manishearth commented Feb 1, 2017

looks okay to me! Might have been better to add a suggestion but it occurred to me that the suggestion can't be applied without changes elsewhere (to make the types match up) so I'm unsure if we should. Maybe we should, and differentiate with rust-lang/rust#39254 whenever that happens

@scott-linder
Copy link

scott-linder commented Feb 1, 2017

Awesome! Thank you for taking a look.

I agree that the suggestion seems redundant in this case if it is not the full type, but I didn't know if it was possible to omit it.

I also notice that this lint is behind #[warn(box_vec)] on by default, but I do not know where that name comes from.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 1, 2017

You accidentally used BOX_VEC instead of BORROWED_BOX here

@scott-linder
Copy link

Thank you, I pushed up the fix to the same branch.

I believe that was the only other problem I noticed by myself; if you are happy with it I can open a PR, otherwise let me know what else could be changed (including any change or removal of the suggestion).

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 1, 2017

if you are happy with it I can open a PR,

yes, please do so.

In the future you can open a PR even when you still have questions. Then we can discuss them directly on the PR.

@jonas-schievink
Copy link
Contributor

This issue applies to &String and &Vec<T>, too. I've seen both patterns used by quite a few people new to Rust. Or should I open a separate issue for that?

@mcarton
Copy link
Member

mcarton commented Feb 17, 2017

We already have a lint (ptr_arg from memory) for &String and &Vec<T> 🎉

@jonas-schievink
Copy link
Contributor

@mcarton Nice, thanks!

@Manishearth
Copy link
Member

#1501

@onnoowl
Copy link

onnoowl commented Aug 14, 2018

Hi there,
I wanted to ask about how this lint works with Trait Objects...

Say you have the following function signature, that retrieves a static boxed dyn MyTrait instance from a hashmap:

fn get(&'static self, id: u32) -> &'static dyn MyTrait

In my current implementation, my get function actually returns (or want to returns) &Box<dyn MyTrait>. When I try to change the specified return type to satisfy this lint, and instead return &dyn MyTrait, I get this:

the trait `MyTrait` is not implemented for `std::boxed::Box<dyn MyTrait>`

Is there some de-referencing magic that I need to do that I'm not understanding? Or did this lint not really consider the case of Trait Objects when it was added? How can I turn &Box<dyn MyTrait> into &dyn MyTrait?

(Sorry if this is the wrong place for this discussion)

@Manishearth
Copy link
Member

Just &* it. Its the same steps for converting &Box<anything> to &anything

@onnoowl
Copy link

onnoowl commented Aug 14, 2018

(Thanks for the quick reply)
For some reason, in my experiment, the found return type remains &Box<dyn MyTrait> even after performing a &*.

@onnoowl
Copy link

onnoowl commented Aug 14, 2018

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 14, 2018

You need to dereference twice if you start out with &Box<T>, once for the & and once for the Box.

So &**value

@onnoowl
Copy link

onnoowl commented Aug 14, 2018

That worked, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types
Projects
None yet
Development

No branches or pull requests

6 participants