[JonathanBrouwer]

View all comments

This is an alternative approach to #164607

This can be reviewed commit by commit, each commit passes tests.
My goal is for a follow-up to remove lint_name_normalized completely and replace it by a LintId

r? @mejrs

[JonathanBrouwer]

My goal for a follow-up PR is to change this field from a Symbol that needs to be looked up in LintStore to a LintId. I think this should be possible, but encoding and decoding will be a challenge. I think this PR is already a good step forwards regardless.

View changes since the review

[JonathanBrouwer]

I'm not sure if people find it cursed to export diagnostic structs such that they can be re-used. I like it

View changes since the review

[mejrs]

I don't, it can hide unused diagnostics 😅

[JonathanBrouwer]

While unused diagnostics are bad, the alternative is duplicating the suggestion structs which risks them diverging when someone edits one but not the other. I think that's worse

[JonathanBrouwer]

@bors try @rust-timer queue

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_ir

cc @jdonszelmann

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

[rust-bors]

☀️ Try build successful (CI)
Build commit: 00e2c28 (00e2c28e2c046bbcd149ca7f1bf88c8cfc9e8875)
Base parent: dc44e7f (dc44e7f2c436b6041742e138b9c2176f093196d5)

[rust-timer]

Finished benchmarking commit (00e2c28): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@bors rollup=iffy rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.1%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.1%]	9

Max RSS (memory usage)

Results (primary -0.5%, secondary -0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.9%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.4%	[-3.1%, -0.5%]	6
Improvements ✅ (secondary)	-0.7%	[-0.7%, -0.7%]	1
All ❌✅ (primary)	-0.5%	[-3.1%, 0.9%]	11

Cycles

Results (primary 0.4%, secondary 0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.5%, 2.1%]	9
Regressions ❌ (secondary)	1.3%	[0.5%, 3.4%]	5
Improvements ✅ (primary)	-1.0%	[-1.7%, -0.7%]	3
Improvements ✅ (secondary)	-1.3%	[-2.1%, -0.5%]	2
All ❌✅ (primary)	0.4%	[-1.7%, 2.1%]	12

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 487.351s -> 486.11s (-0.25%)
Artifact size: 405.87 MiB -> 406.01 MiB (0.03%)

[mejrs]

I don't think this change is correct, it means lowering ast to hir depends on what lints have been registered. Does this mean cargo clippy and cargo check could produce different artefacts? What happens when rust-analyzer decides to implement its own lints? (it's possible, it's a pregistered tool).

Can you talk to people more knowledgeable about this and get them to sign off on this? The people that are working on sharing cargo build caches probably have opinions about this.

@rustbot author

View changes since this review

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[traviscross]

Makes sense to me. Thanks @JonathanBrouwer and @mejrs.

@rfcbot fcp merge lang

[rust-rfcbot]

@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[tmandry]

Makes sense. Thanks @JonathanBrouwer.

@rfcbot reviewed

[traviscross]

We talked about this in the lang call today. With @tmandry's agreement as well (since we were the two boxes checked):

@rfcbot fcp cancel

@joshtriplett will leave a comment with more details.

[rust-rfcbot]

@traviscross proposal cancelled.

[joshtriplett]

We talked about this in today's @rust-lang/lang meeting. On further reflection, we don't think we should do this as proposed.

If we're understanding correctly, this would in general reject unknown lint names pre-macro-expansion. So, for instance, this would error after this PR:

macro_rules! discard {
    ($( $tt:tt )*) => {}
}

fn main() {
    discard! {
        #[allow(not_a_tool::doesnt_exist)]
        #[allow(doesnt_exist)]
        #[allow(raw_pointer_derive)]
        println!();
    }
}

(If that isn't the case please tell us that, as it affects our conclusion here.)

Rejecting unknown lint names pre-macro-expansion seems surprising. If the macro doesn't emit invalid attributes, there shouldn't be an error if its input contained them. The macro could drop them on the floor as above, or do something creative with them.

[steffahn]

If we're understanding correctly, this would in general reject unknown lint names pre-macro-expansion.

I just tested this,¹ it is not rejected.

Footnotes

1. with rustup-toolchain-install-master 47dd86e0ec4beaca68099fc233dd505f33dcd315 ↩

[JonathanBrouwer]

@joshtriplett
The code in your example would not emit diagnostics with this PR.
This PR emits the semantic diagnostics on lint attributes on macros, just before they're expanded.
The println!() macro is never expanded in your example, and therefore the attributes on the macro are never semantically checked.

[joshtriplett]

@JonathanBrouwer So, if these attributes were applied to a non-macro, this would likewise not be an error, and this is just about when we consider attributes to be processed when they're applied to a macro? Essentially, "attributes applied to a macro will be processed when that macro is expanded"?

[JonathanBrouwer]

The following code already emits diagnostics on main:

identity!{
    #[allow(not_a_tool::doesnt_exist)] // ERROR "unknown tool name `not_a_tool`"
    #[allow(doesnt_exist)] // LINT "unknown lint: `doesnt_exist`"
    #[allow(raw_pointer_derive)] // LINT "lint `raw_pointer_derive` has been removed"
    struct Test;
}

This PR specifically changes behavior for lint attributes that don't exist on the AST before expansion, and don't exist on the AST after expansion, but only exist temporarily during expansion. Attributes on macro calls are special because they are discarded after the macro is expanded.

[mejrs]

I think there is a misunderstanding here.

In Rust, macros expand recursively. If a macro's invocation produces another macro as part of its expansion, that macro will be expanded as well. Take for example, this recursive macro:

macro_rules! a {
    () => { a!(1); };
    (1) => { a!(2); };
    (2) => { struct Foo; };
}

Let's walk through an invocation of this macro step by step:

a!();

// ..expands into..

a!(1);

// ..expands into..

a!(2);

// ..expands into..

struct Foo;

// no more macros to expand

Now take the macro in the example, and invoke it like in the example, again one step at a time, now nested in this macro:

macro_rules! identity {
    ($( $tt:tt )*) => { $( $tt )* }
}

identity!{
    #[allow(unregistered::doesnt_exist)] 
    a!();
}

// ..expands into..

#[allow(unregistered::doesnt_exist)] 
a!();

// ..expands into..

a!(1);

// ..expands into..

a!(2);

// ..expands into..

struct Foo;

// no more macros to expand

This time, skip the identity macro; expand step 2 directly:

#[allow(unregistered::doesnt_exist)] // ERROR: unknown tool name `unregistered`
a!();

This is the status quo, where we have this weird corner case where something raises an error, except if it only exists during macro expansion.

The reason why this happens is:

• lint names are only checked during lint passes
• lint passes are run at three stages:
• • pre macro expansion
• • after all macros have expanded
• • much later, when all types etc are known
• as a result, lints have a limitation where they don't get to observe the intermediate stages of macro expansion
• lint names are not checked if they only exist during the intermediate stages of macro expansion

This PR fixes the first step, rather than checking lint names during lint passes they are checked during attribute parsing, meaning lint names are now always checked and the "identity" example will now do:

identity!{
    #[allow(unregistered::doesnt_exist)] 
    a!();
}

// ..expands into..

#[allow(unregistered::doesnt_exist)] // ERROR: unknown tool name `unregistered`
a!();

I consider this a bug fix, but it's a breaking change, a highly theoretical one if you ask me.

[petrochenkov]

Currently, lint attributes on macro calls produced by macros are not semantically validated

I think the current behavior is correct and doesn't need fixing, this was my last point in #165073 (comment).
Until the top level macro (println!(); in your example) is expanded, the stuff in inert attributes is just tokens without any semantic meaning.

(Except I'd rather emit a deprecation lint for any attributes on fn-like macro calls for compatibility with #63221 (comment), but that's not specific to lint attributes.)

[JonathanBrouwer]

I think the current behavior is correct and doesn't need fixing, this was my last point in #165073 (comment).
Until the top level macro (println!(); in your example) is expanded, the stuff in inert attributes is just tokens without any semantic meaning.

@petrochenkov This is not the current behaviour though, on main:

identity!{
    #[allow(INVALID = INVALID)] // ERROR malformed attribute
    #[must_use(INVALID)] // ERROR malformed attribute
    println!();
}

For almost all attribute, even for lint attributes, the current behavior is to emit diagnostics for inert attributes in this position. The diagnostics are emitted after expanding ident!, but before expanding println!. It is correct that before identity! is expanded, things are just tokens. A similar example with discard! (which doesn't emit anything) would not emit any diagnostics.

This PR is an attempt to make a small change to make things more consistent, very specific to a few diagnostics emitted by lint attributes. The change of this PR would still make sense if we end up deciding attributes in this position should not emit diagnostics, which would be achieved by then removing this call:

rust/compiler/rustc_expand/src/expand.rs

Line 2239 in bde452a

AttributeParser::parse_limited_all(

[petrochenkov]

For almost all attribute, even for lint attributes, the current behavior is to emit diagnostics for inert attributes in this position.

Yeah, I think this is also not the right behavior.

[mejrs]

I think the current behavior is correct and doesn't need fixing

Then the current state remains inconsistent:

identity!{
    #[allow(unregistered::doesnt_exist)] // OK
    mac!();
}

#[allow(unregistered::doesnt_exist)] //  ERROR
mac!();

This doesn't fit in my mental model of macro expansion. We should not reject something normally but accept it if it originates from a macro expansion. These examples should be either both an error (my preference) or they should both be accepted.

[JonathanBrouwer]

I would like this PR not to get blocked on a discussion which of these is the desired behavior, as the change of this PR itself is desirable in either case. That discussion is better suited for #165073

[petrochenkov]

We should not reject something normally but accept it if it originates from a macro expansion.

That's true, I assumed the second example was also accepted on main.

[petrochenkov]

We should not reject something normally but accept it if it originates from a macro expansion.

That's true, I assumed the second example was also accepted on main.

Why it's not accepted on main, though, from implementation point of view?
I assumed the logic in compiler/rustc_lint/src/levels.rs cannot be reached (before this PR) if the attribute is dropped during expansion.
Or is it related to pre-expansion linting again? Then the difference between the two examples would be explainable.

[JonathanBrouwer]

Or is it related to pre-expansion linting again

This is indeed caused by pre-expansion linting :)
This PR moves the diagnostic from linting to attribute parsing, giving us more control of when the diagnostics are emitted