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

ast.Node shouldn't be used after COPY ( should regard COPY as MOVE ) #500

Open
AsterDY opened this issue Aug 12, 2023 · 0 comments
Open
Labels
documentation Improvements or additions to documentation known-issue This issue is known to us, we are working on it

Comments

@AsterDY
Copy link
Collaborator

AsterDY commented Aug 12, 2023

Background

Since ast.Node introduces lazy-load parsing mechanism, many APIs may change underlying pointer of a node. Therefore, once a node is passed-by-value to another place, the old one should never used --- in other words, Passing a node by value equals to the semantic of MOVE.

Reproducible codes

func TestNodeMove(t *testing.T) {
	old, _ := sonic.GetFromString(`{"a":1,"b":2}`)
	_ = old.Get("a")
	container := ast.NewObject([]ast.Pair{
		{Key: "data", Value: old}, // old is moved to container here
		{Key: "extra", Value: ast.NewString("hello")},
	})
	_, _ = container.MarshalJSON() // MarshalJSON() change underlying data in old
	_, err := old.Interface()      // thus old is invalid !!!
	if err != nil {
		t.Fatal(err)
	}
}

result

--- FAIL: TestNodeMove (0.00s)
    /ast_test.go:36: "Syntax error at index 13: eof\n\n\t{\"a\":1,\"b\":2}\n\t.............^\n"

How to resolve this problem if I still want to use old node?

The main idea is to UPDATE the old node's reference once you move it.
Given above example, if you still want to use the old node, you can change your code like below:

func TestNodeMoveValid(t *testing.T) {
	raw, _ := sonic.GetFromString(`{"a":1,"b":2}`)
	old := &raw
	_ = old.Get("a") // lazy-loaded "a" here

	container := ast.NewObject([]ast.Pair{
		{Key: "data", Value: *old}, // old is MOVED to container here
		{Key: "extra", Value: ast.NewString("hello")},
	})

	old = container.Get("data") // update the reference of old to container's "data"

	_, _ = container.MarshalJSON() // MarshalJSON() change underlying data in old, both "a" and "b" loaded

	_, err := old.Interface() // old is still valid now
	if err != nil {
		t.Fatal(err)
	}
}
@AsterDY AsterDY changed the title ast.Node shouldn't be used after Copy ( should regard it as Move ) ast.Node shouldn't be used after **Copy** ( should regard Copy as **Move** ) Aug 12, 2023
@AsterDY AsterDY changed the title ast.Node shouldn't be used after **Copy** ( should regard Copy as **Move** ) ast.Node shouldn't be used after COPY ( should regard COPY as MOVE ) Aug 12, 2023
@AsterDY AsterDY added documentation Improvements or additions to documentation known-issue This issue is known to us, we are working on it labels Aug 12, 2023
@AsterDY AsterDY pinned this issue Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation known-issue This issue is known to us, we are working on it
Projects
None yet
Development

No branches or pull requests

1 participant