Skip to content

Allow Path.is_descendant to work on external paths.#14382

Open
ElectreAAS wants to merge 2 commits into
ocaml:mainfrom
ElectreAAS:push-zsxyzmpmuztq
Open

Allow Path.is_descendant to work on external paths.#14382
ElectreAAS wants to merge 2 commits into
ocaml:mainfrom
ElectreAAS:push-zsxyzmpmuztq

Conversation

@ElectreAAS
Copy link
Copy Markdown
Collaborator

Extracted from #13792.
Right now, Path.is_descendant can be called on two Path.t, whatever their subtype, but if at least one of them is an External, it will always return false.
This is surprising, you'd expect is_descendant (e "/foo/bar") ~of_:(e "/"); to return true, to quote one test.

This PR extends Path.is_descendant to use the already existing Path.External.is_descendant, mirroring other type variants.
To further mirror other variants, Path.External.is_descendant is patched to include ... || a = b.

Copy link
Copy Markdown
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a solid improvement. I have one question that would be good to have an answer to, besides that it gets a solid 👍 from me.

Comment thread otherlibs/stdune/src/path_external.ml Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Stdune.Path.is_descendant so that it correctly handles External paths (instead of always returning false), aligning behavior with the other Path.t variants and updating tests accordingly.

Changes:

  • Delegate Path.is_descendant to Path.External.is_descendant when both arguments are External.
  • Adjust Path.External.is_descendant to consider equality (a = b) as a descendant relationship.
  • Update stdune expect tests to assert the new External descendant behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
otherlibs/stdune/src/path.ml Adds an External/External case to Path.is_descendant.
otherlibs/stdune/src/path_external.ml Updates external descendant logic to include equality and prefix-based checks.
otherlibs/stdune/test/path_tests.ml Updates expectations to reflect the new External descendant semantics.

Comment thread otherlibs/stdune/src/path_external.ml Outdated
Copy link
Copy Markdown
Collaborator

@Alizter Alizter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of consumers of this function so I would like to conduct a thorough review. This will take time to do so I will block this PR in the meantime so there are no misunderstandings.

@Alizter Alizter self-requested a review April 30, 2026 11:53
@Alizter Alizter self-assigned this Apr 30, 2026
@ElectreAAS
Copy link
Copy Markdown
Collaborator Author

We took a quick look at the users of Path.is_descendant that don't explicitely use other variants of Path.t.

  • In otherlibs, Temp.clear_dir has an explicit check on inequality, meaning they already presuppose that is_desc x x = true.
  • In the engine, Sandbox.find_corrected_files has an explicit check on equality that could be removed with/after this PR.
  • In melange, Melange_rules.Runtime_deps.targets, there is a call to is_desc that I don't know anything about, maybe worth pinging knowledgeable people on?

ElectreAAS added 2 commits May 6, 2026 11:55
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
…in sandbox

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS ElectreAAS force-pushed the push-zsxyzmpmuztq branch from 887de1d to 06b6160 Compare May 6, 2026 09:57
@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented May 18, 2026

After having a look through the consumers of Path.is_descendent I am coming to the conclusion that this function is entirely misguided. Not the fault of this PR, but a code smell that existed before. The caller has to always keep in mind whether or not the two paths they are providing are of the same variant which is pointless because we have those types and can enforce it. The call sites look buggy to me in a few different ways.

I think our best course of action here would be to understand its usage at each call site. Instead use the correct variant-specific version or remove the check entirely, and finally remove this function and its confusing nature.

If you want to work on this further, I would suggest you make a table of all the call sites and see if we have an idea of what we can replace each site with.

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.

4 participants