Skip to content

Warn about using Arc::new() when using the vec![value; size] macro. #8719

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

Closed
davidbeesley opened this issue Apr 18, 2022 · 8 comments · Fixed by #8769
Closed

Warn about using Arc::new() when using the vec![value; size] macro. #8719

davidbeesley opened this issue Apr 18, 2022 · 8 comments · Fixed by #8769
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy

Comments

@davidbeesley
Copy link

What it does

This lint would warn against using Arc::new in the vec![value; size] macro, because it computes the value once and then clones the value size times instead of generating a new Arc each of the size times.

If the user truly wants a vector of Arcs all pointing to the same arc, then they should make that obvious by constructing the arc before constructing the vector.

Lint Name

No response

Category

No response

Advantage

No response

Drawbacks

No response

Example

  #[test]
  fn unexpected_vec_macro_behavior() {
      let v = vec![Arc::new(Mutex::new(0usize)); 100];
      *v[0].lock().unwrap() = 20;
      *v[1].lock().unwrap() = 30;
      assert_eq!(*v[0].lock().unwrap(), 20);
  }

Could be written as:

    #[test]
    fn vec_behavior_is_more_obvious() {
        let cloned_arc = Arc::new(Mutex::new(0usize));
        let v = vec![cloned_arc; 100];
        *v[0].lock().unwrap() = 20;
        *v[1].lock().unwrap() = 30;
        assert_eq!(*v[0].lock().unwrap(), 30);
    }
@davidbeesley davidbeesley added the A-lint Area: New lints label Apr 18, 2022
@Serial-ATA
Copy link
Contributor

@rustbot label +good-first-issue

@rustbot rustbot added the good-first-issue These issues are a good way to get started with Clippy label Apr 24, 2022
@yonip23
Copy link
Contributor

yonip23 commented Apr 29, 2022

Hello there!
Im interested in contributing to clippy, and this seems like a nice start.
A few questions though:

  1. Is it ok to take over this lint? I'll be progressing quite slowly unfortunately, due to luck of free time..
  2. If there can be anyone available for questions along the way that'd be delightful, please!
  3. Regarding the lint itself - isn't it the case for anything that impls Clone? Although in the case of Arc its even worse than other types, I presume its always readable to extract initialization outside the vec![value;size] macro

@Serial-ATA
Copy link
Contributor

@yonip23 Welcome!

  1. Yep, no rush.
  2. You can ask on Zulip, either I or someone else will try to answer you within a day or so.
  3. Yes, but in this case the behavior could be surprising. This should go in the suspicious category.

@yonip23
Copy link
Contributor

yonip23 commented Apr 30, 2022

Thanks a lot!

@infmagic2047
Copy link

I believe this lint could apply equally to Rc as well.

@yonip23
Copy link
Contributor

yonip23 commented May 4, 2022

I believe this lint could apply equally to Rc as well.

The pr is updated to support Rc as well
Thanks for pointing it out 😃

@davidbeesley
Copy link
Author

Just wanted to hop on and say thanks to @yonip23... pretty amazing to me that someone picked this up and that it's almost done two weeks later.

@yonip23
Copy link
Contributor

yonip23 commented May 5, 2022

@davidbeesley I'm excited myself to contribute to this amazing project! Thanks for the shout-out, appreciate it 🙏

@bors bors closed this as completed in 1a11a49 May 10, 2022
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants