Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gnovm/pkg/gnolang/garbage_collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
// TODO: more accurate benchmark.
const VisitCpuFactor = 8

// Visit visits all reachable associated values.
// Visits all reachable associated values.
// It is used primarily for GC.
// The caller must provide a callback visitor
// which knows how to break cycles, otherwise
Expand Down
14 changes: 14 additions & 0 deletions gnovm/pkg/gnolang/values_fill.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,28 @@
package gnolang

// StringValue, BigintValue, and BigdecValue are used only for constant expressions
// and are not collected by the GC (see garbage_collector.go).
// These types should never require DeepFill calls in correct code.
// In debug builds, we panic to catch potential misuse.

func (sv StringValue) DeepFill(store Store) Value {
if debug {
panic("StringValue.DeepFill should not be called - StringValue is only used for constant expressions")
}
return sv
}

func (biv BigintValue) DeepFill(store Store) Value {
if debug {
panic("BigintValue.DeepFill should not be called - BigintValue is only used for constant expressions")
}
return biv
}

func (bdv BigdecValue) DeepFill(store Store) Value {
if debug {
panic("BigdecValue.DeepFill should not be called - BigdecValue is only used for constant expressions")
}
return bdv
}
Comment on lines 8 to 27
Copy link
Member

Choose a reason for hiding this comment

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

Why not just make these panic?

Copy link
Contributor Author

@Davphla Davphla Nov 27, 2025

Choose a reason for hiding this comment

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

This may cause involuntary informational panic, so I think it is safer to have a potential optimization then an accidental panic.

As well, it break tests 2b0ae1d
Failing test: https://github.com/gnolang/gno/actions/runs/19742512315/job/56569620776#step:5:10

Should the failing tests be considered in that PR?

Copy link
Member

Choose a reason for hiding this comment

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

If it's not intended to happen, then it won't be "informational".

But by seeing the test results, it looks like either the behaviour is incorrect in the callers; or it would be incorrect to have a panic here


Expand Down
22 changes: 22 additions & 0 deletions gnovm/pkg/gnolang/values_fill_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package gnolang

import (
"testing"
)

func TestConstantValuesDeepFill(t *testing.T) {
sv := StringValue("test")
if result := sv.DeepFill(nil); result != sv {
t.Errorf("StringValue.DeepFill: expected %v, got %v", sv, result)
}

biv := BigintValue{}
if result := biv.DeepFill(nil); result != biv {
t.Errorf("BigintValue.DeepFill: expected %v, got %v", biv, result)
}

bdv := BigdecValue{}
if result := bdv.DeepFill(nil); result != bdv {
t.Errorf("BigdecValue.DeepFill: expected %v, got %v", bdv, result)
}
}
Loading