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

HasNext() advances iterators position #46

Closed
MadsRC opened this issue Dec 14, 2024 · 4 comments · Fixed by #47
Closed

HasNext() advances iterators position #46

MadsRC opened this issue Dec 14, 2024 · 4 comments · Fixed by #47
Assignees
Labels

Comments

@MadsRC
Copy link
Contributor

MadsRC commented Dec 14, 2024

Writing integration testing for a code-base that uses this wonderful project, I ran across behavior that runs counter to what the documentation says. I am unsure if this is "working as intended" (in which case the docs most likely wrong) or if it is bug.

The documentation for the Iterator interfaces clearly states that calling Next() advances the iterators position. While the HasNext() documentation states that one should be careful to call HasNext() before calling Next(), I would still argue that the wording of the docs for Next() indicates that one should not expect HasNext() to advance the position of the iterator - otherwise you'd advance the iterator twice when calling HasNext() followed by Next().

The following piece of code reproduces the issue:

package main

import (
	"fmt"

	art "github.com/plar/go-adaptive-radix-tree/v2"
)

func main() {
	// Initialize a new Adaptive Radix Tree
	tree := art.New()

	// Insert key-value pairs into the tree
	tree.Insert(art.Key("apple"), "A sweet red fruit")
	tree.Insert(art.Key("banana"), "A long yellow fruit")
	tree.Insert(art.Key("cherry"), "A small red fruit")
	tree.Insert(art.Key("date"), "A sweet brown fruit")

	iter := tree.Iterator()
	_ = iter.HasNext()
	_ = iter.HasNext()
	_ = iter.HasNext()
	_ = iter.HasNext()
	n, _ := iter.Next()
	fmt.Printf("%+v\n", string(n.Key()))
}

The output of the above is date.

Furthermore, calling Next() does not seem to actually advance the pointer as evident by the following piece of code:

package main

import (
	"fmt"

	art "github.com/plar/go-adaptive-radix-tree/v2"
)

func main() {
	// Initialize a new Adaptive Radix Tree
	tree := art.New()

	// Insert key-value pairs into the tree
	tree.Insert(art.Key("apple"), "A sweet red fruit")
	tree.Insert(art.Key("banana"), "A long yellow fruit")
	tree.Insert(art.Key("cherry"), "A small red fruit")
	tree.Insert(art.Key("date"), "A sweet brown fruit")

	iter := tree.Iterator()
	n, _ := iter.Next()
	fmt.Printf("%+v\n", n)
	n, _ = iter.Next()
	fmt.Printf("%+v\n", n)
	_ = iter.HasNext()
	n, _ = iter.Next()
	fmt.Printf("%+v\n", string(n.Key()))
	n, _ = iter.Next()
	fmt.Printf("%+v\n", string(n.Key()))
}

The output of this block is:

<nil>
<nil>
apple
apple
@plar
Copy link
Owner

plar commented Dec 15, 2024

Hmm, do you mind to check it with v1? Just remove v2 from the import. It looks like a regression, I can't check it right now but I will do it when I have access to my computer.

@plar plar added the bug label Dec 16, 2024
@plar plar self-assigned this Dec 16, 2024
@plar
Copy link
Owner

plar commented Dec 16, 2024

@MadsRC Thanks for catching this! You’re right: HasNext() shouldn’t advance the iterator, and I agree it was misleading.

The code works fine if you use it like this:

it := tree.Iterator()
for it.HasNext() {
    leaf, _ := it.Next()
    // do something
}

But relying on that behavior isn’t clean, and HasNext() SHOULD BE idempotent without side effects.
I’ve fixed it to align with the docs, and now calling HasNext() multiple times won’t advanced the iterator. See #47.

I also added tests to make sure this won’t break again.

Appreciate the detailed report! Let me know if you hit anything else.

@plar
Copy link
Owner

plar commented Dec 16, 2024

@MadsRC v2.0.3 has been released.

@plar plar linked a pull request Dec 16, 2024 that will close this issue
@plar plar closed this as completed Dec 16, 2024
@MadsRC
Copy link
Contributor Author

MadsRC commented Dec 17, 2024

@plar thank you, that was a quick turnaround ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants