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

Invalid test cases for runtime.Marshaler #1501

Closed
movsb opened this issue Jul 5, 2020 · 3 comments · Fixed by #1502
Closed

Invalid test cases for runtime.Marshaler #1501

movsb opened this issue Jul 5, 2020 · 3 comments · Fixed by #1502

Comments

@movsb
Copy link
Contributor

movsb commented Jul 5, 2020

🐛 Bug Report

Taking address of any element in this slice produces the same value (because the element is an empty struct):

var marshalers [3]dummyMarshaler
specs := []struct {
opt runtime.ServeMuxOption
wantIn runtime.Marshaler
wantOut runtime.Marshaler
}{
{
opt: runtime.WithMarshalerOption(runtime.MIMEWildcard, &marshalers[0]),
wantIn: &marshalers[0],
wantOut: &marshalers[0],
},
{
opt: runtime.WithMarshalerOption("application/x-in", &marshalers[1]),
wantIn: &marshalers[1],
wantOut: &marshalers[0],
},
{
opt: runtime.WithMarshalerOption("application/x-out", &marshalers[2]),
wantIn: &marshalers[1],
wantOut: &marshalers[2],
},
}

So, these test cases almost won't work as expected.

As per the last line of golang spec:

A struct or array type has size zero if it contains no fields (or elements, respectively) that have a size greater than zero.

Two distinct zero-size variables may have the same address in memory.

To Reproduce

See the code at playground:

package main

import (
	"fmt"
)

type S struct {
}

type T struct {
	b bool
}

func main() {
	var s [3]S
	var t [3]T

	fmt.Printf("%p,%p,%p\n", &s[0], &s[1], &s[2])
	fmt.Printf("%p,%p,%p\n", &t[0], &t[1], &t[2])
}

Outputs:

0x58fd18,0x58fd18,0x58fd18
0xc00009400b,0xc00009400c,0xc00009400d

Your Environment

Any.

@johanbrandhorst
Copy link
Collaborator

Hi, thanks for the bug report! Do you have any ideas for how we could fix this? I think this is a pretty old part of the code.

yugui added a commit that referenced this issue Jul 6, 2020
Fixes #1501.

The expectations in the test was implemented wrong. But the wrong test
input has been hiding the wrong expectation, as explained in #1501.
@yugui
Copy link
Member

yugui commented Jul 6, 2020

Thank you for reporting this bug, @movsb. That was my fault.
Fortunately the actual implementation of marshaler registry was implemented as expected.

Do you have any ideas for how we could fix this? I think this is a pretty old part of the code.

Giving a non-zero size to the type could solve the issue. (#1502).
I found that the test spec I wrote was completely wrong and that the zero-length type has been hiding it. So I also corrected the spec.

yugui added a commit that referenced this issue Jul 6, 2020
Fixes #1501.

The expectations in the test was implemented wrong. But the wrong test
input has been hiding the wrong expectation, as explained in #1501.
@movsb
Copy link
Contributor Author

movsb commented Jul 6, 2020

Hi, @yugui , thanks for the correction.

Fortunately the actual implementation of marshaler registry was implemented as expected.

Unfortunately, I found this bug when I was trying to open another issue ( #1505 ) 😅

yugui added a commit that referenced this issue Jul 6, 2020
Fixes #1501.

The expectations in the test was implemented wrong. But the wrong test
input has been hiding the wrong expectation, as explained in #1501.
johanbrandhorst pushed a commit that referenced this issue Jul 7, 2020
Fixes #1501.

The expectations in the test was implemented wrong. But the wrong test
input has been hiding the wrong expectation, as explained in #1501.
yugui added a commit that referenced this issue Jul 7, 2020
Fixes #1501.

The expectations in the test was implemented wrong. But the wrong test
input has been hiding the wrong expectation, as explained in #1501.
johanbrandhorst pushed a commit that referenced this issue Jul 7, 2020
Fixes #1501.

The expectations in the test was implemented wrong. But the wrong test
input has been hiding the wrong expectation, as explained in #1501.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants