fix: Add missing context to builtins.{match,split} results

[ztzg]

Motivation

Without this, it is easy to accidentally generate, e.g., a config file referencing a store path without recording proper dependency information.

---

P.-S. — This patch does not backport effortlessly to some older branches, but I have another one that applies there. I can open another PR once/if this is handled in master.

[ztzg]

Cc: @tomberek.

[edolstra]

Important to note that this is a behaviour change that can result in different evaluation results. See for instance the failing flake-regressions test:

       … while evaluating an attribute name
         at /nix/store/5s3d8kijab7azrn8i7bwax9zkxh0g6ra-source/pkgs/build-support/kernel/make-initrd.nix:35:35:
           34| , _compressorName ? compressorName _compressorExecutable
           35| , _compressorMeta ? compressors.${_compressorName} or {}
             |                                   ^
           36|

       error: the string 'zstd' is not allowed to refer to a store path (such as '!bin!ik59basa0s6f2h9yzw5icagav5j4j28y-zstd-1.5.5.drv')

Probably it was a mistake not to use forceStringNoCtx for the arguments of match/split, but we can't do anything about that now...

[roberth]

Should we scan the resulting chunks for hashes?

I suppose that could fail to mark a piece of hash, when it's not the full hash, but it seems acceptable to assume that a piece of a hash does not constitute a reference.

Off-topic:
If we have any string context that is not identifyable by a hash, we should just keep it, but I don't think that's even relevant before things like #8388.

[tomberek]

There is some precedent for creating safer versions of the primops in Nixpkgs lib. That could be another approach, lib.strings.{match,split} that re-add the context back. That would avoid a behavior change in evaluation of historical Nixpkgs.

[ztzg]

@edolstra wrote:

Important to note that this is a behaviour change that can result in different evaluation results. See for instance the failing flake-regressions test [...]

Argh. Of course.

@roberth wrote:

Should we scan the resulting chunks for hashes?

This may work in practice, but it feels a bit too magic/foreign given that e.g. even a zero-length substring preserves its parent's context.

@tomberek wrote:

There is some precedent for creating safer versions of the primops in Nixpkgs lib.

Good idea!

In the meantime, I suppose I should just close this PR and create a new one documenting the caveat?

[roberth]

I'm not 100% sure that not fixing the bug is the best approach.

• builtins.{match,split} remain a risk for users
• lib.{match,split} will be slowed by the added behavior, especially when hash scanning is performed
  • more so than a C++ implementation would

I don't know if we have good data for the possible hash scanning. You may need to use (import drvPath).${outputName} to get an output store path or CA placeholder.

I'd support a fix in either project.