Skip to content

same_item_push is incorrect in several cases #5985

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Ploppz opened this issue Aug 29, 2020 · 3 comments · Fixed by #6016
Closed

same_item_push is incorrect in several cases #5985

Ploppz opened this issue Aug 29, 2020 · 3 comments · Fixed by #6016
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience.

Comments

@Ploppz
Copy link

Ploppz commented Aug 29, 2020

Code: the pdf crate on this commit https://github.com/Ploppz/pdf/tree/535429d8543434fbd5f46b4589c013bf96be9d10/pdf .
cargo clippy says:

warning: it looks like the same item is being pushed into this Vec
   --> pdf/src/object/stream.rs:247:17
    |
247 |                 offsets.push(offset);
    |                 ^^^^^^^
    |
    = note: `#[warn(clippy::same_item_push)]` on by default
    = help: try using vec![offset;SIZE] or offsets.resize(NEW_SIZE, offset)
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push

warning: it looks like the same item is being pushed into this Vec
  --> pdf/src/parser/parse_xref.rs:30:9
   |
30 |         entries.push(entry);
   |         ^^^^^^^
   |
   = help: try using vec![entry;SIZE] or entries.resize(NEW_SIZE, entry)
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push

Which seems incorrect.

Pasting the code in question here:

        let mut offsets = Vec::new();
        {
            debug!("parsing stream");
            let mut lexer = Lexer::new(stream.data()?);
            for _ in 0..(stream.info.num_objects as ObjNr) {
                let _obj_nr = lexer.next()?.to::<ObjNr>()?;
                let offset = lexer.next()?.to::<usize>()?;
                offsets.push(offset);
            }
        }
    let mut entries = Vec::new();
    for _ in 0..num_entries {
        let _type = read_u64_from_stream(width[0], data);
        let field1 = read_u64_from_stream(width[1], data);
        let field2 = read_u64_from_stream(width[2], data);

        let entry =
        match _type {
            0 => XRef::Free {next_obj_nr: field1 as ObjNr, gen_nr: field2 as GenNr},
            1 => XRef::Raw {pos: field1 as usize, gen_nr: field2 as GenNr},
            2 => XRef::Stream {stream_id: field1 as ObjNr, index: field2 as usize},
            _ => return Err(PdfError::XRefStreamType {found: _type}), // TODO: Should actually just be seen as a reference to the null object
        };
        entries.push(entry);
    }

Meta

  • cargo clippy -V: clippy 0.0.212 (de521cb 2020-08-21)
  • rustc -Vv:
rustc 1.47.0-nightly (de521cbb3 2020-08-21)
binary: rustc
commit-hash: de521cbb303c08febd9fa3755caccd4f3e491ea3
commit-date: 2020-08-21
host: x86_64-unknown-linux-gnu
release: 1.47.0-nightly
LLVM version: 10.0
@Ploppz Ploppz added the C-bug Category: Clippy is not doing the correct thing label Aug 29, 2020
@tmpolaczyk
Copy link

Can confirm, self-contained version of the first example: Playground

@flip1995 flip1995 added the E-medium Call for participation: Medium difficulty level problem and requires some initial experience. label Aug 31, 2020
@giraffate
Copy link
Contributor

Hmm, it might be better not to emit a lint when the pushed item is the variable that introduced in loop, but what about? Of cource, while I think the above FP can be avoided FN will occur in the following cases.

let mut vec3: Vec<u8> = Vec::new();
for _ in 0..15 {
let item = 2;
vec3.push(item);
}

@chrisduerr
Copy link
Contributor

Maybe an even simpler example using const fns from STD.

fn main() {
    let start = std::time::Instant::now();
    let mut vec = Vec::new();
    for _ in 0..1000 {
        let elapsed = std::time::Instant::now() - start;
        vec.push(elapsed.as_millis());
    }
}

@bors bors closed this as completed in 8b54f1e Sep 8, 2020
@bors bors closed this as completed in #6016 Sep 8, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 1, 2020
[beta][clippy] backport multiple FP fixes for a warn-by-default lint

This backports the PR rust-lang/rust-clippy#6016 fixing multiple FPs:

rust-lang/rust-clippy#5902
rust-lang/rust-clippy#5979
rust-lang/rust-clippy#5985

We didn't have any complaints about this lint, since me merged this PR.

cc `@ebroto` (sorry I forgot about this, since we talked about the backport 3 weeks ago 😐)

r? `@pietroalbini`
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants