gno icon indicating copy to clipboard operation
gno copied to clipboard

fix(amino): panic when registering types with the same name

Open grepsuzette opened this issue 1 year ago • 4 comments

This adds some doc and tests to tm2/pkg/amino to address the following in amino_test.go:

fixed #2326

// XXX Test registering duplicate names or concrete types not in a package.

  • chore(docs): document frequent functions in tm2/pkg/amino
  • chore(amino): add some tests

To run tests

cd tm2/pkg/amino
go test -v --run=WithPanic\$

Tests have uncovered a potential bug however in TestDupNamesMustPanic. Opening an issue now to document this, with a possible fix.

	// The following does NOT panic, but it should.
	// assert.Panics(t, func() {
	// 	myPkg.WithTypes(
	// 		tests.EmptyStruct{}, "A",
	// 		tests.PrimitivesStruct{}, "B",
	// 		tests.ShortArraysStruct{}, "A", // Same name
	// 	)
	// })
Contributors' checklist...
  • [x] Added new tests, or not needed, or not feasible
  • [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • [x] Updated the official documentation or not needed
  • [x] No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • [ ] Added references to related issues and PRs
  • [x] Provided any useful hints for running manual tests
  • [ ] Added new benchmarks to generated graphs, if any. More info here.

grepsuzette avatar Jun 11 '24 11:06 grepsuzette

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:loudspeaker: Thoughts on this report? Let us know!

codecov[bot] avatar Jun 11 '24 11:06 codecov[bot]

Hello @grepsuzette . I do the following to clone your branch, merge master and run tests.

git clone https://github.com/grepsuzette/gno --branch amino_doc
cd gno
git remote add upstream https://github.com/gnolang/gno
git fetch upstream
git merge upstream/master
make test.components

It prints the test failure:

--- FAIL: TestStdlibs (0.01s)
    --- FAIL: TestStdlibs/regexp/syntax (0.00s)
panic: StaticBlock.Define2(byte) cannot change .V [recovered]
	panic: StaticBlock.Define2(byte) cannot change .V

goroutine 36 [running]:
testing.tRunner.func1.2({0x1049d1620, 0x1400010e090})
	/Users/jefft0/.asdf/installs/golang/1.22.5/go/src/testing/testing.go:1631 +0x1c4
testing.tRunner.func1()
	/Users/jefft0/.asdf/installs/golang/1.22.5/go/src/testing/testing.go:1634 +0x33c
panic({0x1049d1620?, 0x1400010e090?})
	/Users/jefft0/.asdf/installs/golang/1.22.5/go/src/runtime/panic.go:770 +0x124
github.com/gnolang/gno/gnovm/pkg/gnolang.(*StaticBlock).Define2(0x1400019a330, 0x0, {0x104752bef, 0x4}, {0x104acb050, 0x104faf280}, {{0x104acb050, 0x104faf280}, {0x104ac4990, 0x1400010e070}, ...})
	/Users/jefft0/temp/t2/gno/gnovm/pkg/gnolang/nodes.go:1848 +0x7f0
github.com/gnolang/gno/gnovm/pkg/gnolang.(*StaticBlock).Define(...)
	/Users/jefft0/temp/t2/gno/gnovm/pkg/gnolang/nodes.go:1788
github.com/gnolang/gno/gnovm/pkg/gnolang.UverseNode.func1(...)
	/Users/jefft0/temp/t2/gno/gnovm/pkg/gnolang/uverse.go:89
github.com/gnolang/gno/gnovm/pkg/gnolang.UverseNode()
	/Users/jefft0/temp/t2/gno/gnovm/pkg/gnolang/uverse.go:100 +0x470
github.com/gnolang/gno/gnovm/pkg/gnolang.Uverse()
	/Users/jefft0/temp/t2/gno/gnovm/pkg/gnolang/uverse.go:71 +0x28
github.com/gnolang/gno/gnovm/pkg/gnolang.InitStoreCaches({0x104ad8b88, 0x1400041a000})
	/Users/jefft0/temp/t2/gno/gnovm/pkg/gnolang/store.go:866 +0x228
github.com/gnolang/gno/gnovm/pkg/gnolang.NewStore(0x0, {0x104acb950, 0x1400010e010}, {0x104acb9a8, 0x14000128120})
	/Users/jefft0/temp/t2/gno/gnovm/pkg/gnolang/store.go:125 +0x128
command-line-arguments.TestStore({0x140004cc000, 0x5}, {0x140003ec780, 0xd}, {0x104ac1f08, 0x140004ca000}, {0x104ac1e68, 0x1400018e020}, {0x104ac1f28, 0x140004ca030}, ...)
	/Users/jefft0/temp/t2/gno/gnovm/tests/imports.go:426 +0x228
command-line-arguments.runPackageTest(0x140003e0680, {0x14000400000, 0x18}, {0x140003ec780, 0xd})
	/Users/jefft0/temp/t2/gno/gnovm/tests/package_test.go:74 +0x10c
command-line-arguments.TestStdlibs.func2(0x140003e0680)
	/Users/jefft0/temp/t2/gno/gnovm/tests/package_test.go:58 +0x58
testing.tRunner(0x140003e0680, 0x14000406cf0)
	/Users/jefft0/.asdf/installs/golang/1.22.5/go/src/testing/testing.go:1689 +0xec
created by testing.(*T).Run in goroutine 34
	/Users/jefft0/.asdf/installs/golang/1.22.5/go/src/testing/testing.go:1742 +0x318
FAIL	command-line-arguments	0.310s
FAIL
make[1]: *** [_test.gnolang.pkg1] Error 1
make: *** [test.components] Error 2

jefft0 avatar Oct 22 '24 09:10 jefft0

I don't know what went on there, @jefft0, but it seems to work now.

Anyway I fixed the bug involved with the text. Approving this PR, looking for another approval.

thehowl avatar Oct 22 '24 11:10 thehowl

Removed the review/triage-pending label because this is approved by thehowl.

jefft0 avatar Oct 22 '24 11:10 jefft0

This PR is stale because it has been open 3 months with no activity. Remove stale label or comment or this will be closed in 3 months.

github-actions[bot] avatar Jan 21 '25 01:01 github-actions[bot]

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • [ ] IGNORE the bot requirements for this PR (force green CI check)
  • [x] The pull request description provides enough details (checked by @thehowl)
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) 🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ 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
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: grepsuzette/gno)

Then

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

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🟢 At least 1 user(s) of the organization reviewed the pull request (with state "APPROVED")
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

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
The pull request description provides enough details

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
    └── 🟢 Not (🔴 Pull request author is user: dependabot[bot])

Can be checked by

  • team core-contributors

Gno2D2 avatar Jan 21 '25 14:01 Gno2D2