Tracking issue for #![register_tool]

[petrochenkov]

A part of #44690.

Some tools (rustfmt and clippy) used in tool attributes are hardcoded in the compiler.
We need some way to introduce them without hardcoding as well.

#66070 introduced a way to do it with a crate level attribute:

#![register_tool(my_tool)]

#[my_tool::anything] // OK
fn main() {}

The previous attempt to introduce them through command line (#57921) met some resistance.

This probably needs to go through an RFC before stabilization.

[SimonSapin]

Is the tool’s name an arbitrary identifier, that doesn’t necessarily map for example to any crate name?

[petrochenkov]

@SimonSapin
Yes, the tool name is arbitrary.

[jyn514]

This appears to only work for attributes, not lints.

#![feature(register_tool)]
#![register_tool(xyz)]
#![warn(xyz::my_lint)]

$ rustc unknown-lint.rs  --crate-type lib
error[E0710]: an unknown tool name found in scoped lint: `xyz::my_lint`
 --> unknown-lint.rs:3:9
  |
3 | #![warn(xyz::my_lint)]
  |         ^^^

[petrochenkov]

AFAIK, registering a lint tool is useless without also registering actual lints, which are registered with rustc_interface methods (together with their tools), so register_tool may only affect diagnostic wording in this case.

[jyn514]

AFAIK, registering a lint tool is useless without also registering actual lints, which are registered with rustc_interface methods (together with their tools), so register_tool may only affect diagnostic wording in this case.

Then there is no way at all to add tool lints, because rustc_lint will give an error if it doesn't recognize the tool, even if there is a lint registered:

rust/compiler/rustc_lint/src/levels.rs

Lines 307 to 317 in edeee91

	if !attr::is_known_lint_tool(tool_ident) {
	struct_span_err!(
	sess,
	tool_ident.span,
	E0710,
	"an unknown tool name found in scoped lint: `{}`",
	pprust::path_to_string(&meta_item.path),
	)
	.emit();
	continue;
	}

rust/compiler/rustc_ast/src/attr/mod.rs

Lines 36 to 38 in edeee91

	pub fn is_known_lint_tool(m_item: Ident) -> bool {
	[sym::clippy, sym::rustc].contains(&m_item.name)
	}

[SimonSapin]

For what it’s worth this is how Servo uses this feature:

https://github.com/servo/servo/blob/92d6c89a8f3ac3ace5dbf65871dfb7c188ce3f95/components/script_plugins/lib.rs
https://github.com/servo/servo/blob/92d6c89a8f3ac3ace5dbf65871dfb7c188ce3f95/components/script/lib.rs#L10-L17

[jyn514]

@SimonSapin that looks like it only uses a tool attribute and not a lint. unrooted_must_root isn't a 'tool' lint in the sense of this issue because it doesn't use a tool prefix (servo::unrooted_musl_root or something).

[joshtriplett]

To clarify, we don't necessarily need to prohibit name conflicts; rather, we need to document what happens in that case. If what should happen is "silently shadow", we should document that.

[bjorn3]

What about making #![register_tool(...)] "define" a module containing attribute macro definitions under every possible name and making #![register_attr(...)] "define" an attribute macro as far as name resolution is concerned. And then make it possible to publicly re-export said fake attribute macros. This way you could use #![register_tool(xyz)] and then #[xyz::foo] as consumer in case there is no crate xyz and use #![register_attr(foo)] pub use foo; in xyz in case such a crate exists and is used by the consumer that wants to use #[xyz::foo].

[Mark-Simulacrum]

I feel like this merits an RFC -- I'm feeling a bit confused about the overall intent for how this feature is to be used and anticipated workflows.

I agree with this -- it's not obvious to me that we want these in attributes, which are harder to e.g. manage across an entire workspace. I think Manish is right in #57921 (comment) that -Z attributes can be harder to pass, particularly on a workspace basis today, but I'm not sure that's actually the right motivation for not doing things via the CLI, it just means that we need accompanying support in Cargo (or other build systems). That obviously requires its own design work.

I think that a design document laying out the details for how both end-users consuming tools and tool authors should think about these attributes would be useful in moving forward here.

[chorman0773]

I would definately like the attribute and lint controls separate.

For https://github.com/LightningCreations/lccc, I want the ability to support additional lints via its own namespace, however, the namespace is also used for attributes. Unlike, e.g. #[rustfmt::skip] these attributes aren't merely informative to the tool, but have actual semantics, and on lccc itself, they would be gated behind a feature, and should not be considered portable at all. In contrast, having additional lint controls is comparatively harmless. #![allow(lccc::abi_incompatibility)] should have no problems being just treated as a no-op by rustc, but #[lccc::mangle_as = "std"] would not be a good thing to have just silently accepted, as the person writing code using it may except that it has a semantic meaning where accepted - that is, it either affects the external name of the thing it's attached to, or it fails to compile (which indeed it does, it fails to compile outside of lccc or when the feature isn't enabled, or it affects the external name as prescribed by the ABI, such that the relevent name component(s) is set to std).

[xFrednet]

From the previous discussion, it sounds like the main concern with stabilizing this feature is the question of how attributes should be handled and what implications this will have for the future.

For external linters, the main concern is the ability to use lint names in lint attributes, like this:

#[allow(marker::super_helpful_lint)]
fn unknown_linter() {}

Which currently triggers E0710. (See Playground)

Would it maybe be possible to reduce the error to a lint trigger? That would unblock external linters, while leaving all options open when it comes to attributes and other related tool discussions. It would also be inline with the unknown_lints lint, which is the way rustc currently handles unknown lints, without a tool prefix.

[xFrednet]

Hey lang team, could you maybe discuss my suggestion to reduce the E0710 error to a lint trigger? (See comment above)

@rustbot label +I-lang-nominated

[joshtriplett]

We talked about this in today's @rust-lang/lang meeting, and we still hold the position that this needs an RFC before stabilizing, to answer several of the questions here.

[joshtriplett]

Also, the questions in #66079 (comment) seem like they should still apply.

[joshtriplett]

@xFrednet, I would have concerns about the approach of downgrading that from a hard error to a lint. It'd be OK to explore that in the "alternatives' section of the aforementioned RFC, though.

[pnkfelix]

@xFrednet also, in case its not clear: the act of downgrading the existing hard error to a lint itself a stabilization action (since it would be part of the new stabilized surface of the language). Thus, the T-lang design team views such a path as best described in the context of an overall plan for stabilizing this feature as a whole.

[xFrednet]

Alright, thank you for the feedback! In the coming months, I sadly won't have time to work on an RFC. If anyone else is interested, I'm happy to share my use case with Marker and also help with the RFC.