Make SEMICOLON_IN_EXPRESSIONS_FROM_MACROS warn by default

[Aaron1011]

This PR makes the SEMICOLON_IN_EXPRESSIONS_FROM_MACROS lint warn by default.

To avoid showing a large number of un-actionable warnings to users, we only enable the lint for macros defined in the same crate. This ensures that users will be able to fix the warning by simply removing a semicolon.

In the future, I'd like to enable this lint unconditionally, and eventually make it into a hard error in a future edition. This PR is a step towards that goal.

[Aaron1011]

A Crater run with this lint turned into an error (#81477) showed 1313 root regressions. The actual number of crates affected in practice may be smaller, since the warning will be suppressed in upstream dependencies.

This is an unusual kind of lint - it only fires when an expression-position macro_rules! expands to something with a trailing semicolon. If a user calls a macro in an upstream crate with this issue (e.g. old versions of log::log!), then we will emit a warning that can only be fixed by the upstream crate author.

An alternative would be to only emit this warning for macros in the same crate, and rely on the (still unstable) future-incompat-report feature to notify users about the issue when it affects foreign crates.

cc @rust-lang/lang

[Mark-Simulacrum]

Is there a summary of the crates/actual macros causing those regressions? Based on your description it sounds like in practice, the regressions are plausibly incorrectly attributed to the crate invoking the macro (rather than the macro author). Do we have some assessment of how many of these could be fixed locally?

For example, if the vast majority are fixable with a cargo update -p log or something like that, it seems likely that moving ahead would be more acceptable, but if the fixes to upstream crates are not available I at least would be more uncertain. It seems like a good idea to know at least what authors are expected to do. If we choose to enable the lint, I think we should at least improve the diagnostic it gives to provide a clear indication when it is in fact the case that they cannot fix it locally and need to patch the upstream dependency or update it.

(As an aside, it'd be great to put more context in the PR description per recent compiler team meetings -- at least a link to the tracking issue, #79813, seems appropriate).

[Aaron1011]

Based on your description it sounds like in practice, the regressions are plausibly incorrectly attributed to the crate invoking the macro (rather than the macro author).

The issue is that we can only detect this at macro invocation time. A defintion like macro_rules! foo { () => { true; } } would be fine if it's only every used in statement position (e.g. fn bar() { foo!(); foo!(); }). However, it cannot be used in expression position, and we may only ever see it used it expression position when compiling a downstream crate.

[Mark-Simulacrum]

Yes, I understood that. I'm trying to see if we have a sense of how many macros need to be adjusted (presumably in most cases just dropping the semicolon), and the percentage of those which are in the same crate as the problematic call site.

[Aaron1011]

@Mark-Simulacrum: I've having trouble getting a sense of where the regressions are coming from - there's so many failed crates in the Crater run.

To make progress on this lint, I think we could start by supressing the warning in the cross-crate case, and only emit it when the macro definition and invocation are in the same crate. That will give us a chance to get feedback on the behavior of the lint, without flooding people with un-actionable warnings.

[Mark-Simulacrum]

That makes sense to me. It should be backwards compatible (including with older Rust compilers) to remove the semicolon, right?

I think we could also get a sense of the impact by doing another crater run with the suppression on - that would give us a sense of how many crates are now "fixed", compared to the crater run you just did for this PR.

[Aaron1011]

It should be backwards compatible (including with older Rust compilers) to remove the semicolon, right?

That's right. There's explicit code in the compiler to ignore a trailing semicolon when expanding to an expression, so removing the semicolon will just do the right thing.

[Aaron1011]

I've suppressed the lint when the macro is defined in a foreign crate.

[petrochenkov]

(The nomination was for the compiler team apparently, so I've added the corresponding label.)

[Mark-Simulacrum]

I added it intending T-lang - I think we can get approval or at least tentatively so on the question of enabling this for users who can do something about it. Not sure if data will be necessary, but seems good to raise.

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting. We agree with upgrading this lint to a warning, and about only warning about macros in the same crate.

Not a blocker for this PR, but in the future, we'd also ideally like to see this warn about macros invoked from a test in the same crate, since a test seems the most likely place to invoke a macro from the same crate. That may or may not be straightforward, since tests are compiled as a separate crate.

[Aaron1011]

Not a blocker for this PR, but in the future, we'd also ideally like to see this warn about macros invoked from a test in the same crate,

I think the best condition for emitting the lint would be 'can the user modify the source of the crate where the macro is defined'? I believe Cargo has some logic for this, which it uses to determine whether or not to suppress warning messages. It might be useful to make this information available to rustc.

[Aaron1011]

@bors r=petrochenkov

[bors]

📌 Commit e70ce57 has been approved by petrochenkov

[bors]

⌛ Testing commit e70ce57 with merge cceadd1c0cc90c1b0c476ea250d90ed2001a227f...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[petrochenkov]

Spurious?
@bors retry