Follow `path` attribute on inline `mod` blocks

danjl1100 commented 5 months ago

Following discussion in #335,

Progress

[x] Add tests based on the reference
[x] Add logic in visit.rs to follow path attributes #[path="..."] on modules
[x] Verify paths do not contain .. or start with / (not a security boundary, but nice to have)
[x] Report file/line/col of the mod statement on error

Future tasks

This is a list of some enhancements that are not planned to be included in this PR:

Add the command-line option to fail on "referent of mod not found".
- I did verify tests pass with that warn! substituted with a panic! (which means the Ok(None) return path may not be currently exercised by the test suite... it's fun thinking about how cargo-mutants can help improve its own test suite :slightly_smiling_face: )
Address the TODO comment to refactor special handling of mod.rs to SourceFile itself (though the new tests should help with verifying that effort) https://github.com/sourcefrog/cargo-mutants/blob/961f1a3bc03e9d29e564f6c6b70f002c34e8437f/src/visit.rs#L469-L471

danjl1100 commented 5 months ago

I started drafting the error for invalid filesystem traversal (https://github.com/sourcefrog/cargo-mutants/pull/339/commits/6c2620a74e3084b660054bd59216060eea48ad09). It seems a bit clunky to have a "did it fail yet" bool; not sure how best to break out of the Visitor without having a flag or similar.

Do you think I should add a testdata case for this? (named something like dubious_paths) So far, I've tested it manually on a file like this. Thinking if this is added as a test-case, I'll keep the paths harmless (replaced my test /etc/passwd with /root/file, just in case!)

#[path = "/root/file"]
mod slash;

#[path = "foo/../../../../root/file"]
mod dots;

#[path = "/../../../../root/file"]
mod slash_dots;

I'm not sure how much it's used, but it seems like a valid usecase to have relative paths (as long as there's no upward .. traversal) Maybe I should add relative paths to nested_mod as well

#[path="subfolder/foo.rs"]
mod this_is_legal;

-- Up next I'm also planning to work on the file/line/col reporting for the current "referent of mod not found" warning.

sourcefrog commented 5 months ago

Nice, thanks!

Following discussion in #335,

Progress

[x] Add tests based on the reference

[x] Add logic in visit.rs to follow path attributes #[path="..."] on modules

[ ] Verify paths do not contain .. or start with / (not a security boundary, but nice to have)

[ ] Report file/line/col of the mod statement on error

Future tasks

This is a list of some enhancements that are not planned to be included in this PR:

Add the command-line option to fail on "referent of mod not found".

I wonder if this should just be a simple error without an option to make it a warning: it should only be hit if the tree is invalid or if there's a mutants bug causing it to read the tree differently to how Cargo will. (Such as #335 for example.)

On the other hand when we do have those bugs perhaps it's nice for people still to be able to get partial results...

I did verify tests pass with that warn! substituted with a panic! (which means the Ok(None) return path may not be currently exercised by the test suite... it's fun thinking about how cargo-mutants can help improve its own test suite 🙂 )

Yes, that is interesting!

We could add a test tree that has a dangling mod statement and expect that it fails. But why is it not caught?

~~I'm a bit surprised that this is not already caught by mutating Ok(None) to Err(anyhow!(...)).~~

The case you're talking about is: currently it's implemented, and intended, that if the mod is not found, we emit a warning and return Ok, ignoring that module. But, apparently not test asserts that this is what should happen.

I guess line coverage measurements would show that the warn/return Ok is never hit.

We do try mutating the entire function to return Err(_), Ok(None), etc. What we do not yet try, but could, is changing just that one value at the end of the function to be e.g. Err(_). I guess that would be missed today. In other words: mutate a function so that it has all its usual side effects or has the option to take any other early return paths, but return a different value at the end.

Address the TODO comment to refactor special handling of mod.rs to SourceFile itself (though the new tests should help with verifying that effort) https://github.com/sourcefrog/cargo-mutants/blob/961f1a3bc03e9d29e564f6c6b70f002c34e8437f/src/visit.rs#L469-L471

danjl1100 commented 5 months ago

Thanks for the review! I updated from the discussion.

added testdata/dangling_mod to verify the visitor attempts to gracefully continue on a missing module. This seems to fix verifying the Ok(None) return value.
added line/column to the warning when a module file is not found (renamed displayed variable name from parent_path to definition_site), with an integration test to capture the output formatting

sourcefrog commented 5 months ago

Thanks, I'll look on the weekend if not before.

sourcefrog commented 5 months ago

Looks good, I really appreciate your work on this! I will make a release with it shortly.

danjl1100 commented 5 months ago

Thank you for helping me along the way! 🎉

sourcefrog commented 5 months ago

~~I'll fix the test flakes and then merge this~~

Ah, they were post-merge failures, but I'll deal with them separately.

sourcefrog / cargo-mutants