Skip to content

Conversation

jayconrod
Copy link
Contributor

What type of PR is this?

Feature

What package or component does this PR mostly affect?

pathtools

What does this PR do? Why is it needed?

In the fix for #2132, I'll need to iterate over the prefixes of a given
slash-separated path. Rather than duplicate the logic from Walk2,
let's extract it into pathtools.

Prefixes returns an iterator over prefixes to avoid needing to allocate
a slice. The code should be much simpler after we adopt Go 1.23 and can
use range-over-iterator loops.

Which issues(s) does this PR fix?

Fixes #2132

Other notes for review

In the fix for bazel-contrib#2132, I'll need to iterate over the prefixes of a given
slash-separated path. Rather than duplicate the logic from Walk2,
let's extract it into pathtools.

Prefixes returns an iterator over prefixes to avoid needing to allocate
a slice. The code should be much simpler after we adopt Go 1.23 and can
use range-over-iterator loops.

For bazel-contrib#2132
@jbedard
Copy link
Contributor

jbedard commented Jul 25, 2025

Go 1.23 is needed before we can use iter.Seq?

@jbedard
Copy link
Contributor

jbedard commented Jul 25, 2025

You could also use channels instead 🤷

I've avoided using iterators/yield in a few places where I've noticed they add a lot more overhead then channels.

@jayconrod
Copy link
Contributor Author

Go 1.23 is needed before we can use iter.Seq?

Right, it's a new language feature. Older compilers don't support it.

You could also use channels instead 🤷
I've avoided using iterators/yield in a few places where I've noticed they add a lot more overhead then channels.

I'd be really surprised if that were true. Do you have an example with a benchmark?

Channels are synchronization primitives. Many operations on them involve flushing parts of the cache and context switching. They should not be used for iteration.

Iterators are not a zero cost abstraction, but they're implemented with stackless coroutines which are pretty cheap. I think they're usually a good choice when they help avoid allocating a slice or a map like this.

@jbedard
Copy link
Contributor

jbedard commented Jul 25, 2025

I'd be really surprised if that were true. Do you have an example with a benchmark?

I can't reproduce it atm, so maybe I'm wrong or misremembering? Using sequences sure is a lot nicer when analyzing profiles like what I'm staring at right now though. Lets ignore my original comment for now...

@jayconrod jayconrod enabled auto-merge (squash) July 28, 2025 21:19
@jayconrod jayconrod merged commit 6803436 into bazel-contrib:master Jul 28, 2025
14 checks passed
@jayconrod
Copy link
Contributor Author

Should we document this behavior on the function?

I accidentally pushed this onto #2153 instead. About to merge that, so we'll end up in the same place.

@jayconrod jayconrod deleted the path-prefixes branch July 28, 2025 21:58
jayconrod added a commit that referenced this pull request Jul 28, 2025
…2153)

**What type of PR is this?**

> Bug fix

**What package or component does this PR mostly affect?**

> walk

**What does this PR do? Why is it needed?**

GetDirInfo was intended to help expand glob expressions in
resolve.Resolver.Imports implementations. globs may cover
subdirectories,
especially when gazelle:generate_mode is used, so GetDirInfo must
be able to read contents of subdirectories that may not have been
visited yet.

Previously, the contract of GetDirInfo did not allow visiting
directories that hadn't been visited yet, but it would still sometimes
succeed walker.populateCache won the race. This change fully
allows GetDirInfo to be called in directories that haven't been
visited yet.

**Which issues(s) does this PR fix?**

Fixes #2132

**Other notes for review**

This PR is based on #2152. I tried to split them for easier review, but
GitHub doesn't let me set the base to a branch in a forked repo, so I'm
sorry if this makes things more difficult.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

resolver.Imports method needs RegularFiles list to evaluate glob
3 participants