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

Don't return TaffyResult when Taffy methods can't fail. #520

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

gibbz00
Copy link

@gibbz00 gibbz00 commented Jul 20, 2023

Fixes: #519.

Affected methods:

new_leaf, compute_layout, new_leaf_with_measure, new_with_children, remove, set_measure, add_child, set_children, child_count, children, layout, set_style, style, mark_dirty, dirty.

TODO:

Reduce implicit panicking possibilities. (See: #520 (comment))

  • cargo gentest
  • Update doc comments.
  • Update RELEASES.md

Feedback wanted

As commented by @nicoburns: Many of the methods can still panic due to the frequent use of direct array indexing. But this shouldn't really be an issue since the keys themselves can't be created without them being registered by the Taffy instance, and removal takes ownership of the NodeIds. But this can unfortunately easily be bypassed by cloning them, or by creating them it from another Taffy instance.

So rather than creating checked/unchecked method equivalents, I would suggest removing these runtime panic possibilities by continuing to leverage Rust's build in memory safety and module scoping rules. This means passing borrowed NodeIds to methods that don't mutate any values, and removing the NodeId's Clone implementation etc.

@alice-i-cecile alice-i-cecile added code quality Make the code cleaner or prettier. breaking-change A change that breaks our public interface labels Jul 20, 2023
@alice-i-cecile
Copy link
Collaborator

So rather than creating checked/unchecked method equivalents, I would suggest removing these runtime panic possibilities by continuing to leverage Rust's build in memory safety and module scoping rules. This means passing borrowed NodeIds to methods that don't mutate any values, and removing the NodeId's Clone implementation etc.

I really like this, but I'd prefer to do this in a separate PR to keep this reviewable.

@gibbz00 gibbz00 force-pushed the remove_unwraps branch 2 times, most recently from 8cae1ba to 58daf29 Compare July 20, 2023 16:28
Initial commit for: DioxusLabs#519.

Affected methods:

`new_leaf`, `compute_layout`, `new_leaf_with_measure`,
`new_with_children`, `remove`, `set_measure`, `add_child`,
`set_children`, `child_count`, `children`, `layout`, `set_style`,
`style`, `mark_dirty`, `dirty`.

Should be noted that many of the methods can still panic due to the
frequent use of direct array indexing.
@gibbz00 gibbz00 marked this pull request as ready for review July 20, 2023 16:31
@nicoburns
Copy link
Collaborator

I don’t think removing Clone from NodeId is going to work. We use that internally to store NodeId’s during layout (currently we also rely on NodeId being Copy, but that isn’t strictly necessary). And one would always be able to obtain duplicate NodeId’s by for example calling .parent() twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A change that breaks our public interface code quality Make the code cleaner or prettier.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't return TaffyResult when Taffy methods can't fail.
3 participants