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

Add lint to suggest into() to construct reference-counted slice #2807

Closed

Conversation

rudyardrichter
Copy link

To resolve #2623.

@rudyardrichter rudyardrichter force-pushed the use_shared_from_slice branch 2 times, most recently from fd28667 to ebfda29 Compare May 27, 2018 23:21
@rudyardrichter
Copy link
Author

Hi @oli-obk, here's what I started on for #2623. A few questions—how can I print debug information from running the lint? I tried something like this:

cx.sess().span_note(expr.span, &format!("{:?}", ty))

(from that article linked in CONTRIBUTING.md) but it looks like that's outdated. I'm not positive though since it seems like this doesn't actually run the lint at all when I do either of these:

TESTNAME=ui/use_shared_from_slice cargo test --test compile-test

cargo run --bin clippy-driver -- -L ./target/debug tests/ui/use_shared_from_slice.rs

What else do I need to change to get the lint running?

@oli-obk
Copy link
Contributor

oli-obk commented May 28, 2018

The dogfood test complains about

warning: unused import: `LintContext`
 --> clippy_lints/src/use_shared_from_slice.rs:2:57
  |
2 | use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass};
  |                                                         ^^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default
error: useless use of `format!`
  --> clippy_lints/src/use_shared_from_slice.rs:59:21
   |
59 |                     format!("TODO"),
   |                     ^^^^^^^^^^^^^^^ help: consider using .to_string(): `"TODO".to_string()`
   |
   = note: `-D useless-format` implied by `-D clippy`
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.205/index.html#useless_format

@rudyardrichter
Copy link
Author

Sorry, I had committed it after removing some things for debugging. After 5958ddf the tests are passing but I don't think it runs the lint (I don't see anything happen even if I put a panic in check_expr).

@oli-obk
Copy link
Contributor

oli-obk commented May 28, 2018

You need to add the lint to lib.rs.

Also try running the utils/update_lints.py script first

@rudyardrichter
Copy link
Author

Yeah I just ran util/update_lints.py and didn't actually change lib.rs myself. It looks correct here

@rudyardrichter
Copy link
Author

Ah, sorry, didn't see that running the script didn't add a register_late_lint_pass. Thanks—I'll get back to working on this.

@rudyardrichter
Copy link
Author

@oli-obk would you mind checking up on the progress here? I fixed up check_expr and moved the match_type_parameter out from a different lint to the utils to use it for this one, and added the check_local for checking type declarations.

I noticed LateLintPass has a check_struct_field function and thought I might add the same type declaration check for struct fields, but I couldn't find much relevant code or documentation to figure that out, so I've left that out.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling we're overlooking some cases here. Can you run your lint on a bunch of crates like cargo, rls or other big crates to see what kind of situations trigger your lints there?

if let Some(arg) = args.get(0);

then {
if arg_is_vec {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a little dangerous, because the message might appear even if the user has no way to change the type. For Rc<Vec<[T]>> this is is kinda fine, because the suggestion will still work, but for Rc<String> I'm not sure if .as_str().into() is equivalent.

@phansch phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Aug 29, 2018
@phansch
Copy link
Member

phansch commented Nov 1, 2018

ping @rudyardrichter, do you still want to finish this up?

@rudyardrichter
Copy link
Author

@phansch Yeah, I will get back to this and try running this lint like @oli-obk suggested. I may need some suggestions for how to lint the Rc<String> case

@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 19, 2018
@flip1995
Copy link
Member

Ping @rudyardrichter. I'm going over old PRs, that were abandoned by us reviewers (sorry for that!) or by the authors. Are you still interested in completing this?

@rudyardrichter
Copy link
Author

Hey @flip1995, probably best to let someone else take this. Apologies.

@flip1995
Copy link
Member

flip1995 commented Jul 1, 2019

No worries. Sorry for not giving you an earlier review. Thanks for your contribution!

@flip1995 flip1995 closed this Jul 1, 2019
@flip1995 flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 1, 2019
@HKalbasi
Copy link
Member

finished in #6044
@rustbot label -S-inactive-closed

@rustbot rustbot removed the S-inactive-closed Status: Closed due to inactivity label Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint against Rc<String> (and Rc<Vec<T>>)
6 participants