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

Map() does not panic on zero-initialized value #9025

Closed
mackjmr opened this issue Dec 1, 2023 · 3 comments · Fixed by #9038
Closed

Map() does not panic on zero-initialized value #9025

mackjmr opened this issue Dec 1, 2023 · 3 comments · Fixed by #9038
Labels
area:pdata pdata module related issues bug Something isn't working

Comments

@mackjmr
Copy link
Member

mackjmr commented Dec 1, 2023

Describe the bug
The following code will not cause a panic:

var test pcommon.Value

func main() {
	test.Map()
}

although the doc states:

Calling this function on zero-initialized Value will cause a panic.

Steps to reproduce

package main

import (
	"go.opentelemetry.io/collector/pdata/pcommon"
)

var test pcommon.Value

func main() {
	test.Map()
}

What did you expect to see?
A Panic.

What did you see instead?
No Panic.

What version did you use?

require go.opentelemetry.io/collector/pdata v1.0.0
@mackjmr mackjmr added the bug Something isn't working label Dec 1, 2023
@dmitryax
Copy link
Member

dmitryax commented Dec 1, 2023

It returns a zero-initialized map which will panic after being used. Unless the comment says it panics right away, it's close to be true :)

We may complicate the logic and make sure it panics right away, but I'm not sure it's worth it. Maybe just updating the comment is good enough. Something like

// Map returns the map value associated with this Value.
// If the function is called on zero-initialized Value or if the Type() is not ValueTypeMap 
// then it returns an invalid map. Note that using such map can cause panic.

@dmitryax dmitryax added the area:pdata pdata module related issues label Dec 1, 2023
@mackjmr
Copy link
Member Author

mackjmr commented Dec 4, 2023

Maybe just updating the comment is good enough.

Agreed that updating the comment should be good enough 👍

@dmitryax
Copy link
Member

dmitryax commented Dec 4, 2023

@mackjmr I updated all the comments in value.go #9038, but it might be the same issue for other structs. It'd be great if you can submit another issue or help with resolving them as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:pdata pdata module related issues bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants