search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.96k stars 12.53k forks source link

Rustdoc doctests should have their file/lines remapped when using `-Zinstrument-coverage` #79417

Open Swatinem opened 3 years ago

Swatinem commented 3 years ago

I have a workspace with crate_a (and crate_b, but its not used for this example), and use the following env vars set to have my rustdoc tests run through code coverage:

RUSTDOCFLAGS="-Z instrument-coverage -Z unstable-options --persist-doctests=C:\Users\swatinem\AppData\Local\Temp\_rustdoc

Running the result later through llvm-cov gives me errors for my rustdocs, and unexpected results:

> llvm-cov show -format=html -instr-profile=coverage/coverage.profdata -object=C:\Users\swatinem\AppData\Local\Temp\_rustdoc\crate_a_src_somemod_someothermod_mod_rs_1_0\rust_out

error: src\somemod\someothermod\mod.rs: no such file or directory
warning: The file 'src\somemod\someothermod\mod.rs' isn't covered.

When using export -format=lcov, I get the following:

SF:src\somemod\someothermod\mod.rs
FN:3,_RNvCs4fqI2P2rA04_8rust_out4main
FNDA:1,_RNvCs4fqI2P2rA04_8rust_out4main
FNF:1
FNH:1
DA:3,1
DA:4,1
LF:2
LH:2
end_of_record

This is not really a problem per-se, but note that the filename is not relative to my workspace, but to the crate inside the workspace, which creates problems down the road, because uploading that report to codecov actually finds the correct file for some reason, but then highlights lines 3 and 4.

So ideally, the coverage support should similarly re-map line numbers such as compile errors in doctests do, and the filename should be relative to the workspace (but that seems to be a separate issue with rustdoc I guess).

@richkadel I guess you know the code the best: Is such a re-mapping that I propose possible at all? If so, I would be happy to work on this if you point me in the right direction and would be willing to mentor me ;-)

Swatinem commented 3 years ago

Here is some example code: https://github.com/Swatinem/fucov And here are some examples how I try to invoke the tools: https://github.com/Swatinem/fucov/runs/1454817756

richkadel commented 3 years ago

Hi @Swatinem !

Thanks for your patience.

It would be great if you could work on this! (I will probably not be able to work on it any time soon.)

I was able to reproduce this issue as well. Your instructions above are great. Another way to see these results, using cargo commands similar to what's in the updated "Rust Unstable Book" documentation (just for comparison and context) is:

$ rm -rf target/debug/doctestbins/*
$ RUSTFLAGS="-Zinstrument-coverage" RUSTDOCFLAGS="-Zinstrument-coverage -Z unstable-options \
    --persist-doctests target/debug/doctestbins" LLVM_PROFILE_FILE="json5format-%m.profraw" \
    cargo test --doc

# or full coverage status after running both doc and non-doc tests:
# $ RUSTFLAGS="-Zinstrument-coverage" RUSTDOCFLAGS="-Zinstrument-coverage -Z unstable-options \
#     --persist-doctests target/debug/doctestbins" LLVM_PROFILE_FILE="json5format-%m.profraw" \
#     cargo test   # (default: both --tests and --doc)
$ cargo profdata -- merge -sparse json5format-*.profraw -o json5format.profdata
$ cargo cov -- show --color -Xdemangler=rustfilt --instr-profile=json5format.profdata \
    $(for file in target/debug/build/* target/debug/doctestbins/*/*; \
      do [[ ! -d $file &&  -x $file ]] && echo $file; done) \
    --show-line-counts-or-regions --show-instantiations | less -R

When I ran this, I saw (for example) the large test block in json5format/src/lib.rs was apparently executed (3 times, for some reason), but the line numbers were clearly off. Since that test is near the top of the source, the line counters started just a few lines too high, for the reasons you described (I assume).

If you do work on this, I'm happy to try to help and answer questions if you have them, either via this issue (for more strategic issues in particular) or via a Zulip topic.

I don't know how rustdoc does its remapping specifically, but that sounds like a good place to start looking.

The InstrumentCoverage MIR pass uses the Rust compiler's default session SourceMap, via:

tcx.sess.source_map();

to get the file name, line, and column.

One nice thing about that is, I know there are ways to remap the source directly in the SourceMap, so the coverage code doesn't even need to be aware.

I know this works for file names, at least.

If I add the following to RUSTFLAGS and RUSTDOCFLAGS, for example:

--remap-path-prefix=$HOME/.cargo/registry/src/github.com-1ecc6299db9ec823=/tmp/s"

the coverage map embedded in the binary will use the /tmp/s/ file prefix instead of the very long absolute path prefix I get by default for the cargo build dependency crates' sources.

Maybe there's already support for remapping line numbers. If not, that may still be the place to implement a line mapping feature.

Obviously a command line flag is not the right approach for line number mapping. You would need to derive the line number offset from the line position of each test, and you would only apply that offset to the test code, without changing the line numbers of other "real" functions in the source file. So, a bit different, but it should be possible.

Swatinem commented 3 years ago

Thanks for pointing me to SourceMap. Indeed it has a method that re-maps lines specifically for doctests:

https://github.com/rust-lang/rust/blob/a68864b68859e5848865bd392ce6d892b16f5cdd/compiler/rustc_span/src/source_map.rs#L417-L429

Just for reference, this was added here: https://github.com/rust-lang/rust/pull/47274 and later passed via unstable env vars here: https://github.com/rust-lang/rust/pull/63827

I’m playing around with this a bit and it seems like I have found a way to re-map the lines, however there is still other problems related to rustdoc that I would have to fix separately.

I will test a bit more locally and then open a PR :-)

Swatinem commented 3 years ago

https://github.com/rust-lang/cargo/pull/8954 is a change to cargo that should make rustdoc use the correct filenames for workspace crates. I think that will solve the filename problem, although I haven’t tested that directly yet.

richkadel commented 3 years ago

This Issue is partially resolved by #79762. (Thanks @Swatinem!)

There are some known issues still to resolve: