Types borrow box #1501

scott-linder · 2017-02-01T16:45:34Z

Lint for #1480 to check for borrowed boxes

mcarton

Looks good!

mcarton · 2017-02-01T19:45:46Z

clippy_lints/src/types.rs

+                                               BOX_VEC,
+                                               ast_ty.span,
+                                               "you seem to be trying to use `&Box<T>`. Consider using just `&T`",
+                                               "replace `&Box<T>` with simply `&T`");


Could you use span_suggestion here?

I just need to read through some other examples and I think I can use span_suggestion. Something like mcarton@44daa8b?

mcarton · 2017-02-01T19:47:11Z

tests/compile-fail/borrow_box.rs

+#![allow(boxed_local)]
+#![allow(blacklisted_name)]
+
+pub fn test(foo: &Box<bool>) { //~ ERROR you seem to be trying to use `&Box<T>`


For completeness, could you had a test with a &Box<T> in a struct, in an expression (eg. let foo: &Box<T>), etc.?

I will add some more exhaustive tests

In the new tests, the type of a binding (e.g. let foo: &Box<bool>) is not detected by the lint. It seems like the TypePass needs an implementation for check_expr in order to catch this; is my understanding correct?

This will also have the effect of adding this check to the other two lints in the same pass (BOX_VEC and LINKEDLIST); that seems fine, but I just want to make sure that is desired.

I am also going to add a test for associated constants/types on a trait.

After thinking about it, let foo: &Box<T> should probably not be linted about:

let foo: &Box<T> = some_extern_crate::bar();

It's not your fault if the crate returns that. You can't change it.

uhm... let foo: &T = some_extern_crate::bar() will work just fine, even if that function returns a &Box<T>: https://is.gd/kdISz8

scott-linder · 2017-02-02T19:07:34Z

So after looking into testing a few more cases, I am wondering what to check. For example, the current TypePass (that is, lints BOX_VEC and LINKEDLIST) check trait items, but not impl items. Is this intentional?

For trait items, TraitItemKind::{Const,Type} will only have a real type to check in the case of associated constants and default associated types, both of which are behind a feature gate currently.

For impl items, #605 prevents checking methods, and I want to be sure the same is not true for associated types/constants.

As of right now I have tried to add the following tests:

pub fn test2() {
    let foo: &Box<bool>; //~ ERROR you seem to be trying to use `&Box<T>`
}

trait Test4 {
    type T;
}

impl<'a> Test4 for Test3<'a> {
    type T = &'a Box<bool>; //~ ERROR you seem to be trying to use `&Box<T>`
}

But neither is actually detected. Should I implement check_expr and check_impl_item to catch them?

mcarton · 2017-02-02T20:00:35Z

the current TypePass (that is, lints BOX_VEC and LINKEDLIST) check trait items, but not impl items. Is this intentional?

Yes. If you define a trait with one of those it's bad. If you implement a trait with those, you cannot change that, it's not your fault (unless you've also defined the trait, but then you'll have the warning there anyway).

oli-obk · 2017-02-04T08:09:11Z

But neither is actually detected. Should I implement check_expr and check_impl_item to catch them?

I think for the first it is check_local. You need to be careful not to break the other lints, but there should be tests for those. Maybe add a boolean parameter to the check_ty function which decides whether it's called from one of the current functions or one of the new ones.

oli-obk · 2017-03-24T09:27:01Z

Are you planning to continue this PR or do you want someone to take it over?

scott-linder · 2017-03-24T12:35:59Z

I would like to finish it, but I don't think I have the time right now. I'm sorry for squating on it. I will watch this PR to see how the rest is done and pick up another issue after my finals.

oli-obk · 2017-03-24T12:49:22Z

I would like to finish it, but I don't think I have the time right now. I'm sorry for squating on it.

No worries! Just wanted to check up on it.

oli-obk · 2017-06-09T18:12:32Z

Just a reminder that you have a PR open here. No need to hurry or anything.

scott-linder · 2017-06-09T23:12:13Z

I am going to dedicate more time to this. I rebased onto master, and I notice some changes in how testing works. Are the "annotated source" style tests gone? Should I just add an example for this lint to the clippy_tests crate?

scott-linder · 2017-06-09T23:17:46Z

I think I understand the new system after rereading the contributing file. I will move the test over and get back up to speed with what this lint should cover.

scott-linder · 2017-06-11T03:58:58Z

@mcarton I took a crack at adding a span_suggestion, by digging down through Box<T> to get a snippet of the inner type. I formatted this (along with the optional lifetime and mutability), and it works for a couple trivial cases, but I don't know if it is actually "correct."

I will work on the remaining check_* functions tomorrow.

oli-obk · 2017-06-11T05:19:09Z

I think you need to rerun the tests and commit the changes, since you changed the output

scott-linder · 2017-06-11T14:52:52Z

Thank you for sticking with me, I hope after I get through one lint I can make a lot more progress on other lints alone. I had been running only my example, which doesn't update the stderr file.

scott-linder · 2017-06-11T16:17:47Z

I added check_local to the pass and pushed up the new stderr. All of the current tests seem to be passing. Should I add more? Are there more checks I could add for this lint?

I am also not perfectly happy with the extra bool flag needed for check_ty now, but the alternatives seem to require duplicating the code for recursively checking types. Any suggestions are welcome for making it more obvious what is going on.

Adds a boolean flag to indicate whether the current type in `check_ty` is in a local declaration, as only the borrowed box lint should consider these types.

Add negative tests for types in local declarations in the `LINKEDLIST` and `BOX_VEC` lints. They share a pass with `BORROWED_BOX` which does check local delclarations.

scott-linder · 2017-06-11T16:33:17Z

I added tests to the other lints in the same pass, to make sure they are not checking local declarations.

oli-obk · 2017-06-12T06:14:55Z

clippy_lints/src/types.rs

+                    if let Some(def_id) = opt_def_id(def) {
+                        if Some(def_id) == cx.tcx.lang_items.owned_box() {
+                            if_let_chain! {[
+                                let QPath::Resolved(None, ref path) = *qpath,


You can merge the above two ifs into this if let chain.

oli-obk · 2017-06-12T06:17:28Z

clippy_tests/examples/borrow_box.rs

+    println!("{:?}", foo)
+}
+
+pub fn test2() {


Can you add a trait method taking a box reference and an impl to show that only the trait is linted and not the impl?

oli-obk · 2017-06-12T06:18:42Z

Two nits, then we are good to go.

oli-obk · 2017-06-12T06:20:03Z

The bool flag is fine with me. You could use an enum if you wanted, but it's not necessary. Maybe just add a comment explaining the argument?

scott-linder · 2017-06-12T13:00:23Z

I added the test and merged the ifs, and I also added a short comment about the flag.

Has been addressed

oli-obk · 2017-06-13T09:31:31Z

Great work. Thanks.

scott-linder · 2017-06-13T11:57:15Z

Thanks @oli-obk and @mcarton for all the help!

mcarton previously requested changes Feb 1, 2017

View reviewed changes

scott-linder force-pushed the types-borrow-box branch from ab4b5f2 to 4d076ac Compare June 9, 2017 23:10

scott-linder added 9 commits June 11, 2017 12:19

check for borrowed box types

711cad1

Fix typo in types check

a4c4da1

Actually fix the lint applied

80cb48c

Recurse into inner type when not &Box<T>

e6eaa72

Add more exhaustive tests for borrow_box

c061464

Move old-style test to examples

663688f

Use span_suggestion in borrowed_box lint

deef81a

Commit updated example stderr

c29f5ea

Add check_local to TypePass for BORROWED_BOX

74ebe6e

Adds a boolean flag to indicate whether the current type in `check_ty` is in a local declaration, as only the borrowed box lint should consider these types.

scott-linder force-pushed the types-borrow-box branch from f0fc4c0 to 74ebe6e Compare June 11, 2017 16:24

Test for local types in LINKEDLIST and BOX_VEC

54b5205

Add negative tests for types in local declarations in the `LINKEDLIST` and `BOX_VEC` lints. They share a pass with `BORROWED_BOX` which does check local delclarations.

oli-obk reviewed Jun 12, 2017

View reviewed changes

scott-linder added 3 commits June 12, 2017 08:38

Merge nested if into adjacent if_let_chain!

49bba31

Document check_ty and its new is_local arg.

1a50755

Test for trait method decl/impl for borrowed box.

5db8647

oli-obk approved these changes Jun 12, 2017

View reviewed changes

oli-obk merged commit 7056018 into rust-lang:master Jun 13, 2017

scott-linder deleted the types-borrow-box branch June 13, 2017 11:55

Manishearth mentioned this pull request May 28, 2018

Writing out a &Box<T> type #1480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Types borrow box #1501

Types borrow box #1501

scott-linder commented Feb 1, 2017

mcarton left a comment

mcarton Feb 1, 2017

scott-linder Feb 2, 2017

mcarton Feb 1, 2017

scott-linder Feb 2, 2017

scott-linder Feb 2, 2017

mcarton Feb 2, 2017

oli-obk Feb 3, 2017

mcarton Feb 3, 2017

scott-linder commented Feb 2, 2017

mcarton commented Feb 2, 2017

oli-obk commented Feb 4, 2017

oli-obk commented Mar 24, 2017

scott-linder commented Mar 24, 2017

oli-obk commented Mar 24, 2017

oli-obk commented Jun 9, 2017

scott-linder commented Jun 9, 2017

scott-linder commented Jun 9, 2017

scott-linder commented Jun 11, 2017

oli-obk commented Jun 11, 2017

scott-linder commented Jun 11, 2017

scott-linder commented Jun 11, 2017

scott-linder commented Jun 11, 2017

oli-obk Jun 12, 2017

oli-obk Jun 12, 2017

oli-obk commented Jun 12, 2017

oli-obk commented Jun 12, 2017

scott-linder commented Jun 12, 2017

oli-obk commented Jun 13, 2017

scott-linder commented Jun 13, 2017

Types borrow box #1501

Types borrow box #1501

Conversation

scott-linder commented Feb 1, 2017

mcarton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scott-linder commented Feb 2, 2017

mcarton commented Feb 2, 2017

oli-obk commented Feb 4, 2017

oli-obk commented Mar 24, 2017

scott-linder commented Mar 24, 2017

oli-obk commented Mar 24, 2017

oli-obk commented Jun 9, 2017

scott-linder commented Jun 9, 2017

scott-linder commented Jun 9, 2017

scott-linder commented Jun 11, 2017

oli-obk commented Jun 11, 2017

scott-linder commented Jun 11, 2017

scott-linder commented Jun 11, 2017

scott-linder commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jun 12, 2017

oli-obk commented Jun 12, 2017

scott-linder commented Jun 12, 2017

oli-obk commented Jun 13, 2017

scott-linder commented Jun 13, 2017