Unnecessary `crate::` prefixes in `crates/ruff_python_ast/generated.rs`

[chirizxc]

there's also a question about ruff_python_ast/src/generated.rs file

rustrover writes that crate:: is not needed in 834 places, should the crates/ruff_python_ast/generate.py itself be corrected?

This is interesting, I guess clippy doesn't warn about this. We could probably open a separate issue or PR for this, if you wanted!

Originally posted by @ntBre in #18645 (comment)

[chirizxc]

[ntBre]

Thanks for opening the issue! I think it should definitely be safe to remove these, but I'm not immediately seeing where this prefix is added in the code generation script, which I think was changed to include at least some of these in #17180. For example, I don't think I see any of the types from the screenshots here:

ruff/crates/ruff_python_ast/generate.py

Lines 17 to 21 in 02394b8

	# Types that require `crate::`. We can slowly remove these types as we move them to generate scripts.
	types_requiring_create_prefix = {
	"IpyEscapeKind",
	"ExprContext",
	"Identifier",

[dcreager]

As @chirizxc mentions #18652 (comment), this is the unused_qualifications lint from rustc (not clippy). That lint is marked as allow by default, because:

it is somewhat pedantic, and doesn't indicate an actual problem, but rather a stylistic choice, and can be noisy when refactoring or moving around code

If we activate this lint, then we should do so for the whole repo (as suggested here #18652 (comment)) so that this will be checked as part of CI moving forward.

That said, I'm not convinced that we should activate this lint just because rustrover has a different default than rustc. If we do, it should be because we agree with its suggestions. I'm ambivalent to mildly against. @ntBre @MichaReiser, do you have strong feelings on this?

[ntBre]

I don't have strong feelings. I thought it might be nice and low-impact just for the generated code, but I didn't think about enabling the lint or the churn that could cause elsewhere. I usually prefer the style from the lint and fix it in my code when I notice it, but I definitely don't feel strongly enough to impose it throughout the project. And I agree with you that that's what the decision boils down to!

[chirizxc]

maybe just put #![deny(unused_qualifications)] at the beginning of the generated file?

[dcreager]

maybe just put #![deny(unused_qualifications)] at the beginning of the generated file?

I'm not in favor of that — I feel we should either enforce the lint for the whole repo or not at all.

[MichaReiser]

I sometimes remove unnecessary prefixes if I come across them, but not always. For example, it has become an established pattern to use ast::Node in ty, and the prefix shouldn't be removed when the IDE imports a node directly. That's why I don't think we should enable this lint.

However, I don't mind removing the unnecessary prefixes in the generated file if it doesn't introduce extra complexity. I don't think it's worth it even if it adds just one line of extra branching in the generation script because the prefix doesn't hurt anyone and it's a generated fiel that no one should edit anyway.

[chirizxc]

what are we supposed to do about it? close?

[MichaReiser]

Yeah, I'm leaning towards closing unless the "cleanup" comes at no extra cost (which this discussion already shows, it does not 😆)