`#[deny(unsafe_op_in_unsafe_fn)]` in liballoc #72709

LeSeulArtichaut · 2020-05-28T21:35:52Z

This PR proposes to make use of the new unsafe_op_in_unsafe_fn lint, i.e. no longer consider the body of an unsafe function as an unsafe block and require explicit unsafe block to perform unsafe operations.

This has been first (partly) suggested by @Mark-Simulacrum in #69245 (comment)

Tracking issue for the feature: #71668.
~~Blocked on #71862.~~
r? @Mark-Simulacrum cc @nikomatsakis can you confirm that those changes are desirable? Should I restrict it to only BTree for the moment?

rust-highfive · 2020-05-28T21:41:31Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

##[section]Starting: Linux mingw-check
##[section]Starting: Initialize job
Agent name: 'Hosted Agent'
Agent machine name: 'fv-az578'
Current agent version: '2.168.2'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200517.1
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200517.1/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.2)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/840c7178-285a-4dbd-92c9-9b316ab20f42.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/72709/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/72709/merge:refs/remotes/pull/72709/merge
---
 ---> 3adb0605cc65
Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
 ---> Using cache
 ---> 28dbc326cb7f
Step 7/7 : ENV SCRIPT python3 ../x.py test src/tools/expand-yaml-anchors &&            python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python3 ../x.py build --stage 0 src/tools/build-manifest &&            python3 ../x.py test --stage 0 src/tools/compiletest &&            python3 ../x.py test src/tools/tidy &&            python3 ../x.py doc --stage 0 src/libstd &&            /scripts/validate-toolstate.sh
 ---> 537a01811900
Successfully built 537a01811900
Successfully tagged rust-ci:latest
Built container sha256:537a018119009dc218456238dec90b5530050db1e2a1e166550c218003f6159d
---
  local time: Thu May 28 21:41:13 UTC 2020
  network time: Thu, 28 May 2020 21:41:14 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/72709/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/72709/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3542) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

Mark-Simulacrum · 2020-05-28T21:43:41Z

I believe they're desirable and will likely approve this fairly soon to avoid it bitrotting since it touches a bunch of code.

However, I'm going to try and bring this up at tomorrow's compiler team meeting as an announcement and let folks chime in if they want (note: compiler team because of libs-impl area).

While I think this is a definite improvement, looking over the diff, particularly in the "mostly unsafe" functions -- it almost feels like I want to go even further, or at least experiment with doing so. Something like "unsafe only applies one level in" though exactly what levels I'm talking about I'm not sure -- but something along the lines of having one unsafe block per unsafe operation. That's obviously likely to be quite noisy so I've now been thinking about something like expr.unsafe syntax. Not sure how I feel about it quite yet though.

cc @rust-lang/lang @rust-lang/wg-unsafe-code-guidelines -- y'all are probably interested in looking at the diff here too

nikomatsakis

I'm in favor of landing the PR. I left a few suggestions for places we could "tighten" the unsafe block but I quickly got bored of that. =) I think we could refine these over time.

src/liballoc/alloc.rs

LeSeulArtichaut · 2020-05-30T09:04:34Z

#71862 landed.

rust-highfive · 2020-05-31T13:15:10Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

##[section]Starting: Linux mingw-check
##[section]Starting: Initialize job
Agent name: 'Azure Pipelines 14'
Agent machine name: 'fv-az578'
Current agent version: '2.169.1'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200517.1
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200517.1/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.2)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/3400cc97-c34c-469f-a2b6-7922619fb6be.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/72709/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/72709/merge:refs/remotes/pull/72709/merge
---
 ---> 3adb0605cc65
Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
 ---> Using cache
 ---> 28dbc326cb7f
Step 7/7 : ENV SCRIPT python3 ../x.py test src/tools/expand-yaml-anchors &&            python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python3 ../x.py build --stage 0 src/tools/build-manifest &&            python3 ../x.py test --stage 0 src/tools/compiletest &&            python3 ../x.py test src/tools/tidy &&            python3 ../x.py doc --stage 0 src/libstd &&            /scripts/validate-toolstate.sh
 ---> 537a01811900
Successfully built 537a01811900
Successfully tagged rust-ci:latest
Built container sha256:537a018119009dc218456238dec90b5530050db1e2a1e166550c218003f6159d
---
  local time: Sun May 31 13:14:50 UTC 2020
  network time: Sun, 31 May 2020 13:14:50 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/72709/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/72709/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (4087) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

nikomatsakis · 2020-06-01T18:03:30Z

To be clear, this PR is tagged as "waiting on team" -- is it waiting on the lang team? It seems like it'd be good for @rust-lang/lang folks to look at, but I don't know that it has to be blocked on the team, it seems like more of a "compiler team" (well, libs-impl) decision. Still, I'll raise it in the meeting.

rust-highfive · 2020-06-03T15:19:34Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

##[section]Starting: Linux mingw-check
##[section]Starting: Initialize job
Agent name: 'Azure Pipelines 6'
Agent machine name: 'fv-az578'
Current agent version: '2.169.1'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200517.1
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200517.1/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.3)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/f676f154-c0e9-44c5-9098-f4dec78f5e5a.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/72709/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/72709/merge:refs/remotes/pull/72709/merge
---
 ---> 3adb0605cc65
Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
 ---> Using cache
 ---> 28dbc326cb7f
Step 7/7 : ENV SCRIPT python3 ../x.py test src/tools/expand-yaml-anchors &&            python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python3 ../x.py build --stage 0 src/tools/build-manifest &&            python3 ../x.py test --stage 0 src/tools/compiletest &&            python3 ../x.py test src/tools/tidy &&            python3 ../x.py doc --stage 0 src/libstd &&            /scripts/validate-toolstate.sh
 ---> 537a01811900
Successfully built 537a01811900
Successfully tagged rust-ci:latest
Built container sha256:537a018119009dc218456238dec90b5530050db1e2a1e166550c218003f6159d
---
  local time: Wed Jun  3 15:19:23 UTC 2020
  network time: Wed, 03 Jun 2020 15:19:23 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/72709/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/72709/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3742) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

nikomatsakis · 2020-06-08T15:20:13Z

So I think we talked about this in our last lang team meeting and generally concluded that lang team approval isn't really relevant for this PR, as it's really an implementation question, though it's useful data towards the question of whether we want to alter the default levels for these lints.

I'm going to remove the T-lang notification tag -- maybe folks from @rust-lang/compiler might want to weigh in here, since we are supposed to be managing the implementation of the standard library. I suppose an MCP might be appropriate actually, this feels like a kind of "policy question".

Myself I would favor landing this PR if for no other reason than it will give us somewhat more data on whether the more precise unsafe blocks "wear well" over time.

LeSeulArtichaut · 2020-06-08T20:22:54Z

I actually meant to mark it as blocked on the #![feature(unsafe_blocks_in_unsafe_fn)] going into beta so that CI can pass without adding any sort of CFG attribute. I'm sorry if this was unclear.

Last time I tried to run CI after beta branching, it seems like it was using a cached beta so build still failed. I'll retry now and resolve the merge conflicts in the process.

LeSeulArtichaut · 2020-06-19T13:38:03Z

MCP rust-lang/compiler-team#306 has been accepted.

LeSeulArtichaut · 2020-06-19T13:54:28Z

@nikomatsakis If you are still ok with the changes, I think this should be merged as soon as possible because of the many conflicts it could cause or encounter.

nikomatsakis · 2020-06-19T15:42:34Z

@bors r+ p=1

Giving p=1 because of "high merge conflict potential"

bors · 2020-06-19T15:42:35Z

📌 Commit 7b63986 has been approved by nikomatsakis

Dylan-DPC-zz · 2020-06-19T15:44:23Z

bumping priority a bit to avoid conflicts :D

@bors p=3

bors · 2020-06-19T23:02:58Z

⌛ Testing commit 7b63986 with merge 479198141ba473aa1035d3099700b5dda0919d6c...

Manishearth · 2020-06-19T23:04:24Z

@bors yield force retry

@ghost

…arth Rollup of 13 pull requests Successful merges: - rust-lang#71568 (Document unsafety in slice/sort.rs) - rust-lang#72709 (`#[deny(unsafe_op_in_unsafe_fn)]` in liballoc) - rust-lang#73214 (Add asm!() support for hexagon) - rust-lang#73248 (save_analysis: improve handling of enum struct variant) - rust-lang#73257 (ty: projections in `transparent_newtype_field`) - rust-lang#73261 (Suggest `?Sized` when applicable for ADTs) - rust-lang#73300 (Implement crate-level-only lints checking.) - rust-lang#73334 (Note numeric literals that can never fit in an expected type) - rust-lang#73357 (Use `LocalDefId` for import IDs in trait map) - rust-lang#73364 (asm: Allow multiple template string arguments; interpret them as newline-separated) - rust-lang#73382 (Only display other method receiver candidates if they actually apply) - rust-lang#73465 (Add specialization of `ToString for char`) - rust-lang#73489 (Refactor hir::Place) Failed merges: r? @ghost

Manishearth · 2020-06-19T23:04:59Z

@bors r=nikomatsakis

bors · 2020-06-19T23:05:00Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Rollup of 13 pull requests #73511

