Skip to content
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.

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

Ability to check if a match is within a method implementing interface (or method name matches) #409

Open
jippi opened this issue Sep 12, 2022 · 2 comments

Comments

@jippi
Copy link

jippi commented Sep 12, 2022

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)
}
@jippi
Copy link
Author

jippi commented Sep 13, 2022

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`)
}

@peakle
Copy link
Contributor

peakle commented Sep 13, 2022

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants