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

Rewrite sqlvet using the analyzer framework. #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yasushi-saito
Copy link

Use golang.org/x/tools/go/analysis framework. It's simpler, faster. It also
allows analyzing individual packages, not the whole program.

As a simple benchmark, running the sqlvet (old or new) on a 100kloc source takes
about 10s. Analyzing a single file in that source takes <1s.

This PR introduces a few incompatible changes:

  • The commandline format changes. It's now

    sqlvet [-f sqlvet.toml] packages...

    The sqlvet.toml file must be in ".", or its location must be explicitly
    specified by the new "-f" flag. This commandline format is compatible with by
    most other analyzers, including govet and staticcheck.

    The old sqlvet <dir> command becomes:

    cd

    && sqlvet ./...

  • It removes the support for string concatenation.

    db.Query("SELECT" + " 1")

    won't work any more. I personally think this isn't that big deal, we can
    easily rewrite it using a raw string.

Use golang.org/x/tools/go/analysis framework. It's simpler, faster.  It also
allows analyzing individual packages, not the whole program.

As a simple benchmark, running the sqlvet (old or new) on a 100kloc source takes
about 10s.  Analyzing a single file in that source takes <1s.

This PR introduces a few incompatible changes:

- The commandline format changes. It's now

    sqlvet [-f sqlvet.toml] packages...

  The sqlvet.toml file must be in ".", or its location must be explicitly
  specified by the new "-f" flag. This commandline format is compatible with by
  most other analyzers, including govet and staticcheck.

  The old `sqlvet <dir>` command becomes:

  cd <dir> && sqlvet ./...

- It removes the support for string concatenation.

  db.Query("SELECT" + " 1")

  won't work any more. I personally think this isn't that big deal, we can
  easily rewrite it using a `raw string`.
@houqp
Copy link
Owner

houqp commented Dec 11, 2021

Thank you @yasushi-saito, the code simplification looks great 👍 I will take a closer look at it tomorrow :)

@houqp
Copy link
Owner

houqp commented Dec 11, 2021

Thanks @yasushi-saito again for taking a stab at this. I now remember why I went with the SSA form instead of AST in sqlvet. I think I even read the vet-analyzer article you linked at the time because it predates the project. I initially started with AST, but found out it was too restricted for more advanced code analysis. String concatenation is the simplest application of the SSA call graph, in my todo, I was also planning to do function tracing and recursively resolve variable type and value to reduce false positives. For example, the Analyzer approach is currently causing the following false positives to one of my projects:

type Foo struct {
    DbName string
}

func (s Foo) Dosomething() {
    // ...
    dbConn.Exec("QUERY ..." + s.DbName)
}

Have you considered the following two workarounds?

  1. Add a filter in loadGoPackages function to only perform whole program analysis on selected packages
  2. Manage the Analyzer based approach as a feature flag of the cli (fast mode?) so users can decide make their own tradeoff on speed v.s. accuracy.

Comment on lines +61 to +62
// log.Printf("QQQQ: %v err=%v", qs.Query, qs.Err)

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// log.Printf("QQQQ: %v err=%v", qs.Query, qs.Err)

Comment on lines +289 to +291
if shouldIgnoreNode(pass, ignoredNodes, n.Pos()) {
return
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think the ignore check could be performed at the beginning of the callback?

@adamdecaf
Copy link

Any word on updating this?

@adrianlungu
Copy link

@houqp I guess this is somewhat stale ?

If we manage to update this MR or some other version of it; could we have this merged ? It would be a really nice addition to golangci-lint 🙏

@houqp
Copy link
Owner

houqp commented Oct 13, 2024

@adrianlungu yes, I am open to merging this as long as the limitations in my comments are addressed 👍

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

Successfully merging this pull request may close these issues.

4 participants