-
Notifications
You must be signed in to change notification settings - Fork 501
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
Add "Logic errors" as behavior not considered unsafe #919
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly little wording changes.
Can you also point to the Hash+Eq contract defined by Hash? https://doc.rust-lang.org/std/hash/trait.Hash.html#hash-and-eq ; That way it's not just data structures that shown, but also that trait invariants can be broken in safe code. This is important because it means that unsafe code can't rely on invariants defined by safe traits.
Indeed, which is one reason it would be great to actually list all the "logic errors" instances in core etc. somewhere; similar to the UB and unspecified lists in the C standard. Or at least always use a term ("logic error" or something else; or have a section of "Constraints" like the "Panics" one) so that it can be greppable/shown distinctively (e.g. currently the (I haven't spelled this point out since it was an example, but I can split the two examples into two paragraphs and give more detail to this one if you prefer). |
In rust-lang/rust#80657 and rust-lang/rust#80681 it is discussed how to clarify/define what a "logic error" is and what are their consequences. The reference should mention them as well. Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
I forgot to mention, please don't force push after a review. It makes it impossible to see what has changed from push to push. |
cc @RalfJung ; does this look alright? |
Yeah... GitHub should really fix that. Thanks for the review and your patience with my English! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hm... yes the text makes sense, but I am not sure about the best place for putting it. Currently we probably don't have a better place than this though. This touches on the larger story of "language UB" vs "library UB" -- there's assumptions the compiler makes about what the code does (and violating them is language UB), and there are assumptions that libraries make about what their callers do (and violating them is library UB). The "list of UB" in the reference is certainly about language UB, these logic errors are a concept related to library APIs and thus more closely related to library UB. Related to this is also the distinction of validity invariants vs safety invariants, which one could call "language invariants" and "library invariants", respectively. My concern with this change is that it might lead to even more confusion between language UB and library UB. But maybe this is still okay for now. Longer-term, the reference should more clearly tease apart these two concepts. |
It might make sense to try and put this in the API guidelines, which is closer to library and generally feels better than documenting non-language concerns in the reference. |
API guidelines sounds like a good place to me. This chapter is entirely about things that don't actually concern the language, so it doesn't technically fit the reference, but it's a good chapter to have in juxtaposition of the chapter that says what is, so I'm fine with having it here nonetheless. |
I think it'd be better to have a proper introduction of language UB vs library UB. But this is probably a longer-term project which also involves adjusting the stdlib documentation to better distinguish these two kinds of UB. |
Yes, I agree that having the distinction between the kinds of UB in the reference makes sense. I think the impacts of that on libraries (e.g. this notion of logic error / safe but unspecified behavior) might be better suited for API guidelines or similar places. |
What is considered library? Is it always a clear cut? For instance, I guess one could argue the Similarly, in C one could argue So I definitely agree it is a good idea to define these differences between kinds of UB in the reference somewhere. Perhaps the page introducing that could be the parent of the two "Behavior ..." ones. |
@ojeda The language aspect is that @RalfJung Language UB vs Library UB should definitely be documented in the Behavior Considered Unsafe chapter. That said, I think it's orthogonal to logic errors not being UB and that might be surprising, so specifically stating it is important. |
@Havvy I am not talking about what equality means, but about the constraint itself. The constraint may be spelled out in a library file now; but it could very well be that it had been spelled out in the language itself. C++ also has its fair share of magic bits that blend the line between the library and the language; even if the standard delineates what is "library" and what is "language". Anyway, my point is that I believe it is not that important whether some UB comes from the library or from the language -- defining clear terms and what kind of consequences each entails (bounded/safe UB? unbounded/unsafe UB? unspecified? ...) is more important; because those are then used to document things in code/libraries/etc. and in day-to-day conversation. |
Update books ## nomicon 2 commits in a5a48441d411f61556b57d762b03d6874afe575d..a8584998eacdea7106a1dfafcbf6c1c06fcdf925 2020-12-06 10:39:41 +0900 to 2021-01-06 12:49:49 -0500 - Update vector code examples - Remove outdated information about `jemalloc` ## reference 13 commits in b278478b766178491a8b6f67afa4bcd6b64d977a..50af691f838937c300b47812d0507c6d88c14f97 2020-12-21 18:18:03 -0800 to 2021-01-12 21:19:20 -0800 - Update grammar for parser unification. (rust-lang/reference#927) - Define constraining an implementation (rust-lang/reference#928) - Document extra behavior of #[no_mangle] (rust-lang/reference#930) - Add a float examle without a `.`. (rust-lang/reference#929) - Add more details about const generics. (rust-lang/reference#921) - Fix footnotes. (rust-lang/reference#926) - Add "Logic errors" as behavior not considered unsafe (rust-lang/reference#919) - Update grammar for order of parameters/arguments. (rust-lang/reference#920) - Fix formatting in the tuple section (rust-lang/reference#923) - document const generics (rust-lang/reference#901) - Update mdbook (rust-lang/reference#918) - linkage.md: update link to FFI section of the Book. (rust-lang/reference#917) - Document array expression with a const. (rust-lang/reference#914) ## book 8 commits in 5bb44f8b5b0aa105c8b22602e9b18800484afa21..ac57a0ddd23d173b26731ccf939f3ba729753275 2020-12-18 20:07:31 -0500 to 2021-01-09 14:18:45 -0500 - Update version of mdbook we're testing with to 0.4.5 (rust-lang/book#2561) - Fix grammar in ch13-01-closures.md (rust-lang/book#2534) - Merge remote-tracking branch 'origin/pr/2527' - Clarify code example ch6.3 (rust-lang/book#2485) - Fix link added in rust-lang/book#2495 to be relative and at the bottom - Merge remote-tracking branch 'origin/pr/2495' - Update output to match the updated poem punctuation - Fix rust-lang/book#2539 - Remove fancy apostrophes from poem for Windows ## rust-by-example 3 commits in 1cce0737d6a7d3ceafb139b4a206861fb1dcb2ab..03e23af01f0b4f83a3a513da280e1ca92587f2ec 2020-12-21 17:36:29 -0300 to 2021-01-09 10:20:28 -0300 - Replace for loop with iteration (rust-lang/rust-by-example#1404) - Update mdbook (rust-lang/rust-by-example#1402) - Add note for match guards to include catch-all (rust-lang/rust-by-example#1401) ## embedded-book 1 commits in ba34b8a968f9531d38c4dc4411d5568b7c076bfe..ceec19e873be87c6ee5666b030c6bb612f889a96 2020-11-17 00:20:43 +0000 to 2021-01-03 13:13:10 +0000 - book.toml: add link to GitHub repo (rust-embedded/book#276)
Update books ## nomicon 2 commits in a5a48441d411f61556b57d762b03d6874afe575d..a8584998eacdea7106a1dfafcbf6c1c06fcdf925 2020-12-06 10:39:41 +0900 to 2021-01-06 12:49:49 -0500 - Update vector code examples - Remove outdated information about `jemalloc` ## reference 13 commits in b278478b766178491a8b6f67afa4bcd6b64d977a..50af691f838937c300b47812d0507c6d88c14f97 2020-12-21 18:18:03 -0800 to 2021-01-12 21:19:20 -0800 - Update grammar for parser unification. (rust-lang/reference#927) - Define constraining an implementation (rust-lang/reference#928) - Document extra behavior of #[no_mangle] (rust-lang/reference#930) - Add a float examle without a `.`. (rust-lang/reference#929) - Add more details about const generics. (rust-lang/reference#921) - Fix footnotes. (rust-lang/reference#926) - Add "Logic errors" as behavior not considered unsafe (rust-lang/reference#919) - Update grammar for order of parameters/arguments. (rust-lang/reference#920) - Fix formatting in the tuple section (rust-lang/reference#923) - document const generics (rust-lang/reference#901) - Update mdbook (rust-lang/reference#918) - linkage.md: update link to FFI section of the Book. (rust-lang/reference#917) - Document array expression with a const. (rust-lang/reference#914) ## book 8 commits in 5bb44f8b5b0aa105c8b22602e9b18800484afa21..ac57a0ddd23d173b26731ccf939f3ba729753275 2020-12-18 20:07:31 -0500 to 2021-01-09 14:18:45 -0500 - Update version of mdbook we're testing with to 0.4.5 (rust-lang/book#2561) - Fix grammar in ch13-01-closures.md (rust-lang/book#2534) - Merge remote-tracking branch 'origin/pr/2527' - Clarify code example ch6.3 (rust-lang/book#2485) - Fix link added in rust-lang/book#2495 to be relative and at the bottom - Merge remote-tracking branch 'origin/pr/2495' - Update output to match the updated poem punctuation - Fix rust-lang/book#2539 - Remove fancy apostrophes from poem for Windows ## rust-by-example 3 commits in 1cce0737d6a7d3ceafb139b4a206861fb1dcb2ab..03e23af01f0b4f83a3a513da280e1ca92587f2ec 2020-12-21 17:36:29 -0300 to 2021-01-09 10:20:28 -0300 - Replace for loop with iteration (rust-lang/rust-by-example#1404) - Update mdbook (rust-lang/rust-by-example#1402) - Add note for match guards to include catch-all (rust-lang/rust-by-example#1401) ## embedded-book 1 commits in ba34b8a968f9531d38c4dc4411d5568b7c076bfe..ceec19e873be87c6ee5666b030c6bb612f889a96 2020-11-17 00:20:43 +0000 to 2021-01-03 13:13:10 +0000 - book.toml: add link to GitHub repo (rust-embedded/book#276)
Update books ## nomicon 2 commits in a5a48441d411f61556b57d762b03d6874afe575d..a8584998eacdea7106a1dfafcbf6c1c06fcdf925 2020-12-06 10:39:41 +0900 to 2021-01-06 12:49:49 -0500 - Update vector code examples - Remove outdated information about `jemalloc` ## reference 13 commits in b278478b766178491a8b6f67afa4bcd6b64d977a..50af691f838937c300b47812d0507c6d88c14f97 2020-12-21 18:18:03 -0800 to 2021-01-12 21:19:20 -0800 - Update grammar for parser unification. (rust-lang/reference#927) - Define constraining an implementation (rust-lang/reference#928) - Document extra behavior of #[no_mangle] (rust-lang/reference#930) - Add a float examle without a `.`. (rust-lang/reference#929) - Add more details about const generics. (rust-lang/reference#921) - Fix footnotes. (rust-lang/reference#926) - Add "Logic errors" as behavior not considered unsafe (rust-lang/reference#919) - Update grammar for order of parameters/arguments. (rust-lang/reference#920) - Fix formatting in the tuple section (rust-lang/reference#923) - document const generics (rust-lang/reference#901) - Update mdbook (rust-lang/reference#918) - linkage.md: update link to FFI section of the Book. (rust-lang/reference#917) - Document array expression with a const. (rust-lang/reference#914) ## book 8 commits in 5bb44f8b5b0aa105c8b22602e9b18800484afa21..ac57a0ddd23d173b26731ccf939f3ba729753275 2020-12-18 20:07:31 -0500 to 2021-01-09 14:18:45 -0500 - Update version of mdbook we're testing with to 0.4.5 (rust-lang/book#2561) - Fix grammar in ch13-01-closures.md (rust-lang/book#2534) - Merge remote-tracking branch 'origin/pr/2527' - Clarify code example ch6.3 (rust-lang/book#2485) - Fix link added in rust-lang/book#2495 to be relative and at the bottom - Merge remote-tracking branch 'origin/pr/2495' - Update output to match the updated poem punctuation - Fix rust-lang/book#2539 - Remove fancy apostrophes from poem for Windows ## rust-by-example 3 commits in 1cce0737d6a7d3ceafb139b4a206861fb1dcb2ab..03e23af01f0b4f83a3a513da280e1ca92587f2ec 2020-12-21 17:36:29 -0300 to 2021-01-09 10:20:28 -0300 - Replace for loop with iteration (rust-lang/rust-by-example#1404) - Update mdbook (rust-lang/rust-by-example#1402) - Add note for match guards to include catch-all (rust-lang/rust-by-example#1401) ## embedded-book 1 commits in ba34b8a968f9531d38c4dc4411d5568b7c076bfe..ceec19e873be87c6ee5666b030c6bb612f889a96 2020-11-17 00:20:43 +0000 to 2021-01-03 13:13:10 +0000 - book.toml: add link to GitHub repo (rust-embedded/book#276)
Update books ## nomicon 2 commits in a5a48441d411f61556b57d762b03d6874afe575d..a8584998eacdea7106a1dfafcbf6c1c06fcdf925 2020-12-06 10:39:41 +0900 to 2021-01-06 12:49:49 -0500 - Update vector code examples - Remove outdated information about `jemalloc` ## reference 13 commits in b278478b766178491a8b6f67afa4bcd6b64d977a..50af691f838937c300b47812d0507c6d88c14f97 2020-12-21 18:18:03 -0800 to 2021-01-12 21:19:20 -0800 - Update grammar for parser unification. (rust-lang/reference#927) - Define constraining an implementation (rust-lang/reference#928) - Document extra behavior of #[no_mangle] (rust-lang/reference#930) - Add a float examle without a `.`. (rust-lang/reference#929) - Add more details about const generics. (rust-lang/reference#921) - Fix footnotes. (rust-lang/reference#926) - Add "Logic errors" as behavior not considered unsafe (rust-lang/reference#919) - Update grammar for order of parameters/arguments. (rust-lang/reference#920) - Fix formatting in the tuple section (rust-lang/reference#923) - document const generics (rust-lang/reference#901) - Update mdbook (rust-lang/reference#918) - linkage.md: update link to FFI section of the Book. (rust-lang/reference#917) - Document array expression with a const. (rust-lang/reference#914) ## book 8 commits in 5bb44f8b5b0aa105c8b22602e9b18800484afa21..ac57a0ddd23d173b26731ccf939f3ba729753275 2020-12-18 20:07:31 -0500 to 2021-01-09 14:18:45 -0500 - Update version of mdbook we're testing with to 0.4.5 (rust-lang/book#2561) - Fix grammar in ch13-01-closures.md (rust-lang/book#2534) - Merge remote-tracking branch 'origin/pr/2527' - Clarify code example ch6.3 (rust-lang/book#2485) - Fix link added in rust-lang/book#2495 to be relative and at the bottom - Merge remote-tracking branch 'origin/pr/2495' - Update output to match the updated poem punctuation - Fix rust-lang/book#2539 - Remove fancy apostrophes from poem for Windows ## rust-by-example 3 commits in 1cce0737d6a7d3ceafb139b4a206861fb1dcb2ab..03e23af01f0b4f83a3a513da280e1ca92587f2ec 2020-12-21 17:36:29 -0300 to 2021-01-09 10:20:28 -0300 - Replace for loop with iteration (rust-lang/rust-by-example#1404) - Update mdbook (rust-lang/rust-by-example#1402) - Add note for match guards to include catch-all (rust-lang/rust-by-example#1401) ## embedded-book 1 commits in ba34b8a968f9531d38c4dc4411d5568b7c076bfe..ceec19e873be87c6ee5666b030c6bb612f889a96 2020-11-17 00:20:43 +0000 to 2021-01-03 13:13:10 +0000 - book.toml: add link to GitHub repo (rust-embedded/book#276)
Update books ## nomicon 2 commits in a5a48441d411f61556b57d762b03d6874afe575d..a8584998eacdea7106a1dfafcbf6c1c06fcdf925 2020-12-06 10:39:41 +0900 to 2021-01-06 12:49:49 -0500 - Update vector code examples - Remove outdated information about `jemalloc` ## reference 13 commits in b278478b766178491a8b6f67afa4bcd6b64d977a..50af691f838937c300b47812d0507c6d88c14f97 2020-12-21 18:18:03 -0800 to 2021-01-12 21:19:20 -0800 - Update grammar for parser unification. (rust-lang/reference#927) - Define constraining an implementation (rust-lang/reference#928) - Document extra behavior of #[no_mangle] (rust-lang/reference#930) - Add a float examle without a `.`. (rust-lang/reference#929) - Add more details about const generics. (rust-lang/reference#921) - Fix footnotes. (rust-lang/reference#926) - Add "Logic errors" as behavior not considered unsafe (rust-lang/reference#919) - Update grammar for order of parameters/arguments. (rust-lang/reference#920) - Fix formatting in the tuple section (rust-lang/reference#923) - document const generics (rust-lang/reference#901) - Update mdbook (rust-lang/reference#918) - linkage.md: update link to FFI section of the Book. (rust-lang/reference#917) - Document array expression with a const. (rust-lang/reference#914) ## book 8 commits in 5bb44f8b5b0aa105c8b22602e9b18800484afa21..ac57a0ddd23d173b26731ccf939f3ba729753275 2020-12-18 20:07:31 -0500 to 2021-01-09 14:18:45 -0500 - Update version of mdbook we're testing with to 0.4.5 (rust-lang/book#2561) - Fix grammar in ch13-01-closures.md (rust-lang/book#2534) - Merge remote-tracking branch 'origin/pr/2527' - Clarify code example ch6.3 (rust-lang/book#2485) - Fix link added in rust-lang/book#2495 to be relative and at the bottom - Merge remote-tracking branch 'origin/pr/2495' - Update output to match the updated poem punctuation - Fix rust-lang/book#2539 - Remove fancy apostrophes from poem for Windows ## rust-by-example 3 commits in 1cce0737d6a7d3ceafb139b4a206861fb1dcb2ab..03e23af01f0b4f83a3a513da280e1ca92587f2ec 2020-12-21 17:36:29 -0300 to 2021-01-09 10:20:28 -0300 - Replace for loop with iteration (rust-lang/rust-by-example#1404) - Update mdbook (rust-lang/rust-by-example#1402) - Add note for match guards to include catch-all (rust-lang/rust-by-example#1401) ## embedded-book 1 commits in ba34b8a968f9531d38c4dc4411d5568b7c076bfe..ceec19e873be87c6ee5666b030c6bb612f889a96 2020-11-17 00:20:43 +0000 to 2021-01-03 13:13:10 +0000 - book.toml: add link to GitHub repo (rust-embedded/book#276)
Yes, I think it is fairly clear-cut: the language is everything that an interpreter like Miri needs to implement "by hand"; everything else is a library.
Sure, but it wasn't. That's like saying "we are using Now, one could argue that we should permit other Rust implementations to make different choices here than Anyway, for
I think it might be confusing because "logic error" is a library concept, which I feel like shouldn't occur on a page dedicated to language UB. But, as I already said, I have no better place to suggest either. |
And a language can include such a constraint. In fact, one can argue we don't know if it does or doesn't, since the reference is non-normative and not complete (see below).
First of all, I was merely posing a question to try to understand the mind-frame of the writers of the reference, not arguing for or against it particular choices, precisely because I know there is this wiggle room. Second, the reference is non-normative and, most importantly, a work in progress. So no, things are never set in stone -- not even in formally standardized documents that are supposed to be normative. Furthermore, it doesn't really matter that "Rust made certain choices" when discussing the changes to the reference itself: it is a cyclic argument. Third, the key point here is where are these "certain choices" documented and how. In the introduction, the reference claims the "standard library" is not documented by it. Not only that is not clear-cut at all, it is not even defined what the "library" is supposed to be or include or not. One can claim it is "everything not in the reference", but that would require assuming the reference is complete, which it isn't (as it claims itself). One can also claim it is "everything documented in the library" too, but as said above, that requires clarification when written from the point of view of the reference. Finally, if one wants to take away something from this, it should be "this probably needs to be clarified", not "you are making a pointless/vacuous argument"; which is non-constructive.
Well, one of the points of having a reference is to make it clear what and what cannot be done by implementations, definitely; but more importantly, and this is what I was referring about, is to define terms and vocabulary that everyone can refer back to. While instances of "library UB" may be defined in the library itself, the concept of "library UB" should be in the reference. That is to say: to define e.g. "language UB" and "library UB", one needs to know where the line between language and library is -- it does not particularly matter where it is in a given language, just that it is clearly defined what is what.
Agreed. |
In rust-lang/rust#80657 and rust-lang/rust#80681 it is discussed how to clarify/define what a "logic error" is and what are their consequences. The reference should mention them as well.
We could use other term instead of "unspecified", e.g. "unpredictable"; in case we want to emphasize that the results may not the same between runs, builds, kinds of build, etc. For instance, from the description of integer overflow, it looks like the implementation needs to choose some reasonable behavior (e.g. a panic or wrap around), even if not documented (therefore, "unspecified", in terms of the C and C++ standards). However, logic errors seem to require more freedom: the implementation may not even be able (or want) to predict what may happen, so it cannot "reasonably choose" a particular behavior, even if such behavior is guaranteed to be safe (similar to deadlocks, although in that case the set of possible behavior is known, even if nondeterministic between runs).
As I mentioned in one of the linked discussions, it might be a good idea to define a separate, clear term for this kind of behavior that is considered erroneous and unpredictable, yet safe. So not exactly "unspecified", at least as used in the C and C++ standards -- it is closer to "safe UB" than "unspecified". In other words, in the same spirit as C's "bounded UB" or Ada's "bounded error". Ideally, the term would not overload another common term in other language specs (like UB).