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

static_mut_refs: Should the lint cover hidden references? #123060

Closed
ehuss opened this issue Mar 25, 2024 · 10 comments · Fixed by #124895
Closed

static_mut_refs: Should the lint cover hidden references? #123060

ehuss opened this issue Mar 25, 2024 · 10 comments · Fixed by #124895
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-edition-2024 Area: The 2024 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

@ehuss
Copy link
Contributor

ehuss commented Mar 25, 2024

There is some discussion on #114447 (comment) about whether or not static_mut_refs should be triggering on code that takes references without using the & sigil, such as method calls.

This should probably be resolved soon if this is to be part of the edition. If this needs to change after the edition, the implementation will likely require separate code paths for the lint versus edition behavior, which could become confusing, or risk unintentional breakage.

@ehuss ehuss added A-diagnostics Area: Messages for errors, warnings, and lints T-lang Relevant to the language team, which will review and decide on the PR/issue. A-edition-2024 Area: The 2024 edition labels Mar 25, 2024
@traviscross
Copy link
Contributor

traviscross commented Mar 25, 2024

@rustbot labels +I-lang-nominated

What we need to discuss is whether, e.g., these cases should trigger an error in Rust 2024:

// edition: 2024

static mut STATIC: &[u8; 3] = &[0, 1, 2];

fn main() { unsafe {
    let _ = STATIC.len();
    let _ = STATIC[0];
    let _ = format!("{:?}", STATIC);
}}

Playground link

What was implemented in #117556 does not trigger an error for these. However, @scottmcm expressed a view that these should be included also:

I think this should be linting about anything that's created enough of a reference to trigger the reference rules -- if it can trigger UB when someone else holds a &mut to the static, that's worth linting about here.

cc @obeis @scottmcm @RalfJung @est31

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Mar 25, 2024
@scottmcm
Copy link
Member

I think that, if anything, the invisible ones are more important to lint(/break).

At least if someone is doing &STATIC they have a chance at spotting it, but STATIC.frobble() taking a secret reference makes it way harder to manually check.

@scottmcm
Copy link
Member

I think it should cover hidden references, as expressed above.

Triage was sparsely attended today, but maybe we can get team agreement async:
@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 27, 2024

Team member @scottmcm 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 Mar 27, 2024
@Dirbaio
Copy link
Contributor

Dirbaio commented Apr 17, 2024

+1 to this. I was quite confused when I saw that MY_STATIC.some_mut_self_method() doesn't generate a warning, since it's the same in the end as &mut MY_STATIC. I've also seen people in embedded bypassing the lint by rearranging code to get the reference by calling a &mut self method, without being aware it's "equally bad" as their previous code.

@tmandry
Copy link
Member

tmandry commented Apr 17, 2024

Agreed that this should cover hidden references. Not sure if we need an FCP to decide it, but

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 17, 2024
@rfcbot
Copy link

rfcbot commented Apr 17, 2024

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Apr 17, 2024
@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 24, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 27, 2024
@rfcbot
Copy link

rfcbot commented Apr 27, 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.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 29, 2024
@obeis
Copy link
Contributor

obeis commented May 6, 2024

@rustbot claim

@traviscross
Copy link
Contributor

We reviewed this in the edition call today. We'll close this when we merge #124895.

tgross35 added a commit to tgross35/rust that referenced this issue Jul 23, 2024
…haelwoerister

Disallow hidden references to mutable static

Closes rust-lang#123060

Tracking:
- rust-lang#123758
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jul 23, 2024
…haelwoerister

Disallow hidden references to mutable static

Closes rust-lang#123060

Tracking:
- rust-lang#123758
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 23, 2024
…haelwoerister

Disallow hidden references to mutable static

Closes rust-lang#123060

Tracking:
- rust-lang#123758
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 24, 2024
…elwoerister

Disallow hidden references to mutable static

Closes rust-lang#123060

Tracking:
- rust-lang#123758
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 25, 2024
…elwoerister

Disallow hidden references to mutable static

Closes rust-lang#123060

Tracking:
- rust-lang#123758
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 30, 2024
…elwoerister,traviscross

Disallow hidden references to mutable static

Closes rust-lang#123060

Tracking:
- rust-lang#123758
@bors bors closed this as completed in 5ba6db1 Sep 20, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 24, 2024
…ler-errors

Disallow hidden references to mutable static

Closes rust-lang#123060

Tracking:
- rust-lang#123758
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-edition-2024 Area: The 2024 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
None yet
Development

Successfully merging a pull request may close this issue.

9 participants