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 keyword docs less confusing #1379

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Make unsafe keyword docs less confusing #1379

merged 2 commits into from
Jul 19, 2023

Conversation

gregschmit
Copy link
Contributor

This sentence stuck out to me as odd (emphasis mine):

...or the unsafe block could be inside an unsafe fn and use its own proof obligations to discharge the proof obligations of its callees.

I think it means to say that it would use its own proof obligations to discharge the proof obligations of its callers, because it would be the calling functions ("callers") that normally have the proof obligation that the callee has discharged by using an unsafe block.

Am I missing something?

@@ -29,7 +29,7 @@ By putting operations into an unsafe block, the programmer states that they have
Unsafe blocks are the logical dual to unsafe functions:
where unsafe functions define a proof obligation that callers must uphold, unsafe blocks state that all relevant proof obligations have been discharged.
There are many ways to discharge proof obligations;
for example, there could be run-time checks or data structure invariants that guarantee that certain properties are definitely true, or the unsafe block could be inside an `unsafe fn` and use its own proof obligations to discharge the proof obligations of its callees.
for example, there could be run-time checks or data structure invariants that guarantee that certain properties are definitely true, or the unsafe block could be inside an `unsafe fn` and use its own proof obligations to discharge the proof obligations of its callers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yea, the use of "callee" doesn't seem right, at least with my understanding in a caller/callee relationship of a function, the callee is the function itself.

However, I've had to re-read this sentence about a dozen times to try to make sure it is clear in my head and I'm thinking some of my struggle is the use of "its" twice. That makes me need to think a little harder to understand which "it" is being referred to, and the relationship.

I'm wondering, does the following seem clearer to you?

Suggested change
for example, there could be run-time checks or data structure invariants that guarantee that certain properties are definitely true, or the unsafe block could be inside an `unsafe fn` and use its own proof obligations to discharge the proof obligations of its callers.
for example, there could be run-time checks or data structure invariants that guarantee that certain properties are definitely true, or the unsafe block could be inside an `unsafe fn` and the unsafe block can use the safety requirements the unsafe function imposes on its callers to discharge the proof obligations of the unsafe block.

Copy link
Contributor Author

@gregschmit gregschmit Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally sure that's better, because the last part references ...to discharge the proof obligations of the unsafe block, when really I believe the sentence is trying to address the discharge of the proof obligations of the callers, and saying that the unsafe block discharges their proof obligations. In other words, normally you have (with an unsafe fn):

  1. Safety requirements of unsafe fn, which normally are met by the:
  2. Proof obligation of the caller(s)

In the case of using an unsafe block inside an unsafe fn we have an additional:

  1. Proof obligation of the unsafe block, which discharges the proof obligation of the callers (2).

Also, the unsafe block would not be using the safety requirements the unsafe function imposes on its callers; rather, it's using its own proof obligations.

We could replace a couple of the "its" like this:

Suggested change
for example, there could be run-time checks or data structure invariants that guarantee that certain properties are definitely true, or the unsafe block could be inside an `unsafe fn` and use its own proof obligations to discharge the proof obligations of its callers.
for example, there could be run-time checks or data structure invariants that guarantee that certain properties are definitely true, or the unsafe block could be inside an `unsafe fn` and use the block's own proof obligations to discharge the proof obligations of the `unsafe fn`'s callers.

@mattheww
Copy link
Contributor

I think this paragraph is talking about the case where the unsafe block contains calls to unsafe functions, and when it says "callees" it's talking about those unsafe functions.

If that's right, "callees" was correct but "its own proof obligations" should instead have referred to the block's parent function's proof obligations.

I suggest writing something like

"the unsafe block could be inside an unsafe fn, in which case the block can use the proof obligations of that function to discharge the proof obligations of any unsafe functions called in the block."

@ehuss
Copy link
Contributor

ehuss commented Jul 10, 2023

Oh, that does seem to make more sense to me. FWIW, I am not well-versed in formal methods.

cc @RalfJung in case you have a chance to clarify. Does @mattheww's interpretation seem correct?

@RalfJung
Copy link
Member

RalfJung commented Jul 10, 2023

I think it means to say that it would use its own proof obligations to discharge the proof obligations of its callers, because it would be the calling functions ("callers") that normally have the proof obligation that the callee has discharged by using an unsafe block.

