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

go1.21: load rules: parse rules file: typechecker error: ...: could not import #449

Closed
ldez opened this issue Jul 14, 2023 · 22 comments · Fixed by #455 or #451
Closed

go1.21: load rules: parse rules file: typechecker error: ...: could not import #449

ldez opened this issue Jul 14, 2023 · 22 comments · Fixed by #455 or #451

Comments

@ldez
Copy link
Contributor

ldez commented Jul 14, 2023

Hello,

I know it's a bit early but I started to work on go1.21 for golangci-lint.
golangci/golangci-lint#3922

And there is an issue with go-ruleguard.
I run the tests of go-ruleguard and there is the same problem:

ruleguard init error, skip ./rules.go: typechecker error: ./rules.go:6:8: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")

Currently, I don't know the root cause but I want to share it with you, I think will you see the problem faster than me.

Maybe it's related to the new package initialization order, I don't know.

I already created an issue on go-critic.

go test
$ go test ./...                             
?       github.com/quasilyte/go-ruleguard       [no test files]
?       github.com/quasilyte/go-ruleguard/analyzer/testanalyzer [no test files]
?       github.com/quasilyte/go-ruleguard/cmd/ruleguard [no test files]
?       github.com/quasilyte/go-ruleguard/internal/golist       [no test files]
?       github.com/quasilyte/go-ruleguard/internal/xtypes       [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/goutil      [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/ir  [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/irprint     [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/profiling   [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/internal/evaltest   [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qfmt [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qstrconv     [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qstrings     [no test files]
--- FAIL: TestAnalyzer (4.78s)
    --- FAIL: TestAnalyzer/gocritic (0.38s)
        analysistest.go:295: error analyzing ruleguard@gocritic: load rules: parse rules file: typechecker error: ./testdata/src/gocritic/rules.go:6:8: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")
...
@quasilyte
Copy link
Owner

Perhaps a newer version of x/tools should be used yet again. :)
Let me try updating it here.

@quasilyte
Copy link
Owner

See #450

@ldez
Copy link
Contributor Author

ldez commented Jul 17, 2023

Sadly, the update of x/tools doesn't fix the problem.

@ldez
Copy link
Contributor Author

ldez commented Aug 2, 2023

The rc4 is here, the GA is closer than ever.

@quasilyte can I help you?

@ldez
Copy link
Contributor Author

ldez commented Aug 8, 2023

go1.21 is available https://go.dev/blog/go1.21

@liggitt
Copy link
Contributor

liggitt commented Aug 9, 2023

go env output changed in go1.21 to be single-quoted and escape single quotes as '\'' in https://go-review.googlesource.com/c/go/+/493535

parseGoEnv() in https://github.com/quasilyte/go-ruleguard/blob/master/internal/goenv/goenv.go has to handle both double-quoted and single-quoted formats now, something like:

	} else {
		// Line format is: `$name="$value"` before go1.21
		// Line format is: `$name='$value'` in go1.21+
		for _, l := range lines {
			parts := strings.Split(strings.TrimSpace(l), "=")
			if len(parts) != 2 {
				continue
			}
			val := parts[1]
			switch {
			case val == `""` || val == `''`:
				val = ""
			case len(val) > 2 && val[0] == '"' && val[len(val)-1] == '"':
				// go < 1.21 before https://github.com/golang/go/commit/f379e78951a405e7e99a60fb231eeedbf976c108
				var err error
				val, err = strconv.Unquote(parts[1])
				if err != nil {
					continue
				}
			case len(val) > 2 && val[0] == '\'' && val[len(val)-1] == '\'':
				// go >= 1.21 after https://github.com/golang/go/commit/f379e78951a405e7e99a60fb231eeedbf976c108
				// trim leading/trailing single-quotes
				val = val[1 : len(val)-1]
				// replace internally escaped single-quotes
				val = strings.ReplaceAll(val, `'\''`, `'`)
			default:
				val = ""
			}
			vars[parts[0]] = val
		}
	}

@quasilyte
Copy link
Owner

AFAIK parseGoEnv is only used in the fallback importer, if everything else fails.
It's a good thing to make it work again, but other importers should be able to import the packages from the standard library.

@liggitt
Copy link
Contributor

liggitt commented Aug 9, 2023

AFAIK parseGoEnv is only used in the fallback importer, if everything else fails. It's a good thing to make it work again, but other importers should be able to import the packages from the standard library.

not sure how it got there, but resolving this fixed go-critic/go-critic#1359 (comment)

@liggitt
Copy link
Contributor

liggitt commented Aug 9, 2023

go team also suggested the following in https://go-review.googlesource.com/c/go/+/493535:

If you call go env with a specific list of variables, it outputs those variables unquoted, separated by newlines. That's probably the simplest and most portable option.

If for some reason you don't know the list of variables needed, you could call go env once, parse out the list of (unquoted) variable names, and then call go env again with those names as explicit arguments.

Or, to be robust to variables that contain internal newline characters, you could reinvoke go env once per variable — although in most cases that seems overkill. 😅

(Based on https://github.com/search?q=repo%3Aquasilyte%2Fgo-ruleguard%20goenv&type=code, it looks like go-ruleguard only needs GOROOT, GOPATH, GOARCH, GOOS, and CGO_ENABLED?)

@ldez
Copy link
Contributor Author

ldez commented Aug 9, 2023

The link seems private.

The public link is https://go-review.googlesource.com/c/go/+/488375

@cristaloleg
Copy link
Collaborator

Looks like public git with extra steps http://go.dev/cl/488375

@ldez
Copy link
Contributor Author

ldez commented Aug 9, 2023

I try your suggestion (I will remove it after) inside my PR #451
And it works!

@ldez
Copy link
Contributor Author

ldez commented Aug 9, 2023

Maybe you can open a PR.

@ldez
Copy link
Contributor Author

ldez commented Aug 9, 2023

in fact, it works locally but not in the CI...
https://github.com/ldez/go-ruleguard/actions/runs/5813667003/job/15761731524

@ldez
Copy link
Contributor Author

ldez commented Aug 9, 2023

I can reproduce the failure locally:

$ make test 
go test -timeout=10m -count=1 -race -coverpkg=./... -coverprofile=coverage.txt -covermode=atomic ./...
?   d   github.com/quasilyte/go-ruleguardt   dsl[no test files]ATH_DIR     internal/      lint           README.md      ruleguard/     rules.go       _test/         test-master                                                                     
ok      github.com/quasilyte/go-ruleguard/analyzer      60.713s coverage: 75.4% of statements in ./...
?       github.com/quasilyte/go-ruleguard/analyzer/testanalyzer [no test files]
?       github.com/quasilyte/go-ruleguard/cmd/ruleguard [no test files]
?       github.com/quasilyte/go-ruleguard/internal/golist       [no test files]
?       github.com/quasilyte/go-ruleguard/internal/xtypes       [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/goutil      [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/ir  [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/irprint     [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/profiling   [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/internal/evaltest   [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qfmt [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qstrconv     [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qstrings     [no test files]
ok      github.com/quasilyte/go-ruleguard/internal/goenv        1.007s  coverage: 67.7% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/internal/xsrcimporter 2.394s  coverage: 100.0% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard     2.659s  coverage: 41.4% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/irconv      1.368s  coverage: 45.7% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/quasigo     13.643s coverage: 85.8% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/textmatch   1.018s  coverage: 95.7% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/typematch   1.012s  coverage: 66.2% of statements in ./...
go test -count=3 -run=TestE2E ./analyzer
ok      github.com/quasilyte/go-ruleguard/analyzer      76.182s
cd rules && go test -v .
=== RUN   TestRules
    analysistest.go:295: error analyzing ruleguard@_/home/ldez/sources/golangci-lint/go-ruleguard/rules/testdata: load rules: parse rules file: typechecker error: diag.go:4:2: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")
--- FAIL: TestRules (0.33s)
FAIL
FAIL    github.com/quasilyte/go-ruleguard/rules 0.336s
FAIL
make: *** [Makefile:19: test] Error 1

@liggitt
Copy link
Contributor

liggitt commented Aug 9, 2023

opened #455 with the fix suggested by the go team

@ldez
Copy link
Contributor Author

ldez commented Aug 9, 2023

🤔 it still fails:

$ make test
go test -timeout=10m -count=1 -race -coverpkg=./... -coverprofile=coverage.txt -covermode=atomic ./...
?       github.com/quasilyte/go-ruleguard       [no test files]
ok      github.com/quasilyte/go-ruleguard/analyzer      64.546s coverage: 75.6% of statements in ./...
?       github.com/quasilyte/go-ruleguard/analyzer/testanalyzer [no test files]
?       github.com/quasilyte/go-ruleguard/cmd/ruleguard [no test files]
?       github.com/quasilyte/go-ruleguard/internal/golist       [no test files]
?       github.com/quasilyte/go-ruleguard/internal/xtypes       [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/goutil      [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/ir  [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/irprint     [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/profiling   [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/internal/evaltest   [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qfmt [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qstrconv     [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qstrings     [no test files]
ok      github.com/quasilyte/go-ruleguard/internal/goenv        1.008s  coverage: 66.7% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/internal/xsrcimporter 2.433s  coverage: 100.0% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard     2.790s  coverage: 41.6% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/irconv      1.411s  coverage: 45.7% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/quasigo     13.914s coverage: 85.8% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/textmatch   1.018s  coverage: 95.7% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/typematch   1.015s  coverage: 66.2% of statements in ./...
go test -count=3 -run=TestE2E ./analyzer
ok      github.com/quasilyte/go-ruleguard/analyzer      73.428s
cd rules && go test -v .
=== RUN   TestRules
    analysistest.go:295: error analyzing ruleguard@_/home/ldez/sources/golangci-lint/go-ruleguard/rules/testdata: load rules: parse rules file: typechecker error: diag.go:4:2: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")
--- FAIL: TestRules (0.34s)
FAIL
FAIL    github.com/quasilyte/go-ruleguard/rules 0.343s
FAIL
make: *** [Makefile:19: test] Error 1

https://github.com/ldez/go-ruleguard/actions/runs/5814009305

@liggitt
Copy link
Contributor

liggitt commented Aug 9, 2023

hrmm... my fix resolved the issue I was seeing finding stdlib imports... the error you're seeing looks to be in resolving non-stdlib imports

there may be more issues to resolve here, though

@ldez
Copy link
Contributor Author

ldez commented Aug 9, 2023

Maybe someone can reopen this issue.

@cristaloleg cristaloleg reopened this Aug 9, 2023
@ldez
Copy link
Contributor Author

ldez commented Aug 10, 2023

I bisected Go and the first commit that fails is golang/go@f379e78

Bisecting: 0 revisions left to test after this (roughly 0 steps)
[f379e78951a405e7e99a60fb231eeedbf976c108] cmd/go: sanitize go env outputs
running  '/mygo/test.sh'
Building Go cmd/dist using /usr/local/go. (go1.20.4 linux/amd64)
Building Go toolchain1 using /usr/local/go.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for linux/amd64.
---
Installed Go for linux/amd64 in /mygo/go
Installed commands in /mygo/go/bin
*** You need to add /mygo/go/bin to your PATH.
go test -timeout=10m -count=1 -race -coverpkg=./... -coverprofile=coverage.txt -covermode=atomic ./...
?       github.com/quasilyte/go-ruleguard       [no test files]
ok      github.com/quasilyte/go-ruleguard/analyzer      60.893s coverage: 58.2% of statements in ./...
?       github.com/quasilyte/go-ruleguard/analyzer/testanalyzer [no test files]
?       github.com/quasilyte/go-ruleguard/cmd/ruleguard [no test files]
?       github.com/quasilyte/go-ruleguard/internal/golist       [no test files]
?       github.com/quasilyte/go-ruleguard/internal/xtypes       [no test files]
ok      github.com/quasilyte/go-ruleguard/internal/goenv        1.007s  coverage: 66.7% of statements in ./...
?       github.com/quasilyte/go-ruleguard/ruleguard/goutil      [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/ir  [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/irprint     [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/profiling   [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/internal/evaltest   [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qfmt [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qstrconv     [no test files]
?       github.com/quasilyte/go-ruleguard/ruleguard/quasigo/stdlib/qstrings     [no test files]
ok      github.com/quasilyte/go-ruleguard/internal/xsrcimporter 2.545s  coverage: 100.0% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard     3.039s  coverage: 0.0% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/irconv      1.484s  coverage: 12.1% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/quasigo     13.989s coverage: 30.3% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/textmatch   1.035s  coverage: 95.7% of statements in ./...
ok      github.com/quasilyte/go-ruleguard/ruleguard/typematch   1.023s  coverage: 10.3% of statements in ./...
go test -count=3 -run=TestE2E ./analyzer
ok      github.com/quasilyte/go-ruleguard/analyzer      73.342s
cd rules && go test -v .
=== RUN   TestRules
    analysistest.go:295: error analyzing ruleguard@_/mygo/go-ruleguard/rules/testdata: load rules: parse rules file: typechecker error: diag.go:4:2: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")
--- FAIL: TestRules (0.27s)
FAIL
FAIL    github.com/quasilyte/go-ruleguard/rules 0.274s
FAIL
make: *** [Makefile:19: test] Error 1
f379e78951a405e7e99a60fb231eeedbf976c108 is the first bad commit
commit f379e78951a405e7e99a60fb231eeedbf976c108
Author: Michael Matloob <[email protected]>
Date:   Mon Apr 24 16:57:28 2023 -0400

    cmd/go: sanitize go env outputs
    
    go env, without any arguments, outputs the environment variables in
    the form of a script that can be run on the host OS. On Unix, single
    quote the strings and place single quotes themselves outside the
    single quoted strings. On windows use the set "var=val" syntax with
    the quote starting before the variable.
    
    Fixes #58508
    
    Change-Id: Iecd379a4af7285ea9b2024f0202250c74fd9a2bd
    Reviewed-on: https://go-review.googlesource.com/c/go/+/488375
    TryBot-Result: Gopher Robot <[email protected]>
    Reviewed-by: Michael Matloob <[email protected]>
    Reviewed-by: Damien Neil <[email protected]>
    Run-TryBot: Michael Matloob <[email protected]>
    Reviewed-by: Bryan Mills <[email protected]>
    Reviewed-by: Quim Muntal <[email protected]>

 src/cmd/go/internal/envcmd/env.go           | 60 +++++++++++++++++-
 src/cmd/go/internal/envcmd/env_test.go      | 94 +++++++++++++++++++++++++++++
 src/cmd/go/testdata/script/env_sanitize.txt |  5 ++
 src/cmd/go/testdata/script/work_env.txt     |  2 +-
 4 files changed, 158 insertions(+), 3 deletions(-)
 create mode 100644 src/cmd/go/internal/envcmd/env_test.go
 create mode 100644 src/cmd/go/testdata/script/env_sanitize.txt

It's the commit related to the same CL https://go-review.googlesource.com/c/go/+/488375

@liggitt
Copy link
Contributor

liggitt commented Aug 10, 2023

I'm pretty sure it's the issue that's already fixed at HEAD, but we need to:

  1. tag a release of github.com/quasilyte/go-ruleguard containing the fix
  2. update CI to use a version of golangci built against the go-ruleguard tag containing the fix
  3. update the nested dsl, rules, and cmd/gorules go.mod files to a version of go-ruleguard tag containing the fix – the failing module is still referencing github.com/quasilyte/go-ruleguard v0.3.19:
    github.com/quasilyte/go-ruleguard v0.3.19
cd rules && go test -v .
=== RUN   TestRules
    analysistest.go:295: error analyzing ruleguard@_/home/ldez/sources/golangci-lint/go-ruleguard/rules/testdata: load rules: parse rules file: typechecker error: diag.go:4:2: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")
--- FAIL: TestRules (0.34s)

see comment at #457 (comment)

@ldez
Copy link
Contributor Author

ldez commented Aug 10, 2023

see my PR #451

pohly added a commit to pohly/kubernetes that referenced this issue Aug 10, 2023
That release is the first one with official support for Go 1.21. go-ruleguard
must be >= 0.3.20 because of
quasilyte/go-ruleguard#449 with Go
1.21. golangci-lint itself doesn't depend on a recent enough release yet, so
this was done manually.

One downside of updating golangci-lint to v1.54.0 without also updating
logcheck is that golangci-lint now complains about logcheck using the old
plugin API:

    ERROR: level=warning msg="plugin: 'AnalyzerPlugin' plugins are deprecated, please use the new plugin signature: https://golangci-lint.run/contributing/new-linters/#create-a-plugin"
pohly added a commit to pohly/kubernetes that referenced this issue Aug 16, 2023
That release is the first one with official support for Go 1.21. go-ruleguard
must be >= 0.3.20 because of
quasilyte/go-ruleguard#449 with Go
1.21. golangci-lint itself doesn't depend on a recent enough release yet, so
this was done manually.
pohly added a commit to pohly/kubernetes that referenced this issue Aug 17, 2023
That release is the first one with official support for Go 1.21. go-ruleguard
must be >= 0.3.20 because of
quasilyte/go-ruleguard#449 with Go
1.21. golangci-lint itself doesn't depend on a recent enough release yet, so
this was done manually.
pohly added a commit to pohly/kubernetes that referenced this issue Aug 17, 2023
That release is the first one with official support for Go 1.21. go-ruleguard
must be >= 0.3.20 because of
quasilyte/go-ruleguard#449 with Go
1.21. golangci-lint itself doesn't depend on a recent enough release yet, so
this was done manually.
pohly added a commit to pohly/kubernetes that referenced this issue Aug 17, 2023
That release is the first one with official support for Go 1.21. go-ruleguard
must be >= 0.3.20 because of
quasilyte/go-ruleguard#449 with Go
1.21. golangci-lint itself doesn't depend on a recent enough release yet, so
this was done manually.

The new ginkgolinter finds some issues in tests in the release-1.27 branch that
were fixed on master, but not backported. These issues don't need to be fixed
in a release branch, therefore the ginkgolinter gets disabled.
pohly added a commit to pohly/kubernetes that referenced this issue Aug 17, 2023
That release is the first one with official support for Go 1.21. go-ruleguard
must be >= 0.3.20 because of
quasilyte/go-ruleguard#449 with Go
1.21. golangci-lint itself doesn't depend on a recent enough release yet, so
this was done manually.
maaoBit pushed a commit to maaoBit/kubernetes that referenced this issue Aug 20, 2024
That release is the first one with official support for Go 1.21. go-ruleguard
must be >= 0.3.20 because of
quasilyte/go-ruleguard#449 with Go
1.21. golangci-lint itself doesn't depend on a recent enough release yet, so
this was done manually.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants