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

Make unsafe_op_in_unsafe_fn warn-by-default starting in 2024 edition #120535

Closed
tmandry opened this issue Jan 31, 2024 · 10 comments
Closed

Make unsafe_op_in_unsafe_fn warn-by-default starting in 2024 edition #120535

tmandry opened this issue Jan 31, 2024 · 10 comments
Labels
A-edition-2024 Area: The 2024 edition A-maybe-future-edition Something we may consider for a future edition. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Jan 31, 2024

The lang team recently discussed whether we should turn on the unsafe_op_in_unsafe_fn lint by default, and whether this should be done over an edition. The consensus we came to was that we should make the lint warn-by-default and do it over an edition.

Both of these are to help mitigate the disruptiveness of the change. We don't want to discourage too strongly the use of blanket allow in crates where nesting an unsafe block inside every unsafe fn is entirely redundant and unhelpful. We also would like to make use of the edition migration mechanism to help rewrite people's code and gather feedback over time as people migrate, rather than making a big change all at once.

It is possible that after gaining more experience we will want to strengthen the lint to error-by-default or to warn in all editions, but those are not part of this proposal.

@tmandry tmandry added T-lang Relevant to the language team, which will review and decide on the PR/issue. A-maybe-future-edition Something we may consider for a future edition. A-edition-2024 Area: The 2024 edition labels Jan 31, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 31, 2024
@tmandry
Copy link
Member Author

tmandry commented Jan 31, 2024

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 31, 2024

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

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 31, 2024
@scottmcm
Copy link
Member

I remain skeptical of a hard error for this, but a warning than can be allowed in everything-is-unsafe crates if needed sounds good to me. I do think that this is typically good in complex unsafe methods.

@rfcbot reviewed

@Urgau
Copy link
Member

Urgau commented Jan 31, 2024

Wasn't this already FCP-ed in #71668 (comment) ?

Because that FCP was used in #112038 to make the lint warn-by-default in the 2024 edition:

@edition Edition2024 => Warn;

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 31, 2024
@traviscross
Copy link
Contributor

@rustbot labels +I-lang-nominated

@Urgau raises a compelling point that we seem to have already agreed to do this and in fact landed a PR in Rust 2024 to this effect. Considering again my earlier question:

Given the resolution of #119823, let's discuss what else (if anything) needs to happen to land this in Rust 2024.

That would suggest that the answer is in fact, "nothing, this is all set for Rust 2024."

If this FCP merge completes, that action would of course be idempotent. Let's nominate so that if this FCP is still outstanding next week that we discuss it.

@rustbot rustbot added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Feb 1, 2024
@tmandry
Copy link
Member Author

tmandry commented Feb 7, 2024

Thanks for pointing that out, @Urgau. On the basis of the earlier FCP, I'll check the boxes of everyone who checked that FCP and leave the 10-day window for any final objections to be raised.

FYI @joshtriplett @pnkfelix

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 7, 2024
@rfcbot
Copy link

rfcbot commented Feb 7, 2024

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

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

As @tmandry suggested, in our call we agreed that we had already decided on this and that people had relied on that decision to land this in Rust 2024, so we'll idempotently let this FCP complete.

Given that the impl for this has landed, it looks like this is done and ready for Rust 2024, which is exciting.

We'll unnominate, but if any concerns do come up, please nominate for those.

@rustbot rustbot removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Feb 7, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 17, 2024
@rfcbot
Copy link

rfcbot commented Feb 17, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Feb 17, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 22, 2024
@tmandry
Copy link
Member Author

tmandry commented Apr 16, 2024

Let's close this in favor of #123916.

@tmandry tmandry closed this as completed Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition A-maybe-future-edition Something we may consider for a future edition. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Status: Done
Development

No branches or pull requests

8 participants