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

Document #![register_tool] #138586

Merged
merged 1 commit into from
Mar 17, 2025
Merged

Document #![register_tool] #138586

merged 1 commit into from
Mar 17, 2025

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 17, 2025

cc #66079

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2025

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 17, 2025

`register_tool` also allows configuring lint levels for external tools.

Tool attributes are only meant for ignorable attributes. If your code *changes* meaning when the attribute is present, it should not use a tool attribute (because it cannot be compiled with anything other than the external tool, and in a sense is a fork of the language).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this bit really belongs in an RFC, not here. i will try to make an RFC in the new few months.

Comment on lines 32 to 36
#![feature(register_tool)]
#![register_tool(c2rust)]

#[c2rust::header_src = "/home/user/some/workspace/foobar/bar.h:5"]
pub mod bar_h {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kkysen are you ok with me mentioning c2rust in the official docs here?

Copy link
Member

@jieyouxu jieyouxu Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also just use some kind of stub tool name 🤷

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I think so. @thedataking, this is fine, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also just use some kind of stub tool name 🤷

i think giving an example that shows how the attribute is useful is better than just shoving in foo/bar everywhere. motivating examples, and all that.

(of course it doesn't need to be c2rust specifically; i'm happy to find some other external tool if Per doesn't want it mentioned here.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, I think it'd be very cool to mention c2rust here.

Copy link

@thedataking thedataking Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I think so. @thedataking, this is fine, right?

Definitely fine. Please go ahead @jyn514.

Comment on lines +45 to +54
#![feature(register_tool)]
#![register_tool(bevy)]
#![deny(bevy::duplicate_bevy_dependencies)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've already asked @alice-i-cecile about this example, she said mentioning bevy is fine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this lint / tool doesn't exist yet, but I think it's a very clear example of where this might be useful.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this lint / tool doesn't exist yet, but I think it's a very clear example of where this might be useful.

This specific lint comes in the 0.2.0 release, but yea it's not an official bevy tool yet.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good. One tiny nit but r=me with or without.

@jyn514 jyn514 force-pushed the doc-register-tool branch 3 times, most recently from ae1fe37 to e390be7 Compare March 17, 2025 02:48
Copy link

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff; the c2rust section in particular is much clearer to me now.

@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 force-pushed the doc-register-tool branch from e390be7 to 10bc5ac Compare March 17, 2025 05:16
@jieyouxu
Copy link
Member

Thanks!
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 17, 2025

📌 Commit 10bc5ac has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 17, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2025
Rollup of 9 pull requests

Successful merges:

 - rust-lang#136355 (Add `*_value` methods to proc_macro lib)
 - rust-lang#137621 (Add std support to cygwin target)
 - rust-lang#137793 (Stablize anonymous pipe)
 - rust-lang#138341 (std: Mention clone-on-write mutation in Arc<T>)
 - rust-lang#138517 (Improve upvar analysis for deref of child capture)
 - rust-lang#138584 (Update Rust Foundation links in Readme)
 - rust-lang#138586 (Document `#![register_tool]`)
 - rust-lang#138590 (Flatten and simplify some control flow 🫓)
 - rust-lang#138592 (update change entry for rust-lang#137147)

r? `@ghost`
`@rustbot` modify labels: rollup
Copy link

@DaAlbrecht DaAlbrecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the mention of the bevy lint tool!

Comment on lines +45 to +54
#![feature(register_tool)]
#![register_tool(bevy)]
#![deny(bevy::duplicate_bevy_dependencies)]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this lint / tool doesn't exist yet, but I think it's a very clear example of where this might be useful.

This specific lint comes in the 0.2.0 release, but yea it's not an official bevy tool yet.

@bors bors merged commit 47d0c6b into rust-lang:master Mar 17, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 17, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2025
Rollup merge of rust-lang#138586 - jyn514:doc-register-tool, r=jieyouxu

Document `#![register_tool]`

cc rust-lang#66079
@jieyouxu jieyouxu assigned jieyouxu and unassigned ehuss Mar 17, 2025
@BD103
Copy link
Contributor

BD103 commented Mar 17, 2025

Thanks for doing this, this is really cool! If you want any help writing the RFC, I'd be glad to assist, although it would be my first time. :)

@jyn514 jyn514 deleted the doc-register-tool branch March 17, 2025 21:39
@jhpratt
Copy link
Member

jhpratt commented Mar 18, 2025

still in the queue after being merged

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 18, 2025
@jyn514
Copy link
Member Author

jyn514 commented Mar 18, 2025

@jhpratt this pr has not been merged ??

@jhpratt
Copy link
Member

jhpratt commented Mar 18, 2025

@jyn514 It was merged as part of #138595. It is present on master and is indicated as merged by GitHub in multiple places.

I noticed it when I created #138636 and saw that a number of PRs had already been merged.

@jieyouxu
Copy link
Member

Yeah, bors sometimes has its own ideas...

@jyn514
Copy link
Member Author

jyn514 commented Mar 18, 2025

oh lmao the github app was showing me a cached version of the pr, incredible

@jhpratt
Copy link
Member

jhpratt commented Mar 18, 2025

🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.