Clarify what the effects of a 'logic error' are #80681
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
Could we have the definition somewhere else (like in the reference) and then link to it? It could be a useful term to have for this kind of "safe UB", even for external libraries and crates. |
|
That sounds very sensible. It is beyond my rustdoc skills to do it, so I would be very happy if someone else wanted to do that. |
|
I don't think we should block this PR on a canonical location, and I'm unsure that there's a good place to put such a definition. I'd want to duplicate it almost everywhere instead, I think, even if we had it. That said, I think I would prefer language closer to "any defined behavior" - we can also abort, for example, or never return and both would be somewhat reasonable (though of course not great). We could also say that the behavior is unspecified, and then give some examples. |
Mark-Simulacrum
added
S-waiting-on-author
|
Agreed, given it is not a common concept to see, it is a good idea to give a brief explanation inline. Still, I think it is best to have a definition somewhere behind a clickable link. Regarding the wording, I believe unspecified behavior would work as long as we bound somehow what that behavior is, i.e. "the set of all possible behaviors that are not UB". Otherwise, it would make it pretty much UB on itself. I think using "any defined behavior" would require defining that term too, to avoid confusion. Of course, these are orthogonal improvements. |
|
Unspecified and undefined behavior are quite different AFAIK. I think just giving some more examples to what is in this PR (not terminating or aborting) would be fine. |
|
how about "Such logic errors can lead to incorrect behaviour, including but not limited to wrong results, infinite loops or panics. Logic errors will not cause undefined behaviour." |
They are different, but if one has completely unbounded unspecified behavior, then in practice it is the same as saying it is UB, except that one thinks the program is still working as expected. That is why unspecified behavior typically comes with some restricted set of useful, reasonable choices in the C and C++ standards. In C and C++, similar circumstances (like this problem with associative containers) are regarded as UB. The key is that Rust can guarantee that no "actual UB" (in the sense of "safe", i.e. the list in "Behavior considered undefined") happens, yet it is not expected behavior and should be fixed by the programmer (and ideally one could assert it in debug, too). Therefore, I guess we could list this under "Behavior not considered (As a more general point, I think it would be great if Rust could clearly define a term for this "safe UB"; possibly avoiding to rely on the "UB" term of the C and C++ standards altogether; since it is a bit overloaded at this point, even in those standards; in a similar spirit to C's "bounded UB" or Ada's "bounded error")
Agreed! It is an improvement nevertheless. |
|
Yeah, sure, that makes sense. I think I'd say something like: The behavior resulting from such a logic error is not specified, but will not result in undefined behavior. This could include panics, incorrect results, aborts, and non-termination. |
|
Sounds good and simple! |
ChrisJefferson
force-pushed
the
logic-error-doc
branch
from
79a23ed
to
3bc8394
Compare
|
I'd like to get someone from @rust-lang/libs to sign off on this, so randomly r? @m-ou-se -- this takes a rather broad approach to what the allowed set of behaviors is, but I'm not sure we can constrain it much more. |
|
Looks good to me.
Even if we could, I'm not sure if we should. It might make sense to also mention (memory) leaks, although the list doesn't have to be complete of course. r=me either way. |
|
This looks like a great description to me! I think I'd like a mention of memory leaks here too. |
ChrisJefferson
force-pushed
the
logic-error-doc
branch
from
3bc8394
to
78d9192
Compare
|
Added "memory leaks" to the list of possible issues. |
|
Thanks! @bors r+ rollup |
|
📌 Commit 78d9192 has been approved by |
bors
added
S-waiting-on-bors
…-ou-se Clarify what the effects of a 'logic error' are This clarifies what a 'logic error' is (which is a term used to describe what happens if you put things in a hash table or btree and then use something like a refcell to break the internal ordering). This tries to be as vague as possible, as we don't really want to promise what happens, except "bad things, but not UB". This was discussed in rust-lang#80657
Rollup of 17 pull requests Successful merges: - rust-lang#78455 (Introduce {Ref, RefMut}::try_map for optional projections in RefCell) - rust-lang#80144 (Remove giant badge in README) - rust-lang#80614 (Explain why borrows can't be held across yield point in async blocks) - rust-lang#80670 (TrustedRandomAaccess specialization composes incorrectly for nested iter::Zips) - rust-lang#80681 (Clarify what the effects of a 'logic error' are) - rust-lang#80764 (Re-stabilize Weak::as_ptr and friends for unsized T) - rust-lang#80901 (Make `x.py --color always` apply to logging too) - rust-lang#80902 (Add a regression test for rust-lang#76281) - rust-lang#80941 (Do not suggest invalid code in pattern with loop) - rust-lang#80968 (Stabilize the poll_map feature) - rust-lang#80971 (Put all feature gate tests under `feature-gates/`) - rust-lang#81021 (Remove doctree::Import) - rust-lang#81040 (doctest: Reset errors before dropping the parse session) - rust-lang#81060 (Add a regression test for rust-lang#50041) - rust-lang#81065 (codegen_cranelift: Fix redundant semicolon warn) - rust-lang#81069 (Add sample code for Rc::new_cyclic) - rust-lang#81081 (Add test for rust-lang#34792) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
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>
This clarifies what a 'logic error' is (which is a term used to describe what happens if you put things in a hash table or btree and then use something like a refcell to break the internal ordering). This tries to be as vague as possible, as we don't really want to promise what happens, except "bad things, but not UB". This was discussed in #80657