doctests fail with rust 1.26

[johanneskoester]

Input C/C++ Header

    /**
     *  bcf_hdr_set_samples() - for more efficient VCF parsing when only one/few samples are needed
     *  @samples: samples to include or exclude from file or as a comma-separated string.
     *              LIST|FILE   .. select samples in list/file
     *              ^LIST|FILE  .. exclude samples from list/file
     *              -           .. include all samples
     *              NULL        .. exclude all samples
     *  @is_file: @samples is a file (1) or a comma-separated list (0)
     *
     *  The bottleneck of VCF reading is parsing of genotype fields. If the
     *  reader knows in advance that only subset of samples is needed (possibly
     *  no samples at all), the performance of bcf_read() can be significantly
     *  improved by calling bcf_hdr_set_samples after bcf_hdr_read().
     *  The function bcf_read() will subset the VCF/BCF records automatically
     *  with the notable exception when reading records via bcf_itr_next().
     *  In this case, bcf_subset_format() must be called explicitly, because
     *  bcf_readrec() does not see the header.
     *
     *  Returns 0 on success, -1 on error or a positive integer if the list
     *  contains samples not present in the VCF header. In such a case, the
     *  return value is the index of the offending sample.
     */
    int bcf_hdr_set_samples(bcf_hdr_t *hdr, const char *samples, int is_file);

Bindgen Invocation

let bindings = bindgen::Builder::default()
        .header("wrapper.h")
        .blacklist_type("max_align_t")
        .generate()
        .expect("Unable to generate bindings.");
    bindings
        .write_to_file(out.join("bindings.rs"))
        .expect("Could not write bindings.");

Actual Results

The bindings build correctly, but when I run cargo test I get the following error when rust 1.26 tries to build run doctests:

---- target/debug/build/rust-htslib-72a070073c29be22/out/bindings.rs - htslib::bcf_hdr_set_samples (line 7071) stdout ----
	error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `all`
 --> target/debug/build/rust-htslib-72a070073c29be22/out/bindings.rs:7072:12
  |
3 | .. include all samples
  |            ^^^ expected one of 8 possible tokens here

error[E0423]: expected value, found macro `include`
 --> target/debug/build/rust-htslib-72a070073c29be22/out/bindings.rs:7072:4
  |
3 | .. include all samples
  |    ^^^^^^^ did you mean `include!(...)`?

error[E0308]: mismatched types
 --> target/debug/build/rust-htslib-72a070073c29be22/out/bindings.rs:7072:1
  |