How could a block discharge it's callers proof obligations? I don't understand what that would even mean. callers discharge callee's proof obligations, not vice versa. (At least when talking about preconditions, which are usually the proof obligations we talk about here.)

get_unchecked has a proof obligation: the index must be in-bounds. When you write an unsafe fn like

/// Unsafe precondition: `idx < 4`
unsafe fn silly_example(slice: &[u8], idx: usize) {
  assert!(slice.len() >= 4);
  unsafe {
    slice.get_unchecked(idx);
  }
}

then you are using the idx < 4 (the proof obligation of the surrounding unsafe fn) to discharge the "idx in bounds" (the proof obligation of the callee).

What @mattheww wrote sounds good.

src/unsafe-keyword.md Outdated Show resolved Hide resolved
@gregschmit
Copy link
Contributor Author

I think it means to say that it would use its own proof obligations to discharge the proof obligations of its callers, because it would be the calling functions ("callers") that normally have the proof obligation that the callee has discharged by using an unsafe block.

How could a block discharge it's callers proof obligations? I don't understand what that would even mean. callers discharge callee's proof obligations, not vice versa. (At least when talking about preconditions, which are usually the proof obligations we talk about here.)

get_unchecked has a proof obligation: the index must be in-bounds. When you write an unsafe fn like

/// Unsafe precondition: `idx < 4`
unsafe fn silly_example(slice: &[u8], idx: usize) {
  assert!(slice.len() >= 4);
  unsafe {
    slice.get_unchecked(idx);
  }
}

then you are using the idx < 4 (the proof obligation of the surrounding unsafe fn) to discharge the "idx in bounds" (the proof obligation of the callee).

What @mattheww wrote sounds good.

Ohh! Then in that case I totally misunderstood the sentence. I was mistaken, thinking it was trying to communicate that the unsafe block within the unsafe fn was discharging the proof obligations of the callers. Yeah your last edit makes it clearer IMO.

src/unsafe-keyword.md Outdated Show resolved Hide resolved
Co-authored-by: Ralf Jung <post@ralfj.de>
src/unsafe-keyword.md Outdated Show resolved Hide resolved
Co-authored-by: Ralf Jung <post@ralfj.de>
@RalfJung
Copy link
Member

Looking good, thanks for bringing this up and working with us on a less confusing wording!

@bors r+ rollup

@RalfJung RalfJung changed the title Change reference to 'callees' to 'callers'. Make unsafe keyword docs less confusing Jul 19, 2023
@RalfJung
Copy link
Member

... oh, this is the reference. Let's wait for @ehuss to merge this then. :)

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you everyone!

@ehuss ehuss added this pull request to the merge queue Jul 19, 2023
Merged via the queue into rust-lang:master with commit fd8abed Jul 19, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 1, 2023
Update books

## rust-lang/reference

7 commits in 1ea0178266b3f3f613b0fabdaf16a83961c99cdb..9cd5c5a6ccbd4c07c65ab5c69a53286280308c95
2023-07-29 22:29:51 UTC to 2023-07-16 20:12:46 UTC