bors · 2020-06-19T23:05:01Z

📌 Commit 7b63986 has been approved by nikomatsakis

bors · 2020-06-20T02:22:54Z

⌛ Testing commit 7b63986 with merge 34c5cd9...

rust-highfive assigned Mark-Simulacrum May 28, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2020

LeSeulArtichaut added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label May 28, 2020

nikomatsakis approved these changes May 28, 2020

View reviewed changes

src/liballoc/alloc.rs Outdated Show resolved Hide resolved

src/liballoc/alloc.rs Outdated Show resolved Hide resolved

src/liballoc/alloc.rs Show resolved Hide resolved

Mark-Simulacrum removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2020

LeSeulArtichaut force-pushed the unsafe-liballoc branch from d2e2890 to dd18ba7 Compare May 31, 2020 13:08

LeSeulArtichaut added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label May 31, 2020

nikomatsakis added the I-nominated label Jun 1, 2020

LeSeulArtichaut removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jun 2, 2020

LeSeulArtichaut force-pushed the unsafe-liballoc branch from dd18ba7 to 845e3eb Compare June 3, 2020 15:14

LeSeulArtichaut force-pushed the unsafe-liballoc branch 3 times, most recently from a0d9dce to 615372b Compare June 8, 2020 21:07

LeSeulArtichaut force-pushed the unsafe-liballoc branch from 615372b to 7b63986 Compare June 19, 2020 11:54

LeSeulArtichaut added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. labels Jun 19, 2020

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 Jun 19, 2020

Manishearth mentioned this pull request Jun 19, 2020

Rollup of 13 pull requests #73511

Merged

Manishearth closed this Jun 19, 2020

Manishearth reopened this Jun 19, 2020

bors merged commit 55479de into rust-lang:master Jun 20, 2020

LeSeulArtichaut deleted the unsafe-liballoc branch June 20, 2020 07:45

LeSeulArtichaut mentioned this pull request Jun 30, 2020

#![deny(unsafe_op_in_unsafe_fn)] in libstd #73904

Closed

22 tasks

Medowhill mentioned this pull request Jan 15, 2021

Using unsafe in unsafe functions kaist-cp/rv6#335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`#[deny(unsafe_op_in_unsafe_fn)]` in liballoc #72709

`#[deny(unsafe_op_in_unsafe_fn)]` in liballoc #72709

LeSeulArtichaut commented May 28, 2020 •

edited

Loading

rust-highfive commented May 28, 2020

Mark-Simulacrum commented May 28, 2020

nikomatsakis left a comment

LeSeulArtichaut commented May 30, 2020

rust-highfive commented May 31, 2020

nikomatsakis commented Jun 1, 2020

rust-highfive commented Jun 3, 2020

nikomatsakis commented Jun 8, 2020

LeSeulArtichaut commented Jun 8, 2020

LeSeulArtichaut commented Jun 19, 2020

LeSeulArtichaut commented Jun 19, 2020

nikomatsakis commented Jun 19, 2020

bors commented Jun 19, 2020

Dylan-DPC-zz commented Jun 19, 2020

bors commented Jun 19, 2020

Manishearth commented Jun 19, 2020

Manishearth commented Jun 19, 2020

bors commented Jun 19, 2020

bors commented Jun 19, 2020

bors commented Jun 20, 2020

#[deny(unsafe_op_in_unsafe_fn)] in liballoc #72709

#[deny(unsafe_op_in_unsafe_fn)] in liballoc #72709

Conversation

LeSeulArtichaut commented May 28, 2020 • edited Loading

rust-highfive commented May 28, 2020

Mark-Simulacrum commented May 28, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

LeSeulArtichaut commented May 30, 2020

rust-highfive commented May 31, 2020

nikomatsakis commented Jun 1, 2020

rust-highfive commented Jun 3, 2020

nikomatsakis commented Jun 8, 2020

LeSeulArtichaut commented Jun 8, 2020

LeSeulArtichaut commented Jun 19, 2020

LeSeulArtichaut commented Jun 19, 2020

nikomatsakis commented Jun 19, 2020

bors commented Jun 19, 2020

Dylan-DPC-zz commented Jun 19, 2020

bors commented Jun 19, 2020

Manishearth commented Jun 19, 2020

Manishearth commented Jun 19, 2020

bors commented Jun 19, 2020

bors commented Jun 19, 2020

bors commented Jun 20, 2020

`#[deny(unsafe_op_in_unsafe_fn)]` in liballoc #72709

`#[deny(unsafe_op_in_unsafe_fn)]` in liballoc #72709

LeSeulArtichaut commented May 28, 2020 •

edited

Loading