proposal: x/tools/go/analysis: export internal/analysisflags and internal/checker

[Antonboom]

Hello!

I spent a couple of days, but I did not find an elegant way to implement this:

package main

import (
    "flag"
    "log"
    "os"

    "golang.org/x/tools/go/analysis/singlechecker"

    "github.com/Antonboom/myanalyzer/pkg/analyzer"
    "github.com/Antonboom/myanalyzer/pkg/config"
)

var (
    configPath = flag.String("config", "", "path to config file (yml)")
    dumpCfg    = flag.Bool("dump-config", false, "dump config example (yml) in stdout")
)

func main() {
    flag.Parse()

    if *dumpCfg {
        mustNil(config.Dump(config.Default, os.Stdout))
        return
    }

    cfg := config.Default
    if *configPath != "" {
        var err error
        cfg, err = config.ParseFromFile(*configPath)
        mustNil(err)
        mustNil(config.Validate(cfg))
    }

    singlechecker.Main(analyzer.New(cfg)) // <-- Own flags logic inside via internal packages.
}

Inputs:

1. It is assumed that the analyzer is used both as a module and as a binary.
2. I want the linter to be configured using a config file (or via config struct if used as module).
3. I don't want to lose the set of flags and functionality from the golang.org/x/tools/go/analysis/singlechecker.

I found a workaround using analysis.Analyzer.Flags and flag.Value implementation interface for config.Config, but this leads to singletons, package-level hacks, etc.

Now I see that a lot of linters that are configured through the configuration file refuse to use the golang.org/x/tools/go/analysis/singlechecker(multichecker).

[ianlancetaylor]

CC @taking @adonovan

[timothy-king]

@Antonboom I do not yet understand quite a bit of the proposal. Is what is being proposed the title, "export internal/analysisflags and internal/checker"? To cut to the chase, we are unlikely to expose these as is. We may expose specific APIs. The proposal should say exactly what the exposed API should be to be accepted.

I also do not understand the problem you are experiencing very well. What I do understand is that: 1) you want to create a *Analyzer from a config object. 2) The config file should be loadable via a file.

I do now know what these mean or what you are referring to:

I don't want to lose the set of flags and functionality from the golang.org/x/tools/go/analysis/singlechecker.

I found a workaround using analysis.Analyzer.Flags and flag.Value implementation interface for config.Config, but this leads to singletons, package-level hacks, etc.

Now I see that a lot of linters that are configured through the configuration file refuse to use the golang.org/x/tools/go/analysis/singlechecker(multichecker).

Can you please provide more context? Thanks.

[Antonboom]

1. singlechecker uses internal packages for own flags registration and parsing.
2. Logic of singlechecker flags is assumed that the analyzer can be constructed without the use of flags.
3. You can't add your "top level" flags and use it before singlechecker.Main without conflicting with singlechecker.

singlechecker.Main

package singlechecker

import (
    // ...
    "golang.org/x/tools/go/analysis"
    "golang.org/x/tools/go/analysis/internal/analysisflags" // INTERNAL
    "golang.org/x/tools/go/analysis/internal/checker"       // INTERNAL
    "golang.org/x/tools/go/analysis/unitchecker"
)

// Main is the main function for a checker command for a single analysis.
func Main(a *analysis.Analyzer) {
    // ...
    if err := analysis.Validate(analyzers); err != nil {
        log.Fatal(err)
    }

    checker.RegisterFlags() // <- Useful logic.

    flag.Usage = func() {
        // ...
    }

    analyzers = analysisflags.Parse(analyzers, false) // <- Useful logic.

    args := flag.Args() // Not for all linters is useful.
    if len(args) == 0 {
        flag.Usage()
        os.Exit(1)
    }

    if len(args) == 1 && strings.HasSuffix(args[0], ".cfg") { // Not for all linters is useful.
        unitchecker.Run(args[0], analyzers)
        panic("unreachable")
    }

    os.Exit(checker.Run(args, analyzers))
}

How it may be in my case:

package main

import (
    // ...
    "golang.org/x/tools/go/analysis"
    "golang.org/x/tools/go/analysis/checker" // NOT INTERNAL
    "golang.org/x/tools/go/analysis/singlechecker"

    "github.com/Antonboom/myanalyzer/pkg/analyzer"
    "github.com/Antonboom/myanalyzer/pkg/config"
)

var (
    configPath = flag.String("config", "", "path to config file (yml)")
    dumpCfg    = flag.Bool("dump-config", false, "dump config example (yml) in stdout")
)

func main() {
    checker.RegisterFlags()
    flag.Parse()

    if *dumpCfg {
        mustNil(config.Dump(config.Default, os.Stdout))
        return
    }

    cfg := config.Default
    if *configPath != "" {
        var err error
        cfg, err = config.ParseFromFile(*configPath)
        mustNil(err)
        mustNil(config.Validate(cfg))
    }

    a := analyzer.New(cfg)
    analyzers := []*analysis.Analyzer{a}

    err := analysis.Validate(analyzers)
    mustNil(err)
    
    os.Exit(checker.Run(flag.Args(), analyzers))
}

[timothy-king]

IIUC you want to be able to process flags before calling singlechecker.Main(...) in order to create an analyzer based on the flags. This is doable via package level variables at the moment, but IIUC you find this a bit unwieldy. This seems fairly reasonable to me as a feature request.

Important details still need to be worked out in the proposal. It is not yet clear how this would interact with flags from the checkers. It is also not yet clear how such flags will be forwarded to unitchecker. Some level of glue code seems necessary to register and forward these.

[adonovan]

This issue is closely related to #53215.

I think it would be fine for single- and multi-checker to expose any configuration option accepted by go/packages in their command-line interfaces, but those two functions are not intended for use other than as the entirety of the main function.

There is clearly a need for a set of finer grained components for constructing go/packages-based analysis drivers, something that cannot be done today without forking or otherwise using internal/checker. We should expose the Load, Validate, Analyze, and Print operations in some form, though care may be required to help clients maintain the necessary invariants between Load and Analyze. I plan to put together a sketch this week.

[Antonboom]

I plan to put together a sketch this week.

Thank u!

Important details still need to be worked out in the proposal. Some level of glue code seems necessary to register and forward these.

I suggest waiting for @adonovan.

[gopherbot]

Change https://go.dev/cl/411907 mentions this issue: go/analysis/checker: a go/packages-based driver library

[Antonboom]

@timothy-king, can u remove "waiting for info" badge please?
Or you still have any questions?

[Antonboom]

Hello, guys :)

Any updates after 10 months?

[adonovan]

Any updates after 10 months?

Not really, sorry. I prepared a CL https://go.dev/cl/411907, but it served to highlight that there are quite a number of different ways we could design this (as there are quite a number of tasks involved in an analysis driver) and I didn't and still don't have have a good sense of exactly how much control and flexibility it was necessary to expose in a package of reusable components for writers of analysis drivers.

I'd like to look into this and a number of related analysis issues later this year once our gopls work becomes less intense.

[adonovan]

I recently brought https://go.dev/cl/411907 up to date and filled in the various gaps. Please take a look at the CL, and if you have any feedback or suggestions about the new API, please comment on the CL or proposal (#61324).

[adonovan]

Another year passed, and CL 411907 was brought up to date again. This time, it was eventually merged. ;-)