Improvements and a new `tree!` macro #110

alexmozaidze · 2024-07-19T21:39:44Z

Made some refactors that I forgot to do in a previous PR, turned the project into a workspace and added a proc macro crate that implements the tree! macro.

Although, the macro is usable and would make testing indextree less prone to errors, I should add a lot of tests for the macro in order to be sure it works correctly 100% of the time without issues before I start sprinkling it in all tests.

I tried to keep the macro as easy-to-use and ergonomic as possible, settling at the following syntax:

tree!(
    &mut arena,
    // root_node can be any expression, if it returns `NodeId` then it is used as the root
    // node, if it returns a value of other type, then it creates a root node on-the-fly
    // and returns that root node when it's done.
    root_node => {
        "1",
        "2" => {
            "2_1" => { "2_1_1" },
            "2_2",
        },
        "3" => {}, // same as just "3"
    }
)

I will add documentation to the macro and the tests later when I have time, but for now it's a draft.

The previous solution with an impl trait excluded features that the iterator supports, only exposing the plain `Iterator` trait portion. This direct return is better since there is no functionality loss.

This allows users of the library to have an immutable view into the underlying node storage, as well as access to any functionality an immutable slice provides.

Oops, forgot to implement these earlier. Getting the siblings is ok for normal nodes, but for root nodes it doesn't really work, since root nodes don't have siblings. Thus, if a root node is detected whilst attempting to build sibling iterators, the tail will be `None`, meaning it will be an iterator of a single element - itself.

Made the project into a workspace and added a new `tree!` macro which constructs a tree from a root node and an existing arena. Also, made the macro crate be visible as `macros` module from within `indextree`.

Allowed specifying both pre-existing nodes and literal nodes. It checks the types at runtime, but in a way that could *potentially* be optimized out by the compiler. Additionally, improved ergonomics of `tree!` by making the root node part of the tree representation. (`tree!(arena, root_node, ...nodes)` => `tree!(arena, root_node => { ...nodes })`)

The macro cannot work correctly without *both* transmutes. All we can do is silence the warnings.

…mt.toml`

alexmozaidze · 2024-07-19T21:54:15Z

.rustfmt.toml has a lot of unknown or deprecated features, along with required_version that was way below of what I had on my system, not letting me format the project. Are all these options necessary? Can I remove the ones that are labeled as unknown?

This is much safer, as a `TypeId` comparing is safer than comparing strings returned by `type_name*` functions.

alexmozaidze · 2024-07-19T22:11:58Z

Also, check the .codecov.yml. I blindly moved it to the root of the workspace, which may be incorrect, but I don't know.

indextree-macros/src/lib.rs

alexmozaidze · 2024-07-22T08:35:36Z

I may be able to remove unsafe code using autoref specialization.

…ns in `tree!` Refactored the dynamic type checking logic into an autoref specialization trick. This makes the macro much more secure and predictable, as opposed to relying on inference for transmutes.

Made it less probable for external traits/identifiers to "leak" into the logic of the macro. Also, replaced `std` with `core` for portability.

alexmozaidze · 2024-07-23T22:19:47Z

I wasn't able to fully remove unsafe keyword usage, but I made it more secure using autoref specialization and ManuallyDrop::take. The previous iterations used transmute with inferred types (which was already a red flag), and type_name*, which was not reliable, since different types could have the same string representation. When Rust stabilizes specialization, all the tricks and unsafe code could be removed.

The macro is now good for general usage, the only thing left to do is to add tests and documentation for the macro, and then I can safely start refactoring tests to use tree! (as well as add tests for the new iterators).

Forgot to do that before, now it reflects the current way of doing things and why we do it.

If the last group of actions in `tree!` is going to the parent node without taking any action afterwards, then the last group of actions is removed. Also, refactored to file a little.

alexmozaidze · 2024-07-26T23:57:45Z

I will improve existing tests in a different PR. This PR is already quite bloated. Ready for review.

saschagrunert · 2024-07-29T06:43:48Z

.editorconfig

+root = true
+
+[*]
+end_of_line = lf
+insert_final_newline = true
+indent_style = space
+indent_size = 4


Do we need this file as part of this PR?

I mean, editorconfig helps with consistency on different editors (I personally like tab-indentation more, but I set it up to whatever was used in the project). I can remove it if you want.

No it should be fine.

saschagrunert · 2024-07-29T07:13:16Z

Thank you!

alexmozaidze added 9 commits June 25, 2024 13:22

fix(arena): return slice iterator instead of impl trait

175896a

The previous solution with an impl trait excluded features that the iterator supports, only exposing the plain `Iterator` trait portion. This direct return is better since there is no functionality loss.

feat(arena): expose underlying nodes as &[Node<T>]

0a9e5e4

This allows users of the library to have an immutable view into the underlying node storage, as well as access to any functionality an immutable slice provides.

refactor(traverse): use .and_then() instead of .map().flatten()

155027b

fix(id): deprecate NodeId::reverse_children

f4137c3

feat: add indextree-macros and the tree! macro

25bc593

Made the project into a workspace and added a new `tree!` macro which constructs a tree from a root node and an existing arena. Also, made the macro crate be visible as `macros` module from within `indextree`.

refactor(macros): silence clippy::useless_transmute warnings

8561907

The macro cannot work correctly without *both* transmutes. All we can do is silence the warnings.

refactor: ran cargo fmt and removed required_version from `.rustf…

bd11914

…mt.toml`

fix(macros): use TypeId::of instead of type_name* for type checking

0347466

This is much safer, as a `TypeId` comparing is safer than comparing strings returned by `type_name*` functions.

alexmozaidze commented Jul 20, 2024

View reviewed changes

indextree-macros/src/lib.rs Show resolved Hide resolved

alexmozaidze added 3 commits July 24, 2024 01:24

feat(macros): use autoref specialization instead of unsound assumptio…

f12c4fb

…ns in `tree!` Refactored the dynamic type checking logic into an autoref specialization trick. This makes the macro much more secure and predictable, as opposed to relying on inference for transmutes.

fix(macros): improve identifier sandboxing for tree!

72dca49

Made it less probable for external traits/identifiers to "leak" into the logic of the macro. Also, replaced `std` with `core` for portability.

fix(macros): allow a trailing comma in tree!

deb4c98

alexmozaidze added 6 commits July 25, 2024 22:42

feat(macros): allow omitting => {} for root node in tree!

43eb8e9

build(macros): add tests for tree!

1deb430

refactor(macros): update HACK comment for tree!

8c09d1c

Forgot to do that before, now it reflects the current way of doing things and why we do it.

fix(macros): refactor and optimize tree! macro

cb9fe98

If the last group of actions in `tree!` is going to the parent node without taking any action afterwards, then the last group of actions is removed. Also, refactored to file a little.

style: ran cargo fmt

62cb693

docs(macros): add documentation to tree!

42131c8

alexmozaidze changed the title ~~Refactors with improvements and a new tree! macro~~ Improvements and a new tree! macro Jul 26, 2024

alexmozaidze marked this pull request as ready for review July 26, 2024 23:57

saschagrunert reviewed Jul 29, 2024

View reviewed changes

saschagrunert merged commit 768615e into saschagrunert:main Jul 29, 2024
7 checks passed

alexmozaidze mentioned this pull request Jul 30, 2024

[Feature Request] Add indextree! macro to quickly construct an Arena #109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements and a new `tree!` macro #110

Improvements and a new `tree!` macro #110

alexmozaidze commented Jul 19, 2024 •

edited

Loading

alexmozaidze commented Jul 19, 2024

alexmozaidze commented Jul 19, 2024

alexmozaidze commented Jul 22, 2024

alexmozaidze commented Jul 23, 2024 •

edited

Loading

alexmozaidze commented Jul 26, 2024 •

edited

Loading

saschagrunert Jul 29, 2024

alexmozaidze Jul 29, 2024

saschagrunert Jul 29, 2024

saschagrunert commented Jul 29, 2024

Improvements and a new tree! macro #110

Improvements and a new tree! macro #110

Conversation

alexmozaidze commented Jul 19, 2024 • edited Loading

alexmozaidze commented Jul 19, 2024

alexmozaidze commented Jul 19, 2024

alexmozaidze commented Jul 22, 2024

alexmozaidze commented Jul 23, 2024 • edited Loading

alexmozaidze commented Jul 26, 2024 • edited Loading

saschagrunert Jul 29, 2024

Choose a reason for hiding this comment

alexmozaidze Jul 29, 2024

Choose a reason for hiding this comment

saschagrunert Jul 29, 2024

Choose a reason for hiding this comment

saschagrunert commented Jul 29, 2024

Improvements and a new `tree!` macro #110

Improvements and a new `tree!` macro #110

alexmozaidze commented Jul 19, 2024 •

edited

Loading

alexmozaidze commented Jul 23, 2024 •

edited

Loading

alexmozaidze commented Jul 26, 2024 •

edited

Loading