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

borrowed_box lint and Any #1884

Closed
stanislav-tkach opened this issue Jul 12, 2017 · 5 comments
Closed

borrowed_box lint and Any #1884

stanislav-tkach opened this issue Jul 12, 2017 · 5 comments
Assignees
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience. T-middle Type: Probably requires verifiying types

Comments

@stanislav-tkach
Copy link
Contributor

stanislav-tkach commented Jul 12, 2017

I'm not sure that this is an issue, but it can be a little confusing.

I have the following situation: there is a function that works with panic::catch_unwind error (Box<Any + Send + 'static>):

fn foo(e: &Box<Any + Send>) {
    if let Some(s) = e.as_ref().downcast_ref::<&str>() { ... }
    ...
}

Clippy suggests the following:

warning: you seem to be trying to use `&Box<T>`. Consider using just `&T`
 --> src/main.rs:4:11
  |
4 | fn foo(e: &Box<Any + Send>) {
  |           ^^^^^^^^^^^^^^^^ help: try `&Any + Send`

Functions becomes more generic, that for sure, but Any is a specific type: foo call site must be updated otherwise instead of Any that contains a panic error it will take Any that contains Box<Any + Send>.

Probably borrowed_box lint should be disabled for Any type?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 12, 2017

hmm... that definitely is a footgun. Let's special case Any

@oli-obk oli-obk added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages T-middle Type: Probably requires verifiying types labels Jul 12, 2017
@stanislav-tkach
Copy link
Contributor Author

I would like to try to fix this. 😄

@oli-obk
Copy link
Contributor

oli-obk commented Jul 12, 2017

Great! It's probably enough to extract the boxed type (see the boxed_ty() method on Ty) and check whether it's a http://manishearth.github.io/rust-internals-docs/rustc/ty/enum.TypeVariants.html#variant.TyDynamic . The big question is how to figure out whether it's a specific trait. Look around the other lints, especially any doing something with TyDynamic.

@stanislav-tkach
Copy link
Contributor Author

@oli-obk
Sorry, I haven't had time too look at this lately. 😅 And now I'm a little bit stuck here.

You propose to use TyDynamic, but as I can see, in the code rustc::hir::Ty is used, so it is easy to check node type (rustc::hir::Ty_) for TyTraitObject. Is it a right way or not? Probably not, because I cannot find a way to check for the specific trait with TyTraitObject.

As for TyDynamic: check_ty function (inside of which borrowed_box lint is spanned) takes hir::Ty and for boxed_ty I need rustc::ty::TyS. How do I get it?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 24, 2017

Ah right. You need to distinguish between syntactial types and actual types. Syntactical types are what you enter in the code (e.g. Foo for a struct's name). Actual types aren't just a name, but they represent the type with its fields and everything.

You should be able to get the real type via cx.tables.node_id_to_type(ty.id)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience. T-middle Type: Probably requires verifiying types
Projects
None yet
Development

No branches or pull requests

2 participants