- Fix merge queue building twice. (rust-lang/reference#1383)
- Clarify UB around immutability & mutation (rust-lang/reference#1385)
- mention the extra const UB (rust-lang/reference#1273)
- Operator expressions: make the note about division by zero clearer. (rust-lang/reference#1384)
- Make unsafe keyword docs less confusing (rust-lang/reference#1379)
- Say that division by zero for primitive types panics (rust-lang/reference#1382)
- Add CI trigger for merge queues. (rust-lang/reference#1381)

## rust-lang/rust-by-example

3 commits in 8a87926a985ce32ca1fad1be4008ee161a0b91eb..07e0df2f006e59d171c6bf3cafa9d61dbeb520d8
2023-07-24 11:37:55 UTC to 2023-07-24 11:35:36 UTC

- Added attribute unused_labels - fixed warning. (rust-lang/rust-by-example#1729)
- more explanation about panic (rust-lang/rust-by-example#1728)
- chore: add the portuguese version of this project to `readme.md` (rust-lang/rust-by-example#1727)

## rust-lang/rustc-dev-guide

26 commits in b5a12d95e32ae53791cc6ab44417774667ed2ac6..24eebb6df96d037aad285e4f7793bd0393a66878
2023-07-30 11:23:23 UTC to 2023-07-11 06:02:34 UTC

- fix(name-resolution): remove unnecessary closing paranthesis (rust-lang/rustc-dev-guide#1760)
- fix(macro-expansion.md): fix the article `an` to `a` (rust-lang/rustc-dev-guide#1759)
- fix(serialization.md): fix the name of a derive macro (rust-lang/rustc-dev-guide#1756)
- fix(serialization.md): add a necessary plural suffix (rust-lang/rustc-dev-guide#1757)
- fix(salsa.md): add punctuation to prevent confusion (rust-lang/rustc-dev-guide#1754)
- fix(salsa.md): remove duplicate "To Be" verb (rust-lang/rustc-dev-guide#1755)
- feat(fuzzing.md): make `halfempty` word a link (rust-lang/rustc-dev-guide#1750)
- fix(about.md): use `a` instead of `an` (rust-lang/rustc-dev-guide#1751)
- refactor(git.md): make git-scm links clickable (rust-lang/rustc-dev-guide#1747)
- fix(walkthrough.md) add a comma operator to eliminate ambiguity (rust-lang/rustc-dev-guide#1749)
- fix(git.md): remove a confusing end of sentence character (rust-lang/rustc-dev-guide#1748)
- refactor(profiling/with_perf): remove a wrong to be verb (rust-lang/rustc-dev-guide#1746)
- refactor(tests/headers): remove duplicate list item (rust-lang/rustc-dev-guide#1745)
- refactor(test/headers.md): make the meaning more obvious (rust-lang/rustc-dev-guide#1744)
- refactor(tests/ui): remove unnecessary duplicate word (rust-lang/rustc-dev-guide#1743)
- refactor(compiletest): remove unnecessary duplicate word (rust-lang/rustc-dev-guide#1742)
- generic_arguments.md: substs -> GenericArgs (rust-lang/rustc-dev-guide#1741)
- fix(suggested): remove an unnecessary and confusing statement (rust-lang/rustc-dev-guide#1739)
- fix(how-to-build-and-run): fix a typo ("fromer" -> "former") (rust-lang/rustc-dev-guide#1736)
- fix(how-to-build-and-run): remove a wrong paragraph (rust-lang/rustc-dev-guide#1735)
- coverage code has moved (rust-lang/rustc-dev-guide#1728)
- linked issue is closed (rust-lang/rustc-dev-guide#1729)
- remove duplicate reference in about-this-guide.md (rust-lang/rustc-dev-guide#1734)
- Explain more in depth what early and late bound generic parameters are (rust-lang/rustc-dev-guide#1732)
- add section for normalization with the new solver (rust-lang/rustc-dev-guide#1731)
- Improve cleanup-crew.md with an example post (rust-lang/rustc-dev-guide#1730)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 14, 2023
Update books

## rust-lang/book

3 commits in 668c64760b5c7ea654facb4ba5fe9faddfda27cc..72187f5cd0beaaa9c6f584156bcd88f921871e83
2023-08-04 14:42:07 UTC to 2023-08-03 13:36:44 UTC

- redirects: change link for the `#![no_std]` tutorial (rust-lang/book#3705)
- [chpt10.2] - Small wording changes (rust-lang/book#3724)
- Improve sentence (rust-lang/book#3725)

## rust-embedded/book

1 commits in 1e5556dd1b864109985d5871616ae6b9164bcead..99ad2847b865e96d8ae7b333d3ee96963557e621
2023-08-11 06:31:04 UTC to 2023-08-11 06:31:04 UTC

- Fix a small typo in qemu.md (rust-embedded/book#359)

## rust-lang/nomicon

1 commits in 302b995bcb24b70fd883980fd174738c3a10b705..388750b081c0893c275044d37203f97709e058ba
2023-08-10 21:15:21 UTC to 2023-08-10 21:15:21 UTC

- Document thiscall abi (rust-lang/nomicon#311)

## rust-lang/reference

10 commits in 1ea0178266b3f3f613b0fabdaf16a83961c99cdb..d43038932adeb16ada80e206d4c073d851298101
2023-08-12 19:07:28 UTC to 2023-07-16 20:12:46 UTC

- Document thiscall abi (rust-lang/reference#1092)
- add section about implied bounds (rust-lang/reference#1261)
- Clearly specify the `instruction_set` effects (rust-lang/reference#1307)
- Fix merge queue building twice. (rust-lang/reference#1383)
- Clarify UB around immutability & mutation (rust-lang/reference#1385)
- mention the extra const UB (rust-lang/reference#1273)
- Operator expressions: make the note about division by zero clearer. (rust-lang/reference#1384)
- Make unsafe keyword docs less confusing (rust-lang/reference#1379)
- Say that division by zero for primitive types panics (rust-lang/reference#1382)
- Add CI trigger for merge queues. (rust-lang/reference#1381)

## rust-lang/rust-by-example

3 commits in 8a87926a985ce32ca1fad1be4008ee161a0b91eb..07e0df2f006e59d171c6bf3cafa9d61dbeb520d8
2023-07-24 11:37:55 UTC to 2023-07-24 11:35:36 UTC

- Added attribute unused_labels - fixed warning. (rust-lang/rust-by-example#1729)
- more explanation about panic (rust-lang/rust-by-example#1728)
- chore: add the portuguese version of this project to `readme.md` (rust-lang/rust-by-example#1727)

## rust-lang/rustc-dev-guide

31 commits in b5a12d95e32ae53791cc6ab44417774667ed2ac6..b123ab4754127d822ffb38349ce0fbf561f1b2fd
2023-08-14 08:34:59 UTC to 2023-07-11 06:02:34 UTC

- fix: stabilize debugger_visualizer (rust-lang/rustc-dev-guide#1766)
- feat(part-5-intro): make "Part 5" obvious (rust-lang/rustc-dev-guide#1753)
- Update from `#[warn_]` to `#[warning]` diagnostic attributes (rust-lang/rustc-dev-guide#1765)
- Add RPITIT documentation (rust-lang/rustc-dev-guide#1764)
- fix(visitor.md): fix a type name in a code sample (rust-lang/rustc-dev-guide#1762)
- fix(name-resolution): remove unnecessary closing paranthesis (rust-lang/rustc-dev-guide#1760)
- fix(macro-expansion.md): fix the article `an` to `a` (rust-lang/rustc-dev-guide#1759)
- fix(serialization.md): fix the name of a derive macro (rust-lang/rustc-dev-guide#1756)
- fix(serialization.md): add a necessary plural suffix (rust-lang/rustc-dev-guide#1757)
- fix(salsa.md): add punctuation to prevent confusion (rust-lang/rustc-dev-guide#1754)
- fix(salsa.md): remove duplicate "To Be" verb (rust-lang/rustc-dev-guide#1755)
- feat(fuzzing.md): make `halfempty` word a link (rust-lang/rustc-dev-guide#1750)
- fix(about.md): use `a` instead of `an` (rust-lang/rustc-dev-guide#1751)
- refactor(git.md): make git-scm links clickable (rust-lang/rustc-dev-guide#1747)
- fix(walkthrough.md) add a comma operator to eliminate ambiguity (rust-lang/rustc-dev-guide#1749)
- fix(git.md): remove a confusing end of sentence character (rust-lang/rustc-dev-guide#1748)
- refactor(profiling/with_perf): remove a wrong to be verb (rust-lang/rustc-dev-guide#1746)
- refactor(tests/headers): remove duplicate list item (rust-lang/rustc-dev-guide#1745)
- refactor(test/headers.md): make the meaning more obvious (rust-lang/rustc-dev-guide#1744)
- refactor(tests/ui): remove unnecessary duplicate word (rust-lang/rustc-dev-guide#1743)
- refactor(compiletest): remove unnecessary duplicate word (rust-lang/rustc-dev-guide#1742)
- generic_arguments.md: substs -> GenericArgs (rust-lang/rustc-dev-guide#1741)
- fix(suggested): remove an unnecessary and confusing statement (rust-lang/rustc-dev-guide#1739)
- fix(how-to-build-and-run): fix a typo ("fromer" -> "former") (rust-lang/rustc-dev-guide#1736)
- fix(how-to-build-and-run): remove a wrong paragraph (rust-lang/rustc-dev-guide#1735)
- coverage code has moved (rust-lang/rustc-dev-guide#1728)
- linked issue is closed (rust-lang/rustc-dev-guide#1729)
- remove duplicate reference in about-this-guide.md (rust-lang/rustc-dev-guide#1734)
- Explain more in depth what early and late bound generic parameters are (rust-lang/rustc-dev-guide#1732)
- add section for normalization with the new solver (rust-lang/rustc-dev-guide#1731)
- Improve cleanup-crew.md with an example post (rust-lang/rustc-dev-guide#1730)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants