Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add option to replace proc macro #[doc = "..."] with the more usable /// ... #2348

Closed
tgross35 opened this issue Nov 14, 2022 · 8 comments · Fixed by #2375
Closed

Add option to replace proc macro #[doc = "..."] with the more usable /// ... #2348

tgross35 opened this issue Nov 14, 2022 · 8 comments · Fixed by #2375

Comments

@tgross35
Copy link
Contributor

Currently, bindgen generates doc comments using the proc macro style like the following:

#[doc = "This is a doc comment."]
struct thing{
    #[doc="Another doc."]
    a: i32
}

This is very OK for build.rs-generated output that the user may never directly see. However, it is a bit unwieldy when you need to interact with the output - difficult to read and very cumbersome to edit.

It was pointed out in this comment that the main reason for this is a limitation of the quote crate, which makes sense. But I was wondering if a --replace-doc-macros flag would be considered that postprocesses the output text.

My first pass attempt at a regex sequence to do this is:

^(?P<lead_ws>[\t\v\f ]*)#\[\s*doc\s*=\s*"(?P<content>.*)"\s*\]\s*$

(playground)

I usually run something like this after output generation when I need to cache the file, but I think that quite a few users would be happy to have this as a builtin option.

@tgross35
Copy link
Contributor Author

I think that this could just happen during write here

https://github.com/rust-lang/rust-bindgen/blob/678daf56a2384252304fc27ed2b46e769da1890f/bindgen/lib.rs#L2731C33-L2734

Add a check for the CLI option and run the regex replacer if so. rustfmt_bindings is a Cow which is also the result of regex::replace_all() so that works out nicely.

@tgross35
Copy link
Contributor Author

Proposed changes:

  • Add --replace-doc-macros CLI option
  • Add .clean_doc_macros builder option
  • Implement it by doing a regex replace within write
  • Add tests to validate the above regex

If this sounds good to admins, I will get a PR going

@pvdrz
Copy link
Contributor

pvdrz commented Dec 12, 2022

Hi! Thanks for your submission.

Even though I agree that /// is a nicer way to have comments I'm not sure if bindgen itself should handle this as it is fairly straightforward to do it by writing the bindings to a String and then using the regular expression you just showed.

@tgross35
Copy link
Contributor Author

Hey, thanks for the feedback. I don't disagree that it's not a super complicated step, but the regex isn't totally trivial (could be documented somewhere ofc), and it would be much nicer for CLI users to be able to bindgen x.c --replace-doc-macros > x.rs instead of needing to sed/perl/awk the output. And I think it is at least a somewhat desired feature.

Is your disposition more neutral or more against this feature? If you're neutral, I'll do the PR and you can review then. If you're against it, I suppose we could close this issue

@divagant-martian
Copy link

While the change might be doable it's still a component/feature to maintain that it's slightly out of scope since the generated code is valid. I was looking into some way to change the way a token stream is displayed to get the /// style instead.

While doing this I found https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#normalize_doc_attributes a rustfmt configuration option to write doc attributes in the more readable way. However this feature is not yet stabilized.

What would you say about using rust-fmt to get this instead. This way, the burden of the correct writing of a token stream falls on rustfmt, but until stabilization it would need nightly

@pvdrz
Copy link
Contributor

pvdrz commented Dec 15, 2022

That's awesome! We could certainly add a section in the user guide mentioning how to use rustfmt for this. Bindgen uses rustfmt internally too so we might be able to do it from there if the user is using nightly as the rust target.

Edit: focus on the "might" because I'm not sure if we can actually accomplish that

@pvdrz
Copy link
Contributor

pvdrz commented Dec 15, 2022

After tinkering a bit with both bindgen and rustfmt this can be achieved by adding a rustfmt.toml file to the directory from where bindgen will be run with the following contents:

normalize_doc_attributes = true

bindgen will use that configuration file by default because that's what rustfmt already does. If you want to use a filename different from rustfmt.toml you can use whatever you want and then use either the --rustfmt-configuration-file flag or the Builder::rustfmt_configuration_file method with the path to the configuration file as an argument.

Given that the normalize_doc_attributes option is unstable you need to have your environment configured in such a way that the rustfmt command corresponds to the nightly version. If you already do that somehow then you don't need to do anything else.

However, if you are using any stable release of rustfmt you need to do some extra steps and here's where the behavior differs depending on how you're using bindgen. So let's say we have a header file hello.h as an input for bindgen. Then:

When using bindgen as a CLI

If you have rustup you can run:

rustup run nightly bindgen hello.h

and that will set up the environment for you

When using bindgen as a library

If you have rustup you can use the output of the command:

$ rustup which rustfmt --toolchain=nightly

and pass it to Builder::with_rustfmt:

use bindgen::Builder;
use std::process::Command;

fn main() {
    let output = Command::new("rustup")
        .args(["which", "rustfmt", "--toolchain", "nightly"])
        .output()
        .unwrap();

    assert!(output.status.success(), "{:?}", output);

    let rustfmt_path = String::from_utf8(output.stdout).unwrap();

    let bindings = Builder::default()
        .header("./hello.h")
        .with_rustfmt(rustfmt_path)
        .generate()
        .unwrap();

    bindings.write_to_file("path/to/output.rs").unwrap();
}

@tgross35 do you think documenting this in the user guide would be a proper solution for this issue?

@tgross35
Copy link
Contributor Author

That’s a great find! And a docs update definitely works for me. I didn’t realize rustfmt had that option, that’s an awesome time saver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants