make semicolon_in_expressions_from_non_local_macros not report-in-deps

[RalfJung]

View all comments

See #162872 for context. This new lint was made immediately report-in-deps without a crater run. I guess people assumed that the ecosystem fallout would be small, but no data was gathered to confirm that hypothesis. We now have data showing that the ecosystem fallout is gigantic, so let's remove the report-in-deps from this lint.

This should be backported to the 1.100 beta. (1.99 already has this patch.)

Cc @joshtriplett

[rustbot]

r? @mati865

rustbot has assigned @mati865.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 77 candidates
• Random selection from 20 candidates

[joshtriplett]

Part of the issue here is that this is always effectively "report in deps", it's just a question of whether it's "report only in direct deps" or "report in indirect deps". "Report in the original crate" isn't an option available to us.

[joshtriplett]

Is there a straightforward way for us to find out how much changing this will actually reduce the number of crates directly impacted?

[RalfJung]

Yeah this lint is kind of special because it is about macros so it's a bit hard to reason about. But this is a new category of cases we are linting about, where the crate defining the macro doesn't use it (or not in a problematic way, anyway) and so the old lint never triggered. It seems reasonable to me to stage the new lint by first only showing it for direct dependencies that actually use the macro, where this may be a bit less confusing to debug.

Is there a straightforward way for us to find out how much changing this will actually reduce the number of crates directly impacted?

In #162872 the suggestion was to do a crater run with the lint set to deny. We can try doing that, I don't know how well it'll work.

@bors try

[RalfJung]

If we use the "root regressions" vs "dependencies" in https://crater-reports.s3.amazonaws.com/pr-162768/index.html as indication, then apparently 1500 of the errors are root regressions, around 550 of these are from crates.io. So this would drastically reduce how often the lint is shown. That seems to indicate that in most cases, a newer version of the dependency (though potentially not semver-compatible) is available to fix the problem. OTOH >500 crates with root regressions is still a substantial number.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 53347bd (53347bd049319c6f1fbbeda74b547c1c9ac10fe6)
Base parent: a22b02e (a22b02eaecd6ac937d752139c79d0159c725932d)

[RalfJung]

@craterbot check start=a22b02eaecd6ac937d752139c79d0159c725932d end=53347bd049319c6f1fbbeda74b547c1c9ac10fe6+rustflags=-Dsemicolon_in_expressions_from_non_local_macros

[craterbot]

👌 Experiment pr-163364 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[joshtriplett]

where the crate defining the macro doesn't use it (or not in a problematic way, anyway) and so the old lint never triggered

Or where the crate defining it only tests it in integration tests, which built as a separate crate, so they never got the lint.

(I'm not going to pretend that the majority of macro crates have thorough tests, but in the case of this particular lint, integration tests wouldn't have helped either.)

[craterbot]

🚧 Experiment pr-163364 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[joshtriplett]

We talked about this in today's @rust-lang/lang meeting. We don't expect this to make the warning any more targeted to the crates affected (it's still always going to hit dependents rather than ), but it'll reduce the sheer number of hits in the ecoystem, and given the quantity that seems warranted while fixes are rolling out.

@rfcbot merge lang

We should waive the 10-day period if we get all the checkboxes.

[rust-rfcbot]

@joshtriplett 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.

[traviscross]

Thanks @RalfJung.

@rfcbot reviewed

[scottmcm]

Agreed we should avoid being too noisy initially if doing so is reasonable
@rfcbot reviewed

[RalfJung]

It is still approved. bors just didn't notice yet that it failed, apparently.
Cc @Kobzol

[rust-bors]

📌 Commit 3631ff3 has been approved by mati865

It is now in the queue for this repository.

[rust-bors]

💔 Test for 9cf9c87 failed: CI. Failed job:

• auto - dist-aarch64-apple (web logs, enhanced plaintext logs)

[theemathas]

@bors r=mati865

[rust-bors]

📌 Commit 3631ff3 has been approved by mati865

It is now in the queue for this repository.

A failed build status on this PR was cleared due to the approval.

[RalfJung]

(retry would likely have worked now and does not require the label shuffling)

[rust-bors]

⌛ Testing commit 3631ff3 with merge 846251e...

Workflow: https://github.com/rust-lang/rust/actions/runs/37348086272

[JonathanBrouwer]

@bors yield
Yielding to enclosing rollup

[rust-bors]

Auto build was cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/37348086272

The next pull request likely to be tested is #163829.

[Kobzol]

It is still approved. bors just didn't notice yet that it failed, apparently. Cc @Kobzol

Looks like GitHub didn't send us the failure webhook, and not enough time has passed for bors to run its background check to timeout or fail the workflow on its own. @bors yield can be used to restart the workflow, if needed.

[RalfJung]

In old bors, retry would cause bors to abort the currently running job if there was any, I think? So, it could be used to restart a hung CI, for instance. Maybe it would make sense to also make new bors behave like that?

[Kobzol]

It did not, in fact :) It only cleared the failure bit of the build.

I'd like to keep that the same, to avoid the commands doing too much. @bors retry clears the failure bit, and @bors cancel/yield aborts a currently running build.

[rustbot]

beta backport approved as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels are handled by them.