-
Notifications
You must be signed in to change notification settings - Fork 388
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
fix: remove import cycles #3304
Open
n0izn0iz
wants to merge
79
commits into
gnolang:master
Choose a base branch
from
n0izn0iz:forbid-import-cycle
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
79 commits
Select commit
Hold shift + click to select a range
b8bf790
fix: prevent import cycles in stdlibs
n0izn0iz fd7bbc4
fix: remove import cycles
n0izn0iz 9be6649
chore: improve cycle detector
n0izn0iz b7ba5a9
chore: inject match string to break import cycle
n0izn0iz 920c49b
Merge branch 'master' into forbid-import-cycle
n0izn0iz c45a741
chore: revert fuzz move
n0izn0iz 360aa64
chore: fuzztesting -> testing
n0izn0iz d7c3d10
chore: regen
n0izn0iz c69fc43
Merge branch 'master' into forbid-import-cycle
n0izn0iz 096f8cd
fix: strconv tests
n0izn0iz 4517f0f
Merge branch 'master' into forbid-import-cycle
n0izn0iz ced3475
fix: math/rand, some math/bits, strings
n0izn0iz eff15b0
fix: typo
n0izn0iz 761ef83
chore: rename var
n0izn0iz ee265f5
fix: don't hide self imports
n0izn0iz 401d4e3
Merge branch 'master' into forbid-import-cycle
n0izn0iz 0a05f85
chore: add test case for self import
n0izn0iz d9f4d3a
chore: make genstd consider test files
n0izn0iz c6728c5
chore: make genstd consider test files
n0izn0iz 4374a39
fix(genstd): allow native injections in all packages and regen
n0izn0iz 284e56b
tmp
n0izn0iz 6c5d83e
feat: categorize imports
n0izn0iz d05c43b
Merge branch 'master' into imports-map
n0izn0iz 702e3d9
Merge branch 'master' into imports-map
n0izn0iz 1d17b9f
Merge branch 'master' into imports-map
n0izn0iz c4d82dc
Merge branch 'master' into imports-map
n0izn0iz 27bb085
Merge branch 'master' into imports-map
n0izn0iz 3e04787
chore: Merge remote-tracking branch 'origin/master' into imports-map
n0izn0iz a0dd0d8
chore: move code for easier review
n0izn0iz d2d043e
chore: pass fset to GetFileKind
n0izn0iz efda22d
fix: usage of packages.Imports
n0izn0iz fcd66a5
chore: don't expose SortImports
n0izn0iz 37d96ce
chore: error msg with loc
n0izn0iz e591002
Merge branch 'master' into imports-map
n0izn0iz 315b37f
Merge branch 'master' into imports-map
n0izn0iz cd91edb
Merge branch 'master' into imports-map
n0izn0iz c0998d7
chore: nit
n0izn0iz 41ddaa6
chore: revert merge fail
n0izn0iz 4cfda1e
chore: FileKindCompiled -> FileKindPackageSource
n0izn0iz 2100a57
chore: FileKindXtest -> FilKindXTest
n0izn0iz 0be8c69
chore: add FileKind doc
n0izn0iz 1fcf4e3
Merge branch 'master' into imports-map
n0izn0iz 3973f84
chore: Merge branch 'imports-map' into forbid-import-cycle
n0izn0iz 5c7e31b
fix: nocycles binary after upgrade
n0izn0iz 479145b
Merge branch 'master' into forbid-import-cycle
n0izn0iz 35aca6d
chore: use new util to split imports
n0izn0iz 22f1589
feat: correctly check cycles
n0izn0iz 5c4208f
chore: improve explainers
n0izn0iz 64beee7
feat: embed cycle test in actual test
n0izn0iz 5a8beef
chore: don't panic in test
n0izn0iz a3fc750
Merge branch 'master' into forbid-import-cycle
n0izn0iz 39def3e
chore: improve comment
n0izn0iz 1f7d1fc
chore: improve doc
n0izn0iz 0ebf4f5
Merge branch 'master' into imports-map
n0izn0iz 7c11caa
chore: revert pkg change
f7c34e0
chore: refacto test
f01b564
chore: refacto test 2
8ae5f73
chore: remove dev artifact
65fdad5
chore: revert genstd changes
f9e8c94
chore: revert genstd changes
d5ee04e
chore: revert comments changes
bf7ff6f
chore: revert more comment changes
8c7959a
chore: protect bits error values
57a2d6b
chore: revert multi_test changes
5c95bd1
chore: revert unneeded math rand changes
d98eea9
chore: remove uneeded change
288351b
chore: only implement matchString in testing context
4692891
chore: improve comment
6c06b7f
chore: regen
e720c60
chore: add comment about overlay imports
0490b9e
Merge branch 'master' into imports-map
n0izn0iz 2945568
chore: Merge remote-tracking branch 'origin/master' into imports-map
n0izn0iz 31983c8
Merge branch 'master' into imports-map
n0izn0iz 44444c0
chore: Merge branch 'imports-map' into forbid-import-cycle
n0izn0iz 13a5e72
Merge branch 'master' into forbid-import-cycle
n0izn0iz 43c95d4
chore: Merge remote-tracking branch 'origin/master' into forbid-impor…
n0izn0iz 6e1a134
chore: remove merge artifact
n0izn0iz 10ea5f4
Merge branch 'master' into forbid-import-cycle
n0izn0iz b0c1f9f
chore: trigger CI
n0izn0iz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
package md | ||
package md_test | ||
|
||
import ( | ||
"testing" | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
package faucet | ||
package faucet_test | ||
|
||
import ( | ||
"std" | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,212 @@ | ||
package examples_test | ||
|
||
import ( | ||
"fmt" | ||
"io/fs" | ||
"os" | ||
pathlib "path" | ||
"path/filepath" | ||
"slices" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/gnolang/gno/gnovm" | ||
"github.com/gnolang/gno/gnovm/pkg/gnoenv" | ||
"github.com/gnolang/gno/gnovm/pkg/gnomod" | ||
"github.com/gnolang/gno/gnovm/pkg/packages" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
var injectedTestingLibs = []string{"encoding/json", "fmt", "os", "internal/os_test"} | ||
|
||
// TestNoCycles checks that there is no import cycles in stdlibs and non-draft examples | ||
func TestNoCycles(t *testing.T) { | ||
// find stdlibs | ||
gnoRoot := gnoenv.RootDir() | ||
pkgs, err := listPkgs(gnomod.Pkg{ | ||
Dir: filepath.Join(gnoRoot, "gnovm", "stdlibs"), | ||
Name: "", | ||
}) | ||
require.NoError(t, err) | ||
|
||
// find examples | ||
examples, err := gnomod.ListPkgs(filepath.Join(gnoRoot, "examples")) | ||
require.NoError(t, err) | ||
for _, example := range examples { | ||
if example.Draft { | ||
continue | ||
} | ||
examplePkgs, err := listPkgs(example) | ||
require.NoError(t, err) | ||
pkgs = append(pkgs, examplePkgs...) | ||
} | ||
|
||
// detect cycles | ||
visited := make(map[string]bool) | ||
for _, p := range pkgs { | ||
require.NoError(t, detectCycles(p, pkgs, visited)) | ||
} | ||
} | ||
|
||
// detectCycles detects import cycles | ||
// | ||
// We need to check | ||
// 3 kinds of nodes | ||
// | ||
// - normal pkg: compiled source | ||
// | ||
// - xtest pkg: external test source (include xtests and filetests), can be treated as their own package | ||
// | ||
// - test pkg: embedded test sources, | ||
// these should not have their corresponding normal package in their dependencies tree | ||
// | ||
// The tricky thing is that we need to split test sources and normal source | ||
// while not considering them as distincitive packages. | ||
// Otherwise we will have false positive for example if we have these edges: | ||
// | ||
// - foo_pkg/foo_test.go imports bar_pkg | ||
// | ||
// - bar_pkg/bar_test.go import foo_pkg | ||
// | ||
// In go, the above example is allowed | ||
// but the following is not | ||
// | ||
// - foo_pkg/foo.go imports bar_pkg | ||
// | ||
// - bar_pkg/bar_test.go imports foo_pkg | ||
func detectCycles(root testPkg, pkgs []testPkg, visited map[string]bool) error { | ||
// check cycles in package's sources | ||
stack := []string{} | ||
if err := visitPackage(root, pkgs, visited, stack); err != nil { | ||
return fmt.Errorf("pkgsrc import: %w", err) | ||
} | ||
// check cycles in external tests' dependencies we might have missed | ||
if err := visitImports([]packages.FileKind{packages.FileKindXTest, packages.FileKindFiletest}, root, pkgs, visited, stack); err != nil { | ||
return fmt.Errorf("xtest import: %w", err) | ||
} | ||
|
||
// check cycles in tests' imports by marking the current package as visited while visiting the tests' imports | ||
// we also consider PackageSource imports here because tests can call package code | ||
visited = map[string]bool{root.PkgPath: true} | ||
stack = []string{root.PkgPath} | ||
if err := visitImports([]packages.FileKind{packages.FileKindPackageSource, packages.FileKindTest}, root, pkgs, visited, stack); err != nil { | ||
return fmt.Errorf("test import: %w", err) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// visitImports resolves and visits imports by kinds | ||
func visitImports(kinds []packages.FileKind, root testPkg, pkgs []testPkg, visited map[string]bool, stack []string) error { | ||
for _, imp := range root.Imports.Merge(kinds...) { | ||
if slices.Contains(injectedTestingLibs, imp.PkgPath) { | ||
continue | ||
} | ||
idx := slices.IndexFunc(pkgs, func(p testPkg) bool { return p.PkgPath == imp.PkgPath }) | ||
if idx == -1 { | ||
return fmt.Errorf("import %q not found for %q tests", imp.PkgPath, root.PkgPath) | ||
} | ||
if err := visitPackage(pkgs[idx], pkgs, visited, stack); err != nil { | ||
return fmt.Errorf("test import error: %w", err) | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// visitNode visits a package and its imports recursively. It only considers imports in PackageSource | ||
func visitPackage(pkg testPkg, pkgs []testPkg, visited map[string]bool, stack []string) error { | ||
if slices.Contains(stack, pkg.PkgPath) { | ||
return fmt.Errorf("cycle detected: %s -> %s", strings.Join(stack, " -> "), pkg.PkgPath) | ||
} | ||
if visited[pkg.PkgPath] { | ||
return nil | ||
} | ||
|
||
visited[pkg.PkgPath] = true | ||
stack = append(stack, pkg.PkgPath) | ||
|
||
if err := visitImports([]packages.FileKind{packages.FileKindPackageSource}, pkg, pkgs, visited, stack); err != nil { | ||
return err | ||
} | ||
|
||
return nil | ||
} | ||
|
||
type testPkg struct { | ||
Dir string | ||
PkgPath string | ||
Imports packages.ImportsMap | ||
} | ||
|
||
// listPkgs lists all packages in rootMod | ||
func listPkgs(rootMod gnomod.Pkg) ([]testPkg, error) { | ||
res := []testPkg{} | ||
rootDir := rootMod.Dir | ||
visited := map[string]struct{}{} | ||
if err := fs.WalkDir(os.DirFS(rootDir), ".", func(path string, d fs.DirEntry, err error) error { | ||
if err != nil { | ||
return err | ||
} | ||
if d.IsDir() { | ||
return nil | ||
} | ||
if !strings.HasSuffix(d.Name(), ".gno") { | ||
return nil | ||
} | ||
subPath := filepath.Dir(path) | ||
dir := filepath.Join(rootDir, subPath) | ||
if _, ok := visited[dir]; ok { | ||
return nil | ||
} | ||
visited[dir] = struct{}{} | ||
|
||
subPkgPath := pathlib.Join(rootMod.Name, subPath) | ||
|
||
pkg := testPkg{ | ||
Dir: dir, | ||
PkgPath: subPkgPath, | ||
} | ||
|
||
memPkg, err := readPkg(pkg.Dir, pkg.PkgPath) | ||
if err != nil { | ||
return fmt.Errorf("read pkg %q: %w", pkg.Dir, err) | ||
} | ||
pkg.Imports, err = packages.Imports(memPkg, nil) | ||
if err != nil { | ||
return fmt.Errorf("list imports of %q: %w", memPkg.Path, err) | ||
} | ||
|
||
res = append(res, pkg) | ||
return nil | ||
}); err != nil { | ||
return nil, fmt.Errorf("walk dirs at %q: %w", rootDir, err) | ||
} | ||
return res, nil | ||
} | ||
|
||
// readPkg reads the sources of a package. It includes all .gno files but ignores the package name | ||
func readPkg(dir string, pkgPath string) (*gnovm.MemPackage, error) { | ||
list, err := os.ReadDir(dir) | ||
if err != nil { | ||
return nil, err | ||
} | ||
memPkg := &gnovm.MemPackage{Path: pkgPath} | ||
for _, entry := range list { | ||
fpath := filepath.Join(dir, entry.Name()) | ||
if !strings.HasSuffix(fpath, ".gno") { | ||
continue | ||
} | ||
fname := filepath.Base(fpath) | ||
bz, err := os.ReadFile(fpath) | ||
if err != nil { | ||
return nil, err | ||
} | ||
memPkg.Files = append(memPkg.Files, | ||
&gnovm.MemFile{ | ||
Name: fname, | ||
Body: string(bz), | ||
}) | ||
} | ||
return memPkg, nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,12 @@ | ||
package packages | ||
package packages_test | ||
|
||
import ( | ||
"os" | ||
"path/filepath" | ||
"testing" | ||
|
||
"github.com/gnolang/gno/gnovm/pkg/gnolang" | ||
. "github.com/gnolang/gno/gnovm/pkg/packages" | ||
"github.com/stretchr/testify/require" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think this is necessary; we should avoid dot imports unless we have weird cyclical imports in tests like the Go stdlib does. |
||
) | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
package ed25519 | ||
package ed25519_test | ||
|
||
import ( | ||
"crypto/ed25519" | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
package sha256 | ||
package sha256_test | ||
|
||
import ( | ||
"crypto/sha256" | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
package binary | ||
package binary_test | ||
|
||
import ( | ||
"bytes" | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
// Copyright 2020 The Go Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style | ||
// license that can be found in the LICENSE file. | ||
|
||
package io | ||
|
||
// exported for test | ||
var ErrInvalidWrite = errInvalidWrite | ||
var ErrWhence = errWhence | ||
var ErrOffset = errOffset |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this not be a check in
gno lint
instead, which is already run on all ofexamples
?