go-ruleguard

go-ruleguard copied to clipboard

Hey!

First off, thanks for a fantastic tool! :) Been spending a day or two at work to implement it in a large'ish codebase, and it has already proven immensely valuable in finding bad patterns that had slipped through the cracks in review.

I'm trying to write a rule guard that will satisfy the following but can't quite figure out if it is possible or not:

1. I want to flag any call to check.OK() anywhere within a method named Fix(context.Context)
2. Calls to check.OK() in other places is fine
3. Calls to other methods than OK() is fine

type Fixer interface {
	Fix(context.Context) *Entry
}

type CheckLoggedInImpl struct {
	checks.Entry
}

func (check *CheckLoggedInImpl) Fix(ctx context.Context) *checks.Entry {
        // .... lots of code here

	if true {
		return check.OK() // not working
	}   

	return check.OK() // working
}

I've tried a bunch of rules, like the one below, but I can't figure out how to traverse the stack and see what the method name is for the Match(), so I know whether it's a problem or not it's being called.

I got the last return of the Fix() method matching, but I can't figure out how to get any call to check.OK() as reporting - regardless of indentation and what code formatting

// Don't use 'path' package as it doesn't support Windows path separator,
// which often leads to bugs and other trouble
//
// https://stackoverflow.com/a/48050736/1081818
func fixMustCallCheck(m dsl.Matcher) {
	m.Import("$INTERNAL_URL/internal/system/checks")

	m.
		Match(
			`func ($_ $implType) Fix($_ context.Context) $_ { $*_ ; return $check.OK() }`,
			`func ($_ $implType) Fix($_ context.Context) $_ { $*_ ; { $*_ ; return $check.OK() } ; $*_ }`,
		).
		Where(m["implType"].Filter(ensureFixer)).
		At(m["check"]).
		Report(`You should not call check.OK() within a Fix() method - please call check.Check(ctx) instead`)
}

func ensureFixer(ctx *dsl.VarFilterContext) bool {
	fixer := ctx.GetInterface(`$INTERNAL_URL/internal/system/checks.Fixer`)
	return types.Implements(ctx.Type, fixer)
}

I made more progress today that seems to detect all calls to .OK() within my method as expected.

The first matcher will correctly highlight the At() for where the check was detected, but the second one captures calls to OK() regardless of conditionals and indentation via the m["$$"].Contains() logic doesn't - it can just highlight the whole matched group.

I couldn't find any method like .At(m["$$"].FindNode("return $name.OK()") which would return the position / node that matched in the .Contains() so it could properly highlight where the match was found. Basically what .Contains() does, but returning the Var() for use in At() rather than a boolean

func fixMustCallCheck(m dsl.Matcher) {
	m.Import("$INTERNAL_URL/internal/system/checks")

	m.
		Match(
			// captures return statement at the end of the func, and allows for proper At() positioning
			`func ($name $implType) Fix($*_) $_ { $*_ ; return $check.OK() ; $*_ }`,
			// Captures the entire body of the func so that the m[$$].Contains can do sub-filtering on calls to $name.OK()
			// it does not have a way to output the _actual_ position of the issue discovered, but it will complain
			// and let you know which file+method is affecting, which is all you need to fix.
			`func ($name $implType) Fix($*_) $_ { $*check }`,
		).
		Where(m["implType"].Filter(ensureFixer) && m["$$"].Contains("return $name.OK()")).
		At(m["check"]).
		Report(`You should not call check.OK() within a Fix() method - please call check.Check(ctx) instead`)
}

@jippi as i know $$ it about expressions before statements in match method, i think you are looking for something like this: https://github.com/delivery-club/delivery-club-rules/blob/462a5e373a3d63ec51a8687cbb6dbc0379ac30b3/rules.go#L327 or if for whole function/package body: https://github.com/delivery-club/delivery-club-rules/blob/462a5e373a3d63ec51a8687cbb6dbc0379ac30b3/rules.go#L446