BurntSushi / walkdir

Rust library for walking directories recursively.
The Unlicense
1.21k stars 106 forks source link

Fix bug in `skip_current_dir` #124

Closed LukasKalbertodt closed 4 years ago

LukasKalbertodt commented 4 years ago

Fixes #118

As discussed in the issue, the fix is fairly straight forward. I even managed to create a fairly minimal example in form of a test. It indeed panics before the patch and works fine afterwards. I used self.pop() in skip_current_dir as I think it's correct there. As far as I can tell stack_path has always the same length as stack_list or is empty. So there is no reason to treat them independently.

Regarding the second way the invariant can be destroyed I mentioned here: I tried to trigger this bug, but it's hard. So I think this can only go wrong when Ancestor::new returns an error. This apparently only happens on Windows (which I'm not currently using) and I don't know when exactly this error is triggered. So I will stop now in trying to trigger the bug. So the question is: should I "fix" this what I think is a bug in some situations OR should I just keep it as is?

LukasKalbertodt commented 4 years ago

Appveyor fails due to errors when compiling rand_isaac:

``` Running `rustc --crate-name rand_isaac C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\lib.rs --color never --crate-type lib --emit=dep-info,metadata,link -C debuginfo=2 -C metadata=d6e04fd67a256970 -C extra-filename=-d6e04fd67a256970 --out-dir C:\projects\walkdir\target\debug\deps -L dependency=C:\projects\walkdir\target\debug\deps --extern rand_core=C:\projects\walkdir\target\debug\deps\librand_core-018f0181a978bbdd.rlib --cap-lints allow` error[E0407]: method `seed_from_u64` is not a member of trait `SeedableRng` --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\isaac.rs:126:5 | 126 | / fn seed_from_u64(seed: u64) -> Self { 127 | | IsaacRng(BlockRng::::seed_from_u64(seed)) 128 | | } | |_____^ not a member of trait `SeedableRng` error[E0407]: method `seed_from_u64` is not a member of trait `SeedableRng` --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\isaac.rs:331:5 | 331 | / fn seed_from_u64(seed: u64) -> Self { 332 | | let mut key = [w(0); RAND_SIZE]; 333 | | key[0] = w(seed as u32); 334 | | key[1] = w((seed >> 32) as u32); ... | 341 | | Self::init(key, 1) 342 | | } | |_____^ not a member of trait `SeedableRng` error[E0407]: method `seed_from_u64` is not a member of trait `SeedableRng` --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\isaac64.rs:116:5 | 116 | / fn seed_from_u64(seed: u64) -> Self { 117 | | Isaac64Rng(BlockRng64::::seed_from_u64(seed)) 118 | | } | |_____^ not a member of trait `SeedableRng` error[E0407]: method `seed_from_u64` is not a member of trait `SeedableRng` --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\isaac64.rs:301:5 | 301 | / fn seed_from_u64(seed: u64) -> Self { 302 | | let mut key = [w(0); RAND_SIZE]; 303 | | key[0] = w(seed); 304 | | // Initialize with only one pass. ... | 310 | | Self::init(key, 1) 311 | | } | |_____^ not a member of trait `SeedableRng` error[E0599]: no function or associated item named `seed_from_u64` found for type `rand_core::block::BlockRng` in the current scope --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\isaac.rs:127:41 | 127 | IsaacRng(BlockRng::::seed_from_u64(seed)) | ^^^^^^^^^^^^^ function or associated item not found in `rand_core::block::BlockRng` error[E0599]: no function or associated item named `seed_from_u64` found for type `isaac::IsaacRng` in the current scope --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\isaac.rs:141:15 | 94 | pub struct IsaacRng(BlockRng); | ----------------------------------------- function or associated item `seed_from_u64` not found for this ... 141 | Self::seed_from_u64(seed) | ^^^^^^^^^^^^^ | | | function or associated item not found in `isaac::IsaacRng` | help: there is a method with a similar name: `new_from_u64` error[E0599]: no function or associated item named `seed_from_u64` found for type `rand_core::block::BlockRng64` in the current scope --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\isaac64.rs:117:47 | 117 | Isaac64Rng(BlockRng64::::seed_from_u64(seed)) | ^^^^^^^^^^^^^ function or associated item not found in `rand_core::block::BlockRng64` error[E0599]: no function or associated item named `seed_from_u64` found for type `isaac64::Isaac64Rng` in the current scope --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\isaac64.rs:131:15 | 84 | pub struct Isaac64Rng(BlockRng64); | ----------------------------------------------- function or associated item `seed_from_u64` not found for this ... 131 | Self::seed_from_u64(seed) | ^^^^^^^^^^^^^ | | | function or associated item not found in `isaac64::Isaac64Rng` | help: there is a method with a similar name: `new_from_u64` error[E0599]: no function or associated item named `seed_from_u64` found for type `isaac64::Isaac64Core` in the current scope --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\rand_isaac-0.1.0\src\isaac64.rs:283:15 | 138 | pub struct Isaac64Core { | ---------------------- function or associated item `seed_from_u64` not found for this ... 283 | Self::seed_from_u64(seed) | ^^^^^^^^^^^^^ | | | function or associated item not found in `isaac64::Isaac64Core` | help: there is a method with a similar name: `new_from_u64` error: aborting due to 9 previous errors ```

Notably: AppVeyor tries to compile rand_isaac 0.1.0 while Travis compiles 0.1.1 which got some version requirement fixes. Not sure what exactly is happening here tho. But I'm pretty sure this has nothing to do with this PR.

BurntSushi commented 4 years ago

Yeah, ignore rand for now. I have a branch that rips it out, because I'm so tired of dealing with it. I'll try to cherry pick those commits out, put them on master, and then you can rebase. I'll try to take a look at your PR soon. Thanks so much for submitting it. :-)

jonasbb commented 4 years ago

@LukasKalbertodt Thank you for fixing #118. I tested cargo sweep with your branch of walkdir and it works.

BurntSushi commented 4 years ago

@LukasKalbertodt Thanks so much for the amazing analysis and fix here! I merged this (with a small fixup to your test, since I couldn't get it to fail on current master) in #125, among other things, which also got CI straightened out. I also believed your analysis of the second way this could happen and hopefully fixed that as well, although as you discovered, reproducing that bug is a bit tricky so I didn't bother with a regression test.

Either way, the fix is in walkdir 2.2.9 on crates.io, so cargo sweep should probably update!

LukasKalbertodt commented 4 years ago

@BurntSushi Thank you :)