2 | fn main() {
  |           - expected `()` because of default return type
3 | .. include all samples
  | ^^^^^^^^^^ expected (), found struct `std::ops::RangeTo`
  |
  = note: expected type `()`
             found type `std::ops::RangeTo<_>`

thread 'rustc' panicked at 'couldn't compile the test', librustdoc/test.rs:306:13
note: Run with `RUST_BACKTRACE=1` for a backtrace.

The generated bindings are

extern "C" {
    /// bcf_hdr_set_samples() - for more efficient VCF parsing when only one/few samples are needed
    /// @samples: samples to include or exclude from file or as a comma-separated string.
    /// LIST|FILE   .. select samples in list/file
    /// ^LIST|FILE  .. exclude samples from list/file
    /// -           .. include all samples
    /// NULL        .. exclude all samples
    /// @is_file: @samples is a file (1) or a comma-separated list (0)
    ///
    /// The bottleneck of VCF reading is parsing of genotype fields. If the
    /// reader knows in advance that only subset of samples is needed (possibly
    /// no samples at all), the performance of bcf_read() can be significantly
    /// improved by calling bcf_hdr_set_samples after bcf_hdr_read().
    /// The function bcf_read() will subset the VCF/BCF records automatically
    /// with the notable exception when reading records via bcf_itr_next().
    /// In this case, bcf_subset_format() must be called explicitly, because
    /// bcf_readrec() does not see the header.
    ///
    /// Returns 0 on success, -1 on error or a positive integer if the list
    /// contains samples not present in the VCF header. In such a case, the
    /// return value is the index of the offending sample.
    pub fn bcf_hdr_set_samples(
        hdr: *mut bcf_hdr_t,
        samples: *const ::std::os::raw::c_char,
        is_file: ::std::os::raw::c_int,
    ) -> ::std::os::raw::c_int;
}

Expected Results

I expect all doctests to pass. In particular, as I understood previous issues (e.g., #426) rust should even skip them in such a case. Is this a regression or an intended change in rust 1.26? It worked fine with rust 1.25.

[emilio]

I guess this should be an issue in the rust repo, unless it's a bindgen regression.

[emilio]

The relevant C comment from #1478 is:

https://github.com/FFmpeg/FFmpeg/blob/10506de9ad1fb050737ef79cf4853742b793c37d/libavcodec/avcodec.h#L3201

I don't think it really makes sense to run doctests on comments that come from FFI headers, but I'm not aware of any way to skip them in a per-module basis.

[Michael-F-Bryan]

I just encountered the same problem when translating this function.

It's pretty easy to see why rustdoc complains about broken tests around line 8109 of the generated code, although it doesn't look like there's an easy way for bindgen to fix the problem.

In my case an easy workaround is to blacklist the function and write its extern manually.

[jamcleod]

So @emilio I think a reasonable solution to this is to use the same detection code as a rustdoc to detect if any untagged (i.e. doesn't manually specify ```rust) codeblocks exist in the generated code, and if the code is not explicitly Rust code, tag it as no_run (or probably more likely tag it as C or C++). If that's an agreeable fix, I'm down to submit a PR. But I think assuming that C/C++ doc examples are written in Rust (the current behavior) is... weird.

Curious to know your thoughts on how to go about fixing this issue though!

[emilio]

Right, I don't see a great solution here that doesn't involve parsing the doc comments, which is kinda nasty. I think ideally adding a rustdoc attribute to ignore this would be better, but see rust-lang/rust#38825

[pvdrz]

now that #2347 landed. This can be solved by implementing ParseCallbacks::process_comments to either wrap the comment in backticks:

#[derive(Debug)]
struct Cb;

impl ParseCallbacks for Cb {
    fn process_comment(&self, comment: &str) -> Option<String> {
        Some(format!("````\n{}\n````", comment))
    }
}

or parsing it with a javadoc parser (not sure if such thing exists in Rust)

[daixiang0]

@pvdrz I use bindgen v0.63.0:

#[derive(Debug)]
struct CargoCallbacks;

impl bindgen::callbacks::ParseCallbacks for CargoCallbacks{
    fn process_comment(&self, comment: &str) -> Option<String> {
        Some(format!("````\n{}\n````", comment))
    }
}

fn main() {
...

    let bindings = bindgen::Builder::default()
        // The input header we would like to generate
        // bindings for.
        .header("wrapper.h")
        // Given include path of headers.
        .clang_arg(
            inc_path
                .iter()
                .map(|i| format!("-I{}", i.to_string_lossy()).to_string())
                .collect::<String>(),
        )
        // Tell cargo to invalidate the built crate whenever any of the
        // included header files changed.
        .parse_callbacks(Box::new(bindgen::CargoCallbacks))
        // Finish the builder and generate the bindings.
        .generate()
        // Unwrap the Result and panic on failure.
        .expect("Unable to generate bindings");

    // Write the bindings to the $OUT_DIR/bindings.rs file.
    let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
    bindings
        .write_to_file(out_path.join("bindings.rs"))
        .expect("Couldn't write bindings!");
}

The doctest still fails, seems process_comment do not run.

[pvdrz]

you're using bindgen::CargoCallbacks and not the CargoCallbacks you defined.

[daixiang0]

Thanks, it works but doctest still fail.

Original comment format:

/*!
 * 
 * xxxx
 */

"\n{}\n" can not make doctest happy, is there another way to format? I can not find "correct" format for bindgen.

[pvdrz]

I believe "````ignore\n{}\n````" should work

[daixiang0]

After double check, there are 2 types: /*!, /**, both failed:

C:

    /** Reserved */
    uint8_t rsvd2;
   

Rust:

    #[doc = "````\n Reserved\n````"]
    pub rsvd2: u8,

Error:

error[E0425]: cannot find value `Reserved` in this scope
 --> target/debug/build/lib-rs-4256f609e1545549/out/bindings.rs:478:1
  |
3 | Reserved
  | ^^^^^^^^ not found in this scope

[pvdrz]

The ignore is important. What's happening is that rustdoc believes that this:

Reserved

Is a rust code snippet. adding the ignore tells rustdoc to skip it.

[daixiang0]

Thanks a lot, it makes all doctests ignored.

BTW, for now, is there any plan to make doctests pass rather than ignored?

[daixiang0]

Also, even parse_comment is in release 0.62, but bindgen 0.62 does not include this feature. After bump up to 0.63. it is included.

[pvdrz]

BTW, for now, is there any plan to make doctests pass rather than ignored?

Well there's not much you can do there other than using a javadoc parser and then turn that into valid markdown. You could try and come up with your own ad-hoc rules to try and fix your current comments by escaping certain characters like - and wrapping them in back-ticks so markdown doesn't mistake them for items in a list

[pvdrz]

Also, even parse_comment is in release 0.62, but bindgen 0.62 does not include this feature. After bump up to 0.63. it is included.

Whoops... Maybe this was a merge issue and the feature ended up in the wrong version

[daixiang0]

Also, even parse_comment is in release 0.62, but bindgen 0.62 does not include this feature. After bump up to 0.63. it is included.

Whoops... Maybe this was a merge issue and the feature ended up in the wrong version

It would be better to update the package to keep sync with the CHANGELOG.