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

fix: remove import cycles #3304

Draft
wants to merge 52 commits into
base: master
Choose a base branch
from

Conversation

n0izn0iz
Copy link
Contributor

@n0izn0iz n0izn0iz commented Dec 8, 2024

  • add test in examples/no_cycles_test.go to detect import cycles in stdlibs and examples
  • remove import cycles

Depends on #3323

Go never allows import cycles. Our stdlibs have a lot of import cycles, and some examples import self which is not allowed in golang either.
Keeping support for import cycles in stdlib and importing self will require a lot of hacky and weird logic in generic package loading code so I try to tackle this first.

TODO:

  • fix tests
  • check cycles with the test stdlibs overlay applied

@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related labels Dec 8, 2024
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 8, 2024

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: n0izn0iz/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

Copy link

codecov bot commented Dec 8, 2024

Codecov Report

Attention: Patch coverage is 76.92308% with 24 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gnovm/pkg/packages/imports.go 78.04% 8 Missing and 1 partial ⚠️
misc/genstd/genstd.go 33.33% 4 Missing and 2 partials ⚠️
gnovm/pkg/gnomod/pkg.go 66.66% 4 Missing and 1 partial ⚠️
gnovm/stdlibs/testing/testing.go 62.50% 2 Missing and 1 partial ⚠️
gnovm/pkg/doc/dirs.go 66.66% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these cases should be catchable already by the cyclic-import detector in the store; can you take a look at what's going on there?

Copy link
Contributor Author

@n0izn0iz n0izn0iz Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by the cyclic-import detector in the store, the Store in gnovm/pkg/test/imports.go does not seem to have cycle detection

FYI the self-import in faucet had been there for 2 years

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah no apologies I thought about it for two seconds and you're right (because the import will not consider any _test.go files, correctly so)

the store has cyclic import detection, see pkg/gnolang/store.go:

// Gets package from cache, or loads it from baseStore, or gets it from package getter.
func (ds *defaultStore) GetPackage(pkgPath string, isImport bool) *PackageValue {
// helper to detect circular imports
if isImport {
if slices.Contains(ds.current, pkgPath) {
panic(fmt.Sprintf("import cycle detected: %q (through %v)", pkgPath, ds.current))
}
ds.current = append(ds.current, pkgPath)
defer func() {
ds.current = ds.current[:len(ds.current)-1]
}()
}

// find stdlibs
libs := []string{}
gnoRoot := gnoenv.RootDir()
stdlibsDir := filepath.Join(gnoRoot, "gnovm", "stdlibs")
Copy link
Member

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 files

Copy link
Contributor Author

@n0izn0iz n0izn0iz Dec 10, 2024

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:

go run github.com/gnolang/gno/misc/genstd
panic: cyclical package initialization on "bytes" (bytes -> io -> bytes)

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 PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to examples/no_cycles_test.go

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants