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

Fn params are commented out with doc comments. #4936

Closed
HyeonuPark opened this issue Aug 2, 2021 · 7 comments · Fixed by #4985
Closed

Fn params are commented out with doc comments. #4936

HyeonuPark opened this issue Aug 2, 2021 · 7 comments · Fixed by #4985
Assignees

Comments

@HyeonuPark
Copy link

HyeonuPark commented Aug 2, 2021

$ git rev-parse HEAD
3d8cd57c2f166cb92204e95a5f4c73ab20fed4f2

$ cat example.rs
#[discard_params_doc]
trait Trait {
    fn foo(
        &self,
        /// some docs
        bar: String,
        /// another docs
        baz: i32,
    );
}

$ cargo run --bin rustfmt -- --edition=2018 < example.rs 2> /dev/null
#[discard_params_doc]
trait Trait {
    fn foo(&self, /// some docs bar: String, /// another docs baz: i32
    );
}

Normally doc comments on fn params triggers compile error. But it's ok if attribute macro discards it anyway... unless I run rustfmt on it.

@HyeonuPark HyeonuPark changed the title Comments out fn params with doc comment. Fn params are commented out with doc comments. Aug 2, 2021
@calebcartwright
Copy link
Member

Could you please do me a favor and update the issue description with a clean snippet that can be directly reproduced by copying the contents as-is? I appreciate you trying to structure it the way you did, but it actually makes it more difficult to work with.

@HyeonuPark
Copy link
Author

I've edited the Issue! Hope it's better for you to handle it. Otherwise please let me know to update it.

@calebcartwright
Copy link
Member

Yeah that helps thank you! I don't think it would be too hard for us to work around this but I want to try to verify it's a truly valid scenario. Do you have an example somewhere using such a macro that can be compiled?

@calebcartwright calebcartwright added the needs-mcve needs a Minimal Complete and Verifiable Example label Sep 8, 2021
@HyeonuPark
Copy link
Author

Right, I should attach some real usages of such patterns from the start. Sadly there's no such macro in public I'm aware of. But let me describe my own use case and why do I need it.

Recently I'm making some macro which takes a trait def and generates HTTP server, client code and OpenAPI spec from it. The project is pretty much in its initial stage without much features implemented but that's the plan. The OpenAPI allows to attach markdown descriptions to its API parameters, and I thought it would be natural to use doc comment on each method parameters for it. Quick test on my local was successful, but my editor runs auto-formatting and I found this issue.

If you prefer to have working such macro for testing or for fun, I can share one within this week. It's just a matter of stripping down my local test code.

@calebcartwright
Copy link
Member

Fair enough, thanks for the info! It seems like a pretty extreme edge case at the moment but since it's accepted by the compiler's parsing stage (of which the resultant AST is what rustfmt sees/works with) then it's probably fair for us to handle this anyway.

@TonalidadeHidrica
Copy link

@calebcartwright Hi, I still saw this bug in rustfmt 1.4.37-stable (f1edd042 2021-11-29). Is this intended?

@calebcartwright
Copy link
Member

@calebcartwright Hi, I still saw this bug in rustfmt 1.4.37-stable (f1edd042 2021-11-29). Is this intended?

The closing of issues/merges of PRs in tool repos is not directly coupled to Rust releases. You have to wait a good bit for changes to actually make it into the nightly releases (where you can already observe the fix) and longer still for tool changes to land on stable

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