-
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
88
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 12 commits
Commits
Show all changes
88 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 d6fa81c
chore: Merge remote-tracking branch 'origin/master' into forbid-impor…
n0izn0iz 8728744
chore: don't use dot import
n0izn0iz c87d4dd
chore: unexport printtrie
n0izn0iz 348f34b
chore: make native matchString thread-safe
n0izn0iz 04818d8
Merge branch 'master' into forbid-import-cycle
n0izn0iz 784d508
chore: add todo about moving the cycle detector in gno lint
n0izn0iz ebc87af
chore: Merge remote-tracking branch 'origin/master' into forbid-impor…
n0izn0iz ac3a38d
Merge branch 'master' into forbid-import-cycle
thehowl cbeb758
Merge branch 'master' into forbid-import-cycle
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 |
---|---|---|
@@ -0,0 +1,146 @@ | ||
package main | ||
|
||
import ( | ||
"fmt" | ||
"io/fs" | ||
"os" | ||
"path/filepath" | ||
"strings" | ||
|
||
"github.com/gnolang/gno/gnovm" | ||
"github.com/gnolang/gno/gnovm/pkg/gnoenv" | ||
"github.com/gnolang/gno/gnovm/pkg/gnolang" | ||
"github.com/gnolang/gno/gnovm/pkg/gnomod" | ||
"github.com/gnolang/gno/gnovm/pkg/packages" | ||
) | ||
|
||
func main() { | ||
// set to true to dump the final pkg list | ||
verbose := true | ||
|
||
// find stdlibs | ||
libs := []string{} | ||
gnoRoot := gnoenv.RootDir() | ||
stdlibsDir := filepath.Join(gnoRoot, "gnovm", "stdlibs") | ||
fs.WalkDir(os.DirFS(stdlibsDir), ".", func(path string, d fs.DirEntry, err error) error { | ||
if !d.IsDir() { | ||
return nil | ||
} | ||
if path == "." { | ||
return nil | ||
} | ||
libs = append(libs, path) | ||
return nil | ||
}) | ||
|
||
// read stdlibs | ||
pl := gnomod.PkgList{} | ||
for _, lib := range libs { | ||
memPkg := gnolang.MustReadMemPackage(filepath.Join(stdlibsDir, lib), lib) | ||
pkg, xpkg, err := splitMemPackage(memPkg) | ||
if err != nil { | ||
panic(fmt.Errorf("split %q: %w", lib, err)) | ||
} | ||
{ | ||
imports, err := packages.Imports(pkg) | ||
if err != nil { | ||
panic(fmt.Errorf("read %q: %w", lib, err)) | ||
} | ||
pl = append(pl, gnomod.Pkg{ | ||
Dir: "", | ||
Name: lib, | ||
Imports: imports, | ||
}) | ||
} | ||
if !xpkg.IsEmpty() { | ||
imports, err := packages.Imports(xpkg) | ||
if err != nil { | ||
panic(fmt.Errorf("read %q: %w", lib, err)) | ||
} | ||
pl = append(pl, gnomod.Pkg{ | ||
Dir: "", | ||
Name: "_test_" + lib, | ||
Imports: imports, | ||
}) | ||
} | ||
} | ||
|
||
// load all examples | ||
examples, err := gnomod.ListPkgs(filepath.Join(gnoRoot, "examples")) | ||
if err != nil { | ||
panic(fmt.Errorf("load examples: %w", err)) | ||
} | ||
for _, example := range examples { | ||
if example.Draft { | ||
continue | ||
} | ||
pkgPath := example.Name | ||
memPkg := gnolang.MustReadMemPackage(example.Dir, example.Name) | ||
pkg, xpkg, err := splitMemPackage(memPkg) | ||
if err != nil { | ||
panic(fmt.Errorf("split %q: %w", pkgPath, err)) | ||
} | ||
{ | ||
imports, err := packages.Imports(pkg) | ||
if err != nil { | ||
panic(fmt.Errorf("read %q: %w", pkgPath, err)) | ||
} | ||
pl = append(pl, gnomod.Pkg{ | ||
Dir: example.Dir, | ||
Name: pkgPath, | ||
Imports: imports, | ||
}) | ||
} | ||
if !xpkg.IsEmpty() { | ||
imports, err := packages.Imports(xpkg) | ||
if err != nil { | ||
panic(fmt.Errorf("read %q: %w", pkgPath, err)) | ||
} | ||
pl = append(pl, gnomod.Pkg{ | ||
Dir: example.Dir, | ||
Name: "_test_" + pkgPath, | ||
Imports: imports, | ||
}) | ||
} | ||
} | ||
|
||
// detect import cycles | ||
if _, err := pl.Sort(); err != nil { | ||
panic(err) | ||
} | ||
|
||
if verbose { | ||
for _, p := range pl { | ||
fmt.Println(p.Name) | ||
} | ||
} | ||
} | ||
|
||
func splitMemPackage(pkg *gnovm.MemPackage) (*gnovm.MemPackage, *gnovm.MemPackage, error) { | ||
corePkg := gnovm.MemPackage{ | ||
Name: pkg.Name, | ||
Path: pkg.Path, | ||
} | ||
xtestPkg := gnovm.MemPackage{ | ||
Name: pkg.Name + "_test", | ||
Path: pkg.Path, | ||
} | ||
|
||
for _, file := range pkg.Files { | ||
if !strings.HasSuffix(file.Name, "_test.gno") { | ||
corePkg.Files = append(corePkg.Files, file) | ||
continue | ||
} | ||
pkgName, err := packages.FilePackageName(file.Name, file.Body) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("get package name in file %q: %w", file.Name, err) | ||
} | ||
if !strings.HasSuffix(pkgName, "_test") { | ||
corePkg.Files = append(corePkg.Files, file) | ||
continue | ||
} | ||
xtestPkg.Files = append(xtestPkg.Files, file) | ||
} | ||
|
||
return &corePkg, &xtestPkg, nil | ||
} |
thehowl marked this conversation as resolved.
Show resolved
Hide resolved
|
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
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 |
---|---|---|
@@ -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.
for stdlibs, instead, i think you could change
misc/genstd
(note: this binary as well, if it were to stay, would also have to be misc/), by having its import order generator also consider test filesThere 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.
Indeed, when I cherry-pick c6728c5 on master, I get:
I like having code that is dedicated to detecting any cycles in stdlibs and examples though, I'll move the
nocycles
code into a test somewhere I think when I get to the polishing step on this PRThere 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.
moved the test to
examples/no_cycles_test.go
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.
regarding genstd changes:
actually finding a single global order while including all test files is not possible, consider the following imports (which are allowed in go):
foopkg
/foo_test.go importsbarpkg
barpkg
/bar_test.go importsfoopkg
examining each packages tests deps will yield a different order:
foopkg
tests:barpkg
foopkg
barpkg
tests:foopkg
barpkg
we could fill a
map[string][]string
to get the testing order for each lib but I'm not sure this is really helpful, do you want me to add this?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.
Is this PR not meant to find that order?
The order should exist when excluding the Xtest files, no?
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.
no, it only exists when you include solely the
PackageSource
files, the example above is with normal test filesThere 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.
The go stdlib makes it possible to find an order by putting all deps which would lead to circular dependencies in Xtest files. Could we not also do that, and is that not what you did in this PR itself?
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.
No, this PR aims to remove cycles when you follow the normal golang rules. Mainly the source of cycles were due to the dependencies of the
testing
package which must only haveXTest
files otherwise they have a cycle withtesting
since they must import it.I'm not sure the golang stdlibs have a global order if you consider all
Test
sources, would need to checkThere is tests in a few libraries (in gno and go) that test the lib internals and can't be direcly converted to
XTest