Skip to content

fix(assert): avoid panic on invalid regex in Regexp/NotRegexp (#1794) #1818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+41 −8
Open

fix(assert): avoid panic on invalid regex in Regexp/NotRegexp (#1794) #1818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9dd2f76
fix(assert): avoid panic on invalid rx by using regexp.Compile; add t…
kdt523 Oct 25, 2025
e58e326
test(assert): gofmt tabs in regexp_invalid_test.go to satisfy CI form…
kdt523 Oct 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions assert/assertions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1708,22 +1708,28 @@ func ErrorContains(t TestingT, theError error, contains string, msgAndArgs ...in
return true
}

// matchRegexp return true if a specified regexp matches a string.
func matchRegexp(rx interface{}, str interface{}) bool {
// matchRegexp returns whether the compiled regular expression matches the provided
// value. If rx is not a *regexp.Regexp, rx is formatted with fmt.Sprint and compiled.
// When compilation fails, an error is returned instead of panicking.
func matchRegexp(rx interface{}, str interface{}) (bool, error) {
var r *regexp.Regexp
if rr, ok := rx.(*regexp.Regexp); ok {
r = rr
} else {
r = regexp.MustCompile(fmt.Sprint(rx))
var err error
r, err = regexp.Compile(fmt.Sprint(rx))
if err != nil {
return false, err
}
}

switch v := str.(type) {
case []byte:
return r.Match(v)
return r.Match(v), nil
case string:
return r.MatchString(v)
return r.MatchString(v), nil
default:
return r.MatchString(fmt.Sprint(v))
return r.MatchString(fmt.Sprint(v)), nil
}
}

Expand All @@ -1736,7 +1742,11 @@ func Regexp(t TestingT, rx interface{}, str interface{}, msgAndArgs ...interface
h.Helper()
}

match := matchRegexp(rx, str)
match, err := matchRegexp(rx, str)
if err != nil {
Fail(t, fmt.Sprintf("invalid regular expression %q: %v", fmt.Sprint(rx), err), msgAndArgs...)
Copy link
Collaborator

@ccoVeille ccoVeille Oct 28, 2025

Choose a reason for hiding this comment

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

This would be better, no?

Suggested change
Fail(t, fmt.Sprintf("invalid regular expression %q: %v", fmt.Sprint(rx), err), msgAndArgs...)
Fail(t, fmt.Sprintf("invalid regular expression %q: %v", rx, err), msgAndArgs...)

Copy link
Author

@kdt523 kdt523 Oct 29, 2025

Choose a reason for hiding this comment

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

Hi @ccoVeille — thanks for the suggestion, very helpful to question that line.

Quick reason I didn’t pass rx directly to %q: %q expects a string (or string-like) value. In our code rx is an interface{} and callers sometimes pass a compiled *regexp.Regexp. If you do fmt.Sprintf("%q", rx) with a *regexp.Regexp, Go prints an unhelpful %!q(...) type dump instead of the pattern. That makes the error message confusing.
Small examples:
rx := "start" → fmt.Sprintf("%q", rx) => "start" (good)
rx := regexp.MustCompile("start") → fmt.Sprintf("%q", rx) => %!q(*regexp.Regexp=...) (bad)
Safe alternatives

1.Keep the quoted style by converting rx to a string first:
fmt.Sprintf("invalid regular expression %q: %v", fmt.Sprint(rx), err)
Pros: preserves a quoted pattern; simple.

2.Don’t quote, just use %v:
fmt.Sprintf("invalid regular expression %v: %v", rx, err)
Pros: always works for any type; no weird %!q output. Cons: loses the quoted look.

3.(Preferred) Be explicit: if rx is a *regexp.Regexp, call .String(), otherwise fall back to fmt.Sprint:
var rxStr string
switch v := rx.(type) {
case *regexp.Regexp:
rxStr = v.String()
default:
rxStr = fmt.Sprint(rx)
}
Fail(t, fmt.Sprintf("invalid regular expression %q: %v", rxStr, err), msgAndArgs...)

Pros: most robust and readable; shows the actual pattern for compiled regexps and handles any other types cleanly.
If you’re OK with that, I can apply option 3 (tiny, explicit change) to the fix/assert-regexp-compile-error branch and push it. If you prefer a simpler tweak, I can apply option 1 instead — tell me which and I’ll open a small commit/PR.

Copy link
Collaborator

@ccoVeille ccoVeille Oct 29, 2025

Choose a reason for hiding this comment

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

I thought %q would behave the same way %s with quote.

I checked https://cs.opensource.google/go/go/+/master:src/fmt/format.go

I'm a bit surprised it would differ.

Your reply sounds like it would, I will have to check further.

But for now, let's say the code can stay like you did.

Copy link
Collaborator

@ccoVeille ccoVeille Oct 29, 2025

Choose a reason for hiding this comment

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

I'm still doubtful

https://go.dev/play/p/WBwS5ClJU0i

Copy link
Author

@kdt523 kdt523 Oct 29, 2025

Choose a reason for hiding this comment

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

You’re right that for *regexp.Regexp, %q and %s both output the pattern, with %q just adding quotes. In most cases, this isn’t harmful.

My main reason for switching to %v (or [fmt.Sprint]) was to avoid unnecessary quoting and ensure consistent, clear error messages—especially if the input isn’t a string or is a different type. This way, the formatting is robust for all cases, not just regex patterns.

If you find any edge cases where %q would be preferable, I’m happy to revisit! For now, I think the current approach is a bit safer and more general. Thanks again for your thoughtful feedback!

return false
}

if !match {
Fail(t, fmt.Sprintf("Expect \"%v\" to match \"%v\"", str, rx), msgAndArgs...)
Expand All @@ -1753,7 +1763,11 @@ func NotRegexp(t TestingT, rx interface{}, str interface{}, msgAndArgs ...interf
if h, ok := t.(tHelper); ok {
h.Helper()
}
match := matchRegexp(rx, str)
match, err := matchRegexp(rx, str)
if err != nil {
Fail(t, fmt.Sprintf("invalid regular expression %q: %v", fmt.Sprint(rx), err), msgAndArgs...)
Copy link
Collaborator

@ccoVeille ccoVeille Oct 28, 2025

Choose a reason for hiding this comment

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

This would be better, no?

Suggested change
Fail(t, fmt.Sprintf("invalid regular expression %q: %v", fmt.Sprint(rx), err), msgAndArgs...)
Fail(t, fmt.Sprintf("invalid regular expression %q: %v", rx, err), msgAndArgs...)

return false
}

if match {
Fail(t, fmt.Sprintf("Expect \"%v\" to NOT match \"%v\"", str, rx), msgAndArgs...)
Expand Down
19 changes: 19 additions & 0 deletions assert/regexp_invalid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package assert

import "testing"

// Verifies that invalid patterns no longer cause a panic when using Regexp/NotRegexp.
// Instead, the assertion should fail and return false.
func TestRegexp_InvalidPattern_NoPanic(t *testing.T) {
NotPanics(t, func() {
mockT := new(testing.T)
False(t, Regexp(mockT, "\\C", "whatever"))
})
}

func TestNotRegexp_InvalidPattern_NoPanic(t *testing.T) {
NotPanics(t, func() {
mockT := new(testing.T)
False(t, NotRegexp(mockT, "\\C", "whatever"))
})
}