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

Open
ehuss opened this issue Mar 25, 2024 · 10 comments · May be fixed by #124895
Open

static_mut_refs: Should the lint cover hidden references? #123060

ehuss opened this issue Mar 25, 2024 · 10 comments · May be 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 The issue / PR has been 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 The issue / PR has been 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

@obeis obeis linked a pull request May 8, 2024 that will close this issue
@traviscross
Copy link
Contributor

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

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