libexpr: Don't use nix::dirOf in prim_dirOf (fix 2.23 regression)

[xokdvium]

Motivation

Fixes #14513 and adds tests. A lot of builtins.dirOf behavior on strings is cursed, but there's no changing that now. Best we can do is stay bug-for-bug compatible with prior nix versions. This should get backported as far as possible.

Context

The first commit adds tests but doesn't change the regressed behavior. The second fixes it and updates the test.

---

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[Ericson2314]

Should we backport, @xokdvium?

[Ericson2314]

Also, we might want to add a test for dirOf preserving the string context too, so it is not just the flake regression test suite that happens to catch it.

[xokdvium]

Should we backport

Since this is a bug that is somewhat recent it seems prudent to backport to all supported versions while we still can.

[internal-nix-ci]

Successfully created backport PR for 2.28-maintenance:

• [Backport 2.28-maintenance] libexpr: Don't use nix::dirOf in prim_dirOf (fix 2.23 regression) #14520

[internal-nix-ci]

Successfully created backport PR for 2.29-maintenance:

• [Backport 2.29-maintenance] libexpr: Don't use nix::dirOf in prim_dirOf (fix 2.23 regression) #14521

[internal-nix-ci]

Successfully created backport PR for 2.30-maintenance:

• [Backport 2.30-maintenance] libexpr: Don't use nix::dirOf in prim_dirOf (fix 2.23 regression) #14522

[internal-nix-ci]

Successfully created backport PR for 2.31-maintenance:

• [Backport 2.31-maintenance] libexpr: Don't use nix::dirOf in prim_dirOf (fix 2.23 regression) #14523

[internal-nix-ci]

Successfully created backport PR for 2.32-maintenance:

• [Backport 2.32-maintenance] libexpr: Don't use nix::dirOf in prim_dirOf (fix 2.23 regression) #14524

[edolstra]

Did anybody observe this "regression" in the real world? I'm not sure we should revert to worse behavior if nobody was affected by this... Bug-for-bug compatibility should really only apply if people were actually depending on the buggy behaviour. Since nobody complained in almost a year since 2.23, that's good evidence that nobody was affected by this.

[xokdvium]

Since nobody complained in almost a year since 2.23, that's good evidence that nobody was affected by this.

Yes, that does mean that we can fix the behavior in the future, but using std::filesystem was definitely an accident. Because of it e.g. dirOf "///" evaluated to ///. That is trading one incorrect behavior for another and that doesn't help much. Sticking to a "known" bad at least lessens the amount of chaos in the ecosystem.

[Ericson2314]

I agree with @xokdvium and also that that I would be comfortable changing this intentionally in the future, because I also don't believe in bug-for-bug compat. But yes this was neither intentional, not a clear net-improvement (and using std::filesystem in the wrong way was definitely a clear negative).

[roberth]

bug-for-bug compat

You have more important things to achieve than risking making a mess of this builtin, when changes like this eventually boomerang back to you.

If you want a clean language, I'd suggest making a new one that's data-model-interoperable through some small interface (JSON + string context + function calls).
Don't waste your time on the builtins.