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 rust_analyzer as a predefined tool #125241

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented May 18, 2024

Given all the other rust-lang tools have it, I'd expect r-a to have it too. (we have a few ideas for using this rust-lang/rust-analyzer#11556).

@rustbot
Copy link
Collaborator

rustbot commented May 18, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
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 May 18, 2024
@ehuss
Copy link
Contributor

ehuss commented May 18, 2024

If you add this, can you please update the recognized tool attributes in https://github.com/rust-lang/reference/blob/e356977fceaa8591c762312d8d446769166d4b3e/src/attributes.md#L199?

@BoxyUwU
Copy link
Member

BoxyUwU commented May 18, 2024

r? @ehuss

@Veykril
Copy link
Member Author

Veykril commented May 19, 2024

rust-lang/reference#1498

@ehuss
Copy link
Contributor

ehuss commented May 20, 2024

I am not the correct person to review this.

r? compiler

Also, I would expect this to go through the MCP process. This is adding a new attribute which can be used on stable.

I think there is still an open design question around how tools are registered, and I think caution should be used when adding more built-in attributes. I think there should be some mechanism for being able to support tool attributes that doesn't bake them into the language, or require different compiler implementations to also hard-code this list. My concern is that if I handed this code to some other compiler:

#[rustfmt::skip]
fn f() {}

should all implementations know what rustfmt is? What about the other tools listed here? If so, then that bakes that into the language, or else makes code non-portable (assuming an error is generated for an unknown attribute).

#66079 has implemented #![register_tool(my_tool)] for this purpose, but then do we expect anyone that uses clippy or rustfmt (or any of these other built-in-tools) to have to specify something like #![register_tool(clippy)]?

I'm not quite seeing how tools could be registered, while keeping code portable across implementations. One option would be to keep all tools in a tool namespace (like #[tool::rust_analyzer::foo]), and then ignore everything under tool (similar to the diagnostic namespace).

cc @petrochenkov FYI, not sure if you have an opinion here.

@rustbot rustbot assigned davidtwco and unassigned ehuss May 20, 2024
@petrochenkov
Copy link
Contributor

From what I remember the existing tools were hardcoded because #![register_tool] wasn't yet stable, after it's stable all tools were supposed to be registered explicitly.
(I also missed the moment miri was added to the list.)

Is there any specific motivation for adding rust_analyzer to the list now?

@Veykril
Copy link
Member Author

Veykril commented May 20, 2024

(I also missed the moment miri was added to the list.)

I saw the miri one being added without second thoughts and figured rust project tools would be just fine.

after it's stable all tools were supposed to be registered explicitly.

How would that even work? Removing them would break things so that doesn't seem like an option?

Is there any specific motivation for adding rust_analyzer to the list now?

rust-lang/rust-analyzer#11556 has a bunch of ideas listed, but if you ask right now there isn't a concrete plan yet. This would make experimenting for these feature for r-a nicer though. I'm fine with this being closed though for now, if we don't want to add every rust-project tool here (though as I read the register_tool issue it does feel like its ages away from ever being stabilized, and as eric pointed out, the feature is questionable designed in the first place)

@davidtwco
Copy link
Member

davidtwco commented May 28, 2024

I think this makes sense to do. Tools have been added to this list in the past without any broader approvals other than the review (most recently, miri in #124293), but I think we probably should do an MCP for this (and tools going forward). After that, r=me.

@petrochenkov
Copy link
Contributor

How would that even work? Removing them would break things so that doesn't seem like an option?

They can be removed on an edition bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants