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

Implement support for github.com/jackc/pgx #25

Merged
merged 5 commits into from
Aug 27, 2023
Merged

Implement support for github.com/jackc/pgx #25

merged 5 commits into from
Aug 27, 2023

Conversation

hallabro
Copy link
Contributor

@hallabro hallabro commented Aug 16, 2023

These changes add support for github.com/jackc/pgx (including pgxpool).

Two of the biggest changes with this implementation:

  1. The pgx functions return interface types. Because of this we need to check for instr.Call.Method calls as well instr.Call.Value.
  2. For the same reason we need to check for Named types in addition to Pointer types.

There was an issue with the tests in pkg/analyzer that would not allow the tests to run with external packages. From the documentation for analysistest.Run:

// If the directory contains a go.mod file, Run treats it as the root of the
// Go module in which to work. Otherwise, Run treats it as the root of a
// GOPATH-style tree, with package contained in the src subdirectory.

The solution was to add a go.mod and vendor in the pkg/analyzer/testdata directory. This leads to some packages being vendored twice unfortunately but I see no easy solution around this.

This is the first time I write some linter/static code analyzer tool so any feedback is appreciated.

Thanks.

Closes #23.

@hallabro hallabro marked this pull request as ready for review August 16, 2023 09:38
README.md Show resolved Hide resolved
@ryanrolds
Copy link
Owner

Thank you. I will take a look at this by the end of the weekend.

Copy link
Owner

@ryanrolds ryanrolds left a comment

Choose a reason for hiding this comment

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

Thank you. This is overall great work, and look forward to merging it. 😸

I have one question about a variable rename and would like to see some of the new code DRYed out a bit. Happy to chat about strategies to eliminate the repeated code. This is my first linter too, I'm learning with each change as well.

pkg/analyzer/analyzer.go Show resolved Hide resolved
pkg/analyzer/analyzer.go Outdated Show resolved Hide resolved
pkg/analyzer/analyzer.go Outdated Show resolved Hide resolved
Comment on lines 313 to 324
switch tt := targetType.(type) {
case *types.Pointer:
if types.Identical(instrType, tt) {
if checkClosed(instr.Referrers(), targetTypes) {
return actionHandled
}
}
case *types.Named:
if types.Identical(instrType, tt) {
if checkClosed(instr.Referrers(), targetTypes) {
return actionHandled
}
Copy link
Owner

Choose a reason for hiding this comment

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

Another example. There are a couple more areas. Don't worry about the Value and Method paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please elaborate on the Value and Method paths? They don't seem to be part of the code that you highlighted in your comment.

@ryanrolds ryanrolds added the enhancement New feature or request label Aug 20, 2023
@hallabro
Copy link
Contributor Author

Thanks for the feedback Ryan! I'll have a look at your comments today.

@hallabro
Copy link
Contributor Author

I have pushed some changes around the duplicate code. Thanks for pointing it out! All this type casting is making it a little bit difficult for me to fully understand what's going on sometimes.

Hopefully I've responded to your other comments as well. Let me know what you think.

I'd happily continue to work on the code until it reaches a mergeable state.

@ryanrolds
Copy link
Owner

I was thinking something like:

func getTargetTypesValues(b *ssa.BasicBlock, i int, targetTypes []any) []targetValue {
	targetValues := []targetValue{}

	instr := b.Instrs[i]
	call, ok := instr.(*ssa.Call)
	if !ok {
		return targetValues
	}

	signature := call.Call.Signature()
	results := signature.Results()
	for i := 0; i < results.Len(); i++ {
		v := results.At(i)
		varType := v.Type()

		for _, targetType := range targetTypes {
			switch tt := targetType.(type) {
			case *types.Pointer:
				targetValues = checkAndAddTargetValue(varType, tt, call, targetValues)
			case *types.Named:
				targetValues = checkAndAddTargetValue(varType, tt, call, targetValues)
			}
		}
	}

	return targetValues
}

func checkAndAddTargetValue(varType types.Type, tt types.Type, call *ssa.Call, targetValues []targetValue) []targetValue {
	if !types.Identical(varType, tt) {
		return targetValues
	}

	for _, cRef := range *call.Referrers() {
		switch instr := cRef.(type) {
		case *ssa.Call:
			if len(instr.Call.Args) >= 1 && types.Identical(instr.Call.Args[0].Type(), tt) {
				targetValues = append(targetValues, targetValue{
					value: &instr.Call.Args[0],
					instr: call,
				})
			}
		case ssa.Value:
			if types.Identical(instr.Type(), tt) {
				targetValues = append(targetValues, targetValue{
					value: &instr,
					instr: call,
				})
			}
		}
	}

	return targetValues
}

@ryanrolds
Copy link
Owner

Thank you. :) Great job.

@ryanrolds ryanrolds merged commit 986f197 into ryanrolds:main Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can this be made to work with pgx.Rows?
2 participants