Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch mutable_borrow_reservation_conflict lint to deny by default #76104

Conversation

marmeladema
Copy link
Contributor

Lint mutable_borrow_reservation_conflict was added about a year and a half ago in warning mode by default. Issue #59159 states that at some point, it should be set to deny by default which this PR implements.

I am mainly creating this PR to start the discussion and try to help stabilizing full nll.
I also don't know if we need a crater run for this kind of change?

Assigning to @pnkfelix because of the tracking issue but feel free to re-assign to whoever might be better suited to review this change.

r? @pnkfelix

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2020
@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@marmeladema marmeladema force-pushed the mutable-borrow-reservation-conflict-lint-deny-by-default branch from 188ae3c to 82833e0 Compare August 30, 2020 19:16
@jyn514 jyn514 added the A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. label Sep 3, 2020
@Aaron1011
Copy link
Member

The crater queue is pretty small, so let's kick off a run to see what the breakage looks like.

@bors try

@bors
Copy link
Contributor

bors commented Sep 15, 2020

⌛ Trying commit 82833e0 with merge b1cc2963e8346ba1f5afb1edb86dbcf3f9ef0c5f...

@bors
Copy link
Contributor

bors commented Sep 15, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: b1cc2963e8346ba1f5afb1edb86dbcf3f9ef0c5f (b1cc2963e8346ba1f5afb1edb86dbcf3f9ef0c5f)

@Aaron1011
Copy link
Member

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-76104 created and queued.
🤖 Automatically detected try build b1cc2963e8346ba1f5afb1edb86dbcf3f9ef0c5f
🔍 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

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-76104 is now running

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

@Aaron1011
Copy link
Member

Yielding to a very small re-test of #76130

@craterbot retry

@craterbot
Copy link
Collaborator

🚨 Error: Experiment pr-76104 didn't fail!

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-76104 is completed!
📊 117 regressed and 7 fixed (122155 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 22, 2020
@marmeladema
Copy link
Contributor Author

Hello @Aaron1011 and thank to have started the crater run.

I looked at various regressions and they all seem legit. Unfortunately the 117 are individual regressions which mean there is not a handful of crate to fix but rather all of them.
Compared to other similar changes, I don't know if 117 regressions is "acceptable"? Or should I provide a fix for all of the 117 crates? I can do that but that will definitely take a while.

Unrelated question, for how long is the crater report going to be available for?

@Aaron1011
Copy link
Member

@marmeladema: I'm not sure what amount of breakage is considered acceptable for this lint - @nikomatsakis or someone else should weigh in.

@rust-lang/infra Are Crater reports hosted indefinitely?

@pietroalbini
Copy link
Member

Are Crater reports hosted indefinitely?

Yes. If we're going to ever start deleting them (which we don't plan to) we'll surely keep the past couple months of reports available.

@pnkfelix
Copy link
Member

pnkfelix commented Oct 2, 2020

The lang team had agreed (described here) that we need to do a more thorough evaluation of the semantic models before we convert this to a hard error.

I think several members of the lang team would be annoyed if we turned this into a hard error simply because "its been a warning for so long." We reached the compromise that we adopted in part because we promised we would not do that.

@pnkfelix
Copy link
Member

pnkfelix commented Oct 2, 2020

(However, I'm also not sure whether turning this lint to deny-by-default needs to be coupled to turning on feature(nll) by default? maybe I misremember, but I had thought this lint was largely independent of all the other stuff turned on by feature(nll)...)

@pnkfelix
Copy link
Member

pnkfelix commented Oct 2, 2020

nominating for discussion at T-lang meeting, so that we can ponder when/if we are going to have time to do the follow-on work of evaluating semantic models et cetera that was promised here.

@pnkfelix pnkfelix added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 2, 2020
@joshtriplett
Copy link
Member

joshtriplett commented Oct 5, 2020

I agree that we shouldn't just switch to deny just because time has passed.

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum I might be reading this wrong, but this crater report appears to me to suggest 113 regressions due to 'mutable borrow reservation conflict". From poking at random entries they all appeared to be root regressions.

Yeah, taking another look I think this is right. I will note that the vast majority are GitHub repositories, many of which are advent of code or similar -- I think that likely means that the lint has indeed discouraged new uses of this pattern forming in code people are actively touching (unsurprisingly).

It seems like all of the cases this lint touched that I saw in the code examples are quite simple and usually occur on just 1-4 lines without anything all that complicated in between (if anything), leading me to feel that in practice we're not likely to lose much without the definite aliasing information (since there's not much that could be done); furthermore I'd sort of expect that perhaps the aliasing information could be derived from the code in many cases as well through local analysis, even if it is not guaranteed by the language to always be there. Maybe I'm reading too much into these patterns though.

I am also inclined to remove/silence this lint, personally.

@nikomatsakis
Copy link
Contributor

Update from @rust-lang/lang meeting:

Last week @pnkfelix mentioned they'd look into some patterns. Removing nomination for now but we don't have agreement to go forward.

@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 4, 2020
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 15, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 5, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Apr 2, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Apr 23, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 14, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 5, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 25, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 17, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Aug 14, 2021
@Dylan-DPC-zz
Copy link

@pnkfelix @nikomatsakis any update on this? I'm leaning towards closing this for now if @marmeladema and lang team is fine with this since it's been sitting around for sometime now

@nikomatsakis
Copy link
Contributor

No update as far as I know -- I am grateful to @marmeladema for preparing this PR -- closing is probably reasonable though, I think this isn't the major blocker for completing NLL. That said, we really should try to settle this question. It is going to be progressively more difficult to turn this lint into a hard error, which makes me wonder if indeed we even should.

@bstrie
Copy link
Contributor

bstrie commented Feb 11, 2022

It is going to be progressively more difficult to turn this lint into a hard error, which makes me wonder if indeed we even should.

AIUI, the reason that this lint was not turned into an error for the 2021 edition is because it was decided that things like this can be done at any time, without an edition. If people are concerned that that would lead to too much breakage, then rather than close this entirely I'd prefer to just resubmit this as an item for a future edition.

@nikomatsakis
Copy link
Contributor

I think we should close this PR for now. The number of regressions is non-trivial and I am very interested in whether we can make this pattern be accepted by extending stacked borrows in some way (or perhaps our compilation into MIR). If not, I would be ok going forward, but it's obviously going to require a bit of a "phasing-in".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.