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.

Sign up for GitHub

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

Jump to bottom

Add a strange test for unsafe_code lint. #89821

Merged
merged 1 commit into from
Oct 15, 2021
Merged

Add a strange test for unsafe_code lint. #89821

merged 1 commit into from
Oct 15, 2021

Conversation

crlf0710
Copy link
Member

@crlf0710 crlf0710 commented Oct 12, 2021 •
edited by the8472
Loading

The current behavior is a little surprising to me. I'm not sure whether people would change it, but at least let me document the current behavior with a test.

I learnt about this from the totally-speedy-transmute crate.

cc #10599 the original implementation pr.

@crlf0710 crlf0710 added A-testsuite Area: The testsuite used to check the correctness of rustc A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels Oct 12, 2021
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@taiki-e
Copy link
Member

taiki-e commented Oct 12, 2021

cc #10599 the original implementation pr.

FYI: #52467 is the PR that introduced the current behavior.

@Mark-Simulacrum
Copy link
Member

I think the particular case tested here probably shouldn't lint, since the macro has requested a call site span. However, I believe the lint doesn't currently consider the span of the unsafe block at all.

That seems like it might be good to fix to help with writing macros which enforce safety through static checking at compile-time (like println!) and currently can't easily do so without allowing unsafe in their arguments as well, due to needing to make sure the unsafe block is appropriately scoped. For example, see #89139. But it's also totally fine to leave that for future PR(s) or discussions; we can merge the test in the meantime. I think the broader changes here would likely need to start as a T-lang MCP proposing an initiative to look at the unsafe code interaction with macros, though maybe a PR would be small enough to be OK. I'm a little hesitant personally to pursue small steps without a broader plan in mind though here.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2021
@crlf0710 crlf0710 force-pushed the unsafe_code_lint_test branch from db3d234 to 7326a97 Compare October 13, 2021 17:45
@crlf0710
Copy link
Member Author

I think i'd recommend just leave any potential discussions or changes to the future, because any change would quite likely break existing code in the wild. We can just land this test in the test suite so this special case won't get forgotten.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2021
@crlf0710 crlf0710 force-pushed the unsafe_code_lint_test branch from 7326a97 to 545de51 Compare October 13, 2021 19:10
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 2021
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2021
@apiraino apiraino added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Oct 14, 2021
@crlf0710 crlf0710 force-pushed the unsafe_code_lint_test branch from 545de51 to c76c620 Compare October 14, 2021 17:21
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Oct 14, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 15, 2021

📌 Commit c76c620 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2021
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 15, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2021
@bors
Auto merge of rust-lang#89903 - matthiaskrgr:rollup-s0c69xl, r=matthi…
72d6606 
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#86011 (move implicit `Sized` predicate to end of list)
 - rust-lang#89821 (Add a strange test for `unsafe_code` lint.)
 - rust-lang#89859 (add dedicated error variant for writing the discriminant of an uninhabited enum variant)
 - rust-lang#89870 (Suggest Box::pin when Pin::new is used instead)
 - rust-lang#89880 (Use non-checking TLS relocation in aarch64 asm! sym test.)
 - rust-lang#89885 (add long explanation for E0183)
 - rust-lang#89894 (Remove unused dependencies from rustc_const_eval)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b74ae04 into rust-lang:master Oct 15, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 15, 2021
@crlf0710 crlf0710 deleted the unsafe_code_lint_test branch January 24, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.
8 participants
@crlf0710 @rust-highfive @rust-log-analyzer @taiki-e @Mark-Simulacrum @bors @apiraino @rustbot