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

ldez · 2023-07-14T16:40:12Z

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")
...

The text was updated successfully, but these errors were encountered:

quasilyte · 2023-07-17T08:00:32Z

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

quasilyte · 2023-07-17T08:03:08Z

See #450

ldez · 2023-07-17T10:46:16Z

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

ldez · 2023-08-02T17:22:39Z

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

@quasilyte can I help you?

ldez · 2023-08-08T15:35:05Z

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

liggitt · 2023-08-09T20:07:44Z

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 · 2023-08-09T20:11:51Z

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 · 2023-08-09T20:12:37Z

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 · 2023-08-09T20:14:19Z

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 · 2023-08-09T20:18:47Z

The link seems private.

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

cristaloleg · 2023-08-09T20:21:56Z

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

ldez · 2023-08-09T20:22:32Z

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

ldez · 2023-08-09T20:27:53Z

Maybe you can open a PR.

ldez · 2023-08-09T20:32:12Z

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

ldez · 2023-08-09T20:37:16Z

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 · 2023-08-09T20:38:05Z

opened #455 with the fix suggested by the go team

ldez · 2023-08-09T20:46:14Z

🤔 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 · 2023-08-09T20:49:29Z

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 · 2023-08-09T21:05:41Z

Maybe someone can reopen this issue.

ldez · 2023-08-10T03:29:54Z

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 <matloob@golang.org>
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 <gobot@golang.org>
    Reviewed-by: Michael Matloob <matloob@golang.org>
    Reviewed-by: Damien Neil <dneil@google.com>
    Run-TryBot: Michael Matloob <matloob@golang.org>
    Reviewed-by: Bryan Mills <bcmills@google.com>
    Reviewed-by: Quim Muntal <quimmuntal@gmail.com>

 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 · 2023-08-10T05:15:19Z

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

tag a release of github.com/quasilyte/go-ruleguard containing the fix
update CI to use a version of golangci built against the go-ruleguard tag containing the fix
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:

go-ruleguard/rules/go.mod

Line 6 in 067b930

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 · 2023-08-10T09:17:15Z

see my PR #451

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"

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.

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.

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.

ldez mentioned this issue Jul 14, 2023

go1.21 support golangci/golangci-lint#3922

Merged

ldez mentioned this issue Jul 17, 2023

chore(ci): use different version of Go #451

Merged

stefanb mentioned this issue Jul 20, 2023

go 1.21.0 Homebrew/homebrew-core#134468

Merged

89 tasks

ldez mentioned this issue Aug 8, 2023

go1.21 support golangci/golangci-lint#3933

Closed

2 tasks

liggitt mentioned this issue Aug 9, 2023

Make goenv parsing robust across go versions and OSes #455

Merged

liggitt mentioned this issue Aug 9, 2023

go1.21: load embedded ruleguard rules: rules/rules.go:13: can't load fmt go-critic/go-critic#1359

Closed

quasilyte closed this as completed in #455 Aug 9, 2023

cristaloleg reopened this Aug 9, 2023

cristaloleg closed this as completed in #451 Aug 10, 2023

pohly mentioned this issue Aug 10, 2023

golangci-lint update and support for Go 1.21 kubernetes/kubernetes#119860

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ldez commented Jul 14, 2023

quasilyte commented Jul 17, 2023

quasilyte commented Jul 17, 2023

ldez commented Jul 17, 2023

ldez commented Aug 2, 2023

ldez commented Aug 8, 2023 •

edited

liggitt commented Aug 9, 2023 •

edited

quasilyte commented Aug 9, 2023

liggitt commented Aug 9, 2023

liggitt commented Aug 9, 2023 •

edited

ldez commented Aug 9, 2023 •

edited

cristaloleg commented Aug 9, 2023

ldez commented Aug 9, 2023

ldez commented Aug 9, 2023

ldez commented Aug 9, 2023

ldez commented Aug 9, 2023

liggitt commented Aug 9, 2023

ldez commented Aug 9, 2023 •

edited

liggitt commented Aug 9, 2023

ldez commented Aug 9, 2023

ldez commented Aug 10, 2023 •

edited

liggitt commented Aug 10, 2023 •

edited

ldez commented Aug 10, 2023

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

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

Comments

ldez commented Jul 14, 2023

quasilyte commented Jul 17, 2023

quasilyte commented Jul 17, 2023

ldez commented Jul 17, 2023

ldez commented Aug 2, 2023

ldez commented Aug 8, 2023 • edited

liggitt commented Aug 9, 2023 • edited

quasilyte commented Aug 9, 2023

liggitt commented Aug 9, 2023

liggitt commented Aug 9, 2023 • edited

ldez commented Aug 9, 2023 • edited

cristaloleg commented Aug 9, 2023

ldez commented Aug 9, 2023

ldez commented Aug 9, 2023

ldez commented Aug 9, 2023

ldez commented Aug 9, 2023

liggitt commented Aug 9, 2023

ldez commented Aug 9, 2023 • edited

liggitt commented Aug 9, 2023

ldez commented Aug 9, 2023

ldez commented Aug 10, 2023 • edited

liggitt commented Aug 10, 2023 • edited

ldez commented Aug 10, 2023

ldez commented Aug 8, 2023 •

edited

liggitt commented Aug 9, 2023 •

edited

liggitt commented Aug 9, 2023 •

edited

ldez commented Aug 9, 2023 •

edited

ldez commented Aug 9, 2023 •

edited

ldez commented Aug 10, 2023 •

edited

liggitt commented Aug 10, 2023 •

edited