How to handle `regexp.MustCompile`

[cloudlena]

I have a couple of calls to regexp.MustCompile which, in my opinion, make sense to be stored in a global value because they ensure that the regex is valid at compile time. Would it make sense to add an exception for cases like that? So to whitelist a couple of functions that work like regexp.MustCompile from the standard library?

An example can be found here: https://github.com/mastertinner/latest/blob/master/internal/app/latest/brew/upgrade.go#L13

[leighmcculloch]

I agree, this is probably a good thing to add as something that shouldn't error. But I think the motivation comes from the fact that these are typically used as constants. As far as I'm aware the regex.MustCompile function won't actually check the regex at the time the go compiler compiles the application, because the contents of the string won't be evaluated until the application runs but the function is named MustCompile because it is a companion function to Compile which panics instead of returning an error.

Would you be up for opening a PR with this change?

[leonklingele]

because they ensure that the regex is valid at compile time

As noted by @leighmcculloch this is not the case. It will compile fine, but instantly panics once run:

$ cat main.go
package main

import (
	"regexp"
)

var (
	upgradeRegex = regexp.MustCompile(`[`)
)

func main() {
	println("Hello World")
}

$ go build -o main # Works fine
$ ./main
panic: regexp: Compile(`[`): error parsing regexp: missing closing ]: `[`
[..]

Given that, I see no reason in supporting regexp.MustCompile for global vars.

[palsivertsen]

Having regex patterns as global variables is actually quite useful. Consider you're making an API where you need to validate a bunch of fields. Then you could compile all your regex patterns once at init and use them all over.

package main

import (
	"fmt"
	"regexp"
)

var RegexpPassword = regexp.MustCompile(`^[[:graph:]]+$`)
var RegexpUsername = regexp.MustCompile(`^[[:alpha:]]{1,15}$`)

func main() {
	fmt.Printf("Is valid password: %v\n", RegexpPassword.MatchString(`super secret pa55w0rd!`))
	fmt.Printf("Is valid username: %v\n", RegexpPassword.MatchString(`--neo--`))
}

Instead gochecknoglobals forces one to do compile the regex inline:

package main

import (
	"fmt"
	"regexp"
)

var RegexpPasswordStr = `^[[:graph:]]+$`
var RegexpUsernameStr = `^[[:alpha:]]{1,15}$`

func main() {
	RegexpPassword := regexp.MustCompile(RegexpPasswordStr)
	RegexpUsername := regexp.MustCompile(RegexpUsernameStr)
	fmt.Printf("Is valid password: %v\n", RegexpPassword.MatchString(`super secret pa55w0rd!`))
	fmt.Printf("Is valid username: %v\n", RegexpUsername.MatchString(`--neo--`))
}

The problem here is that now the regex pattern is compiled each time it is used.

The standard library have examples of both uses approaches.

[leighmcculloch]

I think allowing regex's with MustCompile in global vars makes sense. It's a common pattern and this tool shouldn't get in the way of common patterns that don't collide with the purpose of this tool.

[leighmcculloch]

#12 should get merged soon which adds support for this and I just need some personal time to go through it, get it merged, and released.

[palsivertsen]

@leighmcculloch: Should DefaultXXX variables also be allowed? Ref DefaultClient and DefaultServeMux in the http package.

[leighmcculloch]

Should DefaultXXX variables also be allowed?

@palsivertsen I don't think so. Instead replace them with functions that create a default. Global variables like http.DefaultClient result in side-effects where code in one place can change the behavior and state of code in another, unintentionally.

[leighmcculloch]

This has been addressed and merged in #12.