Make the dangerous_implicit_autorefs lint deny-by-default

[Urgau]

I intended for the dangerous_implicit_autorefs lint to be deny-by-default, the T-lang nomination comment even clearly mentioned deny-by-default, but somehow I and other missed that it is only warn-by-default.

I think the lint should still be deny-by-default as the implicit aliasing requirements can be quite dangerous.

In any-case, opening this PR for T-lang awareness.

@rustbot label +I-lang-nominated +T-lang
r? @traviscross

[traviscross]

@rustbot labels +P-lang-drag-1

Thanks @Urgau. Somehow we all missed this discrepancy. It's too late, in my view, for this to go in for Rust 1.88; we want some time in nightly and a full beta cycle with this as deny-by-default. So let's just make the change and put it on the train.

Given that many of us reviewed the diff on #123239, and that, in reviewing our minutes, we did not discuss the lint level specifically, it's actually hard to say for sure what we intended, and the situation has now changed, so let's propose to bump this lint level by FCP.

@rfcbot fcp merge

[rfcbot]

Team member @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]

I think the lint should still be deny-by-default as the implicit aliasing requirements can be quite dangerous.

I buy that they can be dangerous, but deny-by-default still seems strong to me unless we can show that code triggering this lint is "almost certainly UB".

Deny-by-default is a funky middle ground between warning and hard error that we have typically reserved for operations that we wish we had made errors to begin with, and that now have a better alternative available. I'm not sure we can say that here yet.

[traviscross]

Perhaps the most similar comparable is what we did in making static_mut_refs, including ones from autoref, deny-by-default in Rust 2024.

[Nadrieril]

This is not as footgun-heavy as static_mut_refs, however I see the appeal in deny-by-default for reviewers: if I'm reviewing unsafe code I really don't want to have to run type inference in my head to figure out if there's a hidden autoref.

[traviscross]

I notice that ImplicitUnsafeAutorefsSuggestion is marked as "maybe-incorrect". What's the theory on this, in terms of not marking it "machine-applicable"? I'm guessing it's because we're not sure whether the user really meant for reference semantics to apply here. On the other hand, our docs for "machine-applicable" say:

The suggestion is definitely what the user intended, or maintains the exact meaning of the code.

Does this suggestion do the latter?

Probably I wonder whether having a machine-applicable fix might make the stronger lint level more appealing to everyone.

[tmandry]

This is not as footgun-heavy as static_mut_refs, however I see the appeal in deny-by-default for reviewers: if I'm reviewing unsafe code I really don't want to have to run type inference in my head to figure out if there's a hidden autoref.

That's compelling to me. @traviscross also said in the lang team meeting something along the lines of "we want people to write this explicit form all the time so that when you're reviewing you don't have to think about whether there's implicit autoref", which I agree with.

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[Urgau]

I notice that ImplicitUnsafeAutorefsSuggestion is marked as "maybe-incorrect". What's the theory on this, in terms of not marking it "machine-applicable"?

My reasoning when implementing the lint is that it's not clear if the user intended to use a implicit reference, some cases are really subtle, and as such I fear that if we made it machine-applicable users would just cargo fix their code without looking at the lint.

That's also why the "make it explicit" is the 2nd suggestion, preceded by using raw pointer method:

help: try using a raw pointer method instead; or if this reference is intentional, make it explicit

[scottmcm]

that we have typically reserved for operations that we wish we had made errors to begin with

I've generally phrased this as things where we have to define what it does anyway, and thus where making it a hard error would actually be more annoying than it being a lint, especially if it's something worth tuning later. (Notably here there's no question of what it actually means, just that it's probably not what you meant.)

I don't know that this is necessary the "not even worth running tests" bar that I've sometimes used for deny-by-default, but I'm fine with going deny-by-default -- I agree this is worth strongly conveying and we can always re-evaluate later if necessary.

@rfcbot reviewed