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

Clarify what the effects of a 'logic error' are #80681

Merged
merged 1 commit into from
Jan 16, 2021

Conversation

ChrisJefferson
Copy link
Contributor

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

@rust-highfive
Copy link
Collaborator

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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 4, 2021
@ojeda
Copy link
Contributor

ojeda commented Jan 4, 2021

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.

@ChrisJefferson
Copy link
Contributor Author

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.

@Mark-Simulacrum
Copy link
Member

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 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 Jan 4, 2021
@ojeda
Copy link
Contributor

ojeda commented Jan 4, 2021

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.

@Mark-Simulacrum
Copy link
Member

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.

@ChrisJefferson
Copy link
Contributor Author

ChrisJefferson commented Jan 4, 2021

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."

@ojeda
Copy link
Contributor

ojeda commented Jan 5, 2021

Unspecified and undefined behavior are quite different AFAIK.

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 unsafe" of the reference. I can send a PR to discuss such a change separately, if you want; since it goes a bit out of scope for this PR.

(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")

I think just giving some more examples to what is in this PR (not terminating or aborting) would be fine.

Agreed! It is an improvement nevertheless.

@Mark-Simulacrum
Copy link
Member

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.

@ojeda
Copy link
Contributor

ojeda commented Jan 6, 2021

Sounds good and simple!

@Mark-Simulacrum
Copy link
Member

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.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 9, 2021

Looks good to me.

I'm not sure we can constrain it much more.

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.

@KodrAus
Copy link
Contributor

KodrAus commented Jan 14, 2021

This looks like a great description to me! I think I'd like a mention of memory leaks here too.

@ChrisJefferson
Copy link
Contributor Author

Added "memory leaks" to the list of possible issues.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 16, 2021

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 16, 2021

📌 Commit 78d9192 has been approved by m-ou-se

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 16, 2021
…-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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2021
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
@bors bors merged commit 40d2506 into rust-lang:master Jan 16, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 16, 2021
wsh pushed a commit to wsh/rust-reference that referenced this pull request Jan 17, 2021
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>
@ChrisJefferson ChrisJefferson deleted the logic-error-doc branch December 27, 2022 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants