Ah, need a rebase for daemon/prune.go

Discussing in the maintainers call; one downside of this is that mistakes in regular expressions may now result in a panic at runtime. Such mistakes should of course be revealed as part of tests (i.e. if it doesn't trigger in tests, there's no coverage of the regular suppression 😇), so we were discussing approaches for having a Linter verify if any regex.MustCompile is covered by tests.

While discussing that @corhere round an interesting package in golang.org/x/mod that seems to handle most of this, but it's internal, so we'd have to copy it. We could rewrite parts of it to us sync.OnceValue() (as it was likely written before that was introduced);

https://pkg.go.dev/golang.org/x/mod/internal/lazyregexp

it has a special case when running under test

https://cs.opensource.google/go/x/mod/+/refs/tags/v0.19.0:internal/lazyregexp/lazyre.go;l=66-78

The special case when running under test is much simpler to achieve with Go 1.21: https://pkg.go.dev/testing#Testing

Oh! Just after I mentioned "I wish stdlib has something for this" 😂

Yet another alternative to this is https://github.com/containers/storage/tree/main/pkg/regexp (godoc: https://pkg.go.dev/github.com/containers/storage/pkg/regexp).

In particular, it solves the issue of checking all regexes by using a build tag (regexp_precompile), so all that's needed is a CI job to compile everything with this tag set.

so all that's needed is a CI job to compile everything with this tag set.

Ah, yes, that's an option. I think I want to avoid special build-tags though, and it looks like the testing.Testing() would provide an elegant solution to do this automatically.

Looking at the Lazyregexp approach, the only thing I'm not super fond of is that it's also wrapping the Regexp; I gave it a try to continue using a sync.OnceValue to keep it simple.

the only thing I'm not super fond of is that it's also wrapping the Regexp; I gave it a try to continue using a sync.OnceValue to keep it simple.

laxyregexp wrapping the Regexp is one of the things I liked the most about it! Wrapping makes it a near drop-in replacement for regexp.MustCompile: none of the lines which reference the compiled regexp need to be touched. And it blocks the shared regexp from being mutated. While your implementation of lazyregexp is simplified, the complexity is pushed to the consumers.

Thoughts on adding a forbidigo lint rule to block direct uses of regexp.MustCompile outside of lazyregexp, like I did with old-style atomics?

laxyregexp wrapping the Regexp is one of the things I liked the most about it! Wrapping makes it a near drop-in replacement for regexp.MustCompile: none of the lines which reference the compiled regexp need to be touched.

Yeah, that's a good point, and I was going back-and-forth a bit on that. It's nice for it to be a drop-in replacement, but I was also somewhat concerned on that requiring us to keep it in sync with any changes / new features that may be introduced in regexp.Regexp. Overall it looks fairly stable, but I see for example that go1.21 introduced Regexp.MarshalText() (currently missing in the golang.org/x/mod/internal/lazyregexp implementation, although that implementation looks to be only a subset of regexp.Regexp).

Implementing all of those would not be too complicated, so if we want it to be a full drop-in replacement, I can have a look of that, but input welcome.

BTW it probably doesn't make any sense to touch any _test.go files -- those optimizations do not worth it per se. The only worthiness such changes may have is if you consider it a way to test the lazyregexp package, and/or to serve as lazyregexp usage examples.

One other thing is, some uses of regular expressions (in non-test code) may and should be eliminated, as NOT using regexp usually results in faster code and smaller binaries. Yet the task is somewhat hard; for a few examples, see opencontainers/runc#3460 and the linked PRs/CLs.

Overall it looks fairly stable, but I see for example that go1.21 introduced Regexp.MarshalText() (currently missing in the golang.org/x/mod/internal/lazyregexp implementation, although that implementation looks to be only a subset of regexp.Regexp).

Implementing only a subset of regexp.Regexp is a feature, not a bug. Regexp.UnmarshalText, like Regexp.Longest(), mutates the receiver! We need package-global regexp values to be immutable to avoid chaos.

if we want it to be a full drop-in replacement

It's in an internal package. We could trivially add wrapper methods on demand, as needed, just like golang.org/x/mod's lazyregexp package. Though I doubt it would take much time to whip up a full (minus methods which mutate the compiled regexp) wrapper with a bit of editor-fu.

@corhere @kolyshkin I updated this PR to use a fork of the lazyregexp package from golang.org/x/mod, and added a Linter rule to check for MustCompile() uses in non-test code. I already had some of the tests and test-utility packages updated, so I kept those for now, but let me know if you feel strongly about removing those (at least in _test.go files).

Rebased after #49195 was merged (dropped one commit, and removed the now unused ReplaceAllLiteralString from lazyregexp.