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.

Sign up for GitHub

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

Jump to bottom

Panic on semantic error, rather than returning error? #29

Closed
lo48576 opened this issue Jul 31, 2019 · 3 comments · Fixed by #31
Closed

Panic on semantic error, rather than returning error? #29

lo48576 opened this issue Jul 31, 2019 · 3 comments · Fixed by #31
Labels
bug

Comments

@lo48576
Copy link
Contributor

lo48576 commented Jul 31, 2019

It seems indextree avoids panicking, even if there are semantic error (caused by broken inconsistent data).

For example:

indextree/src/lib.rs

Lines 460 to 471 in 0e51295

if let Some((self_borrow, new_child_borrow)) =
arena.nodes.get_tuple_mut(self.index0(), new_child.index0())
{
new_child_borrow.parent = Some(self);
last_child_opt =
mem::replace(&mut self_borrow.last_child, Some(new_child));
if let Some(last_child) = last_child_opt {
new_child_borrow.previous_sibling = Some(last_child);
} else {
if self_borrow.first_child.is_some() {
bail!(NodeError::FirstChildAlreadySet);
}

In this code, if self_borrow.last_child was None, self_borrow.first_child should be also None, if indextree is working correctly.
If it is not, then the data can (and may) have been broken in unexpected way, and it seems meaningless to do more operation on that broken data.
Additionally, results are returned as Fallible<_> and it does type erasure, so users might easily miss such semantic bug.

I think semantic errors should be caught by panicking (typically .expect() on Result and Option) and assert macros.
@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label bug to this issue, with a confidence of 0.87. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@issue-label-bot issue-label-bot bot added the bug label Jul 31, 2019
@saschagrunert
Copy link
Owner

Hey @lo48576 👋 thanks for the issue report. Do you mind proposing a change as PR? :)

@lo48576
Copy link
Contributor Author

lo48576 commented Jul 31, 2019

I'll try to write a PR.

lo48576 added a commit to lo48576/indextree that referenced this issue Jul 31, 2019
@lo48576
Use asserts instead of returning errors on semantic errors
2000f8e 
When the functions noticed inconsistent data, they are broken in
unexpected way and nothing meaningful can be done anymore.
In such cases, the library should panic and all following operations
should be cancelled.

Additionally, now `NodeId::prepend()` works same as `append()` when the
parent node has no children.
(Previously, it is not possible to `prepend()` to the parent node which
has no children yet.)
By this change, their behavior become symmetric and `prepend()` requires
fewer preconditions.

Fixed saschagrunert#29 and saschagrunert#30.
lo48576 added a commit to lo48576/indextree that referenced this issue Jul 31, 2019
@lo48576
Use asserts instead of returning errors on semantic errors
7bea6c4 
When the functions noticed inconsistent data, they are broken in
unexpected way and nothing meaningful can be done anymore.
In such cases, the library should panic and all following operations
should be cancelled.

Additionally, now `NodeId::prepend()` works same as `append()` when the
parent node has no children.
(Previously, it is not possible to `prepend()` to the parent node which
has no children yet.)
By this change, their behavior become symmetric and `prepend()` requires
fewer preconditions.

Fixes saschagrunert#29 and saschagrunert#30.
@lo48576 lo48576 mentioned this issue Jul 31, 2019
Merged
lo48576 added a commit to lo48576/indextree that referenced this issue Jul 31, 2019
@lo48576
Use asserts instead of returning errors on semantic errors
b11bd01 
When the functions noticed inconsistent data, they are broken in
unexpected way and nothing meaningful can be done anymore.
In such cases, the library should panic and all following operations
should be cancelled.

Additionally, now `NodeId::prepend()` works same as `append()` when the
parent node has no children.
(Previously, it is not possible to `prepend()` to the parent node which
has no children yet.)
By this change, their behavior become symmetric and `prepend()` requires
fewer preconditions.

Fixes saschagrunert#29 and saschagrunert#30.
saschagrunert pushed a commit that referenced this issue Jul 31, 2019
@lo48576 @saschagrunert
Use asserts instead of returning errors on semantic errors (#31)
cb5e48c 
When the functions noticed inconsistent data, they are broken in
unexpected way and nothing meaningful can be done anymore.
In such cases, the library should panic and all following operations
should be cancelled.

Additionally, now `NodeId::prepend()` works same as `append()` when the
parent node has no children.
(Previously, it is not possible to `prepend()` to the parent node which
has no children yet.)
By this change, their behavior become symmetric and `prepend()` requires
fewer preconditions.

Fixes #29 and #30.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Panic on semantic error, instead of returning error lo48576/indextree
2 participants
@saschagrunert @lo48576