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

reflect: StructOf allows creation of structs with invalid field names #20600

Closed
dsnet opened this Issue Jun 7, 2017 · 9 comments

Comments

Projects
None yet
6 participants
@dsnet
Member

dsnet commented Jun 7, 2017

Using Go1.8.3

t := reflect.StructOf([]reflect.StructField{
	reflect.StructField{Name: "", Type: reflect.TypeOf(0)},
	reflect.StructField{Name: "+", Type: reflect.TypeOf(0)},
})
fmt.Printf("%#v\n", reflect.New(t).Elem())

What did I see:
The creation of a struct with ridiculous field names.

struct { int; + int }{int:0, +:0}

What did I expect:
A panic of some sort.

@bradfitz bradfitz added the NeedsFix label Jun 7, 2017

@bradfitz bradfitz added this to the Go1.10 milestone Jun 7, 2017

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz
Member

bradfitz commented Jun 7, 2017

@pravj

This comment has been minimized.

Show comment
Hide comment
@pravj

pravj Jun 10, 2017

Contributor

If I'm allowed, I would like to work on it.

Right now, it's only checking for an empty field name.

...
if field.Name == "" {
        panic("reflect.StructOf: field " + strconv.Itoa(i) + " has no name")
}
...

According to the language spec, a field name should be an identifier.

identifier = letter { letter | unicode_digit } .

letter = unicode_letter | "_" .

An extra check can be applied to make sure the field name matches the correct form.

Contributor

pravj commented Jun 10, 2017

If I'm allowed, I would like to work on it.

Right now, it's only checking for an empty field name.

...
if field.Name == "" {
        panic("reflect.StructOf: field " + strconv.Itoa(i) + " has no name")
}
...

According to the language spec, a field name should be an identifier.

identifier = letter { letter | unicode_digit } .

letter = unicode_letter | "_" .

An extra check can be applied to make sure the field name matches the correct form.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Jun 10, 2017

Contributor

@pravj Please go ahead. Thanks.

Contributor

ianlancetaylor commented Jun 10, 2017

@pravj Please go ahead. Thanks.

@pravj

This comment has been minimized.

Show comment
Hide comment
@pravj

pravj Jun 12, 2017

Contributor

@ianlancetaylor,

I've implemented a function to verify the field name and tested it, but I'm facing an issue following the contribution guideline.

The git change command is reporting error because of a previous change by @rsc.

git-codereview: gofmt reported errors:
	reflect/all_test.go:5991:12: expected type, found '='

Sorry for starting the conversation here, as I wasn't able to make a change list because of this error.

Contributor

pravj commented Jun 12, 2017

@ianlancetaylor,

I've implemented a function to verify the field name and tested it, but I'm facing an issue following the contribution guideline.

The git change command is reporting error because of a previous change by @rsc.

git-codereview: gofmt reported errors:
	reflect/all_test.go:5991:12: expected type, found '='

Sorry for starting the conversation here, as I wasn't able to make a change list because of this error.

@pravj

This comment has been minimized.

Show comment
Hide comment
@pravj

pravj Jun 12, 2017

Contributor

Just to be clear, this file all_test.go is in my staged changes because I've written corresponding test for the field-name-verification in it.

Contributor

pravj commented Jun 12, 2017

Just to be clear, this file all_test.go is in my staged changes because I've written corresponding test for the field-name-verification in it.

@odeke-em

This comment has been minimized.

Show comment
Hide comment
@odeke-em

odeke-em Jun 12, 2017

Member

@pravj if you can go into your $GOPATH/bin and remove gofmt then try again I think it should work.
Seems like your gofmt in the bin directory is stale and needs to be rebuilt.

You can always rebuild with ./make.bash although that for some reason(but I was able to run it even before the rebuild), it didn't rebuild my gofmt so I built it like this:

$ cd $GOPATH/src/go.googlesource.com/go/src/cmd/gofmt && go build -o $GOPATH/bin/gofmt

and when ready run gofmt on your test, commit then you should be gucci to go!

Member

odeke-em commented Jun 12, 2017

@pravj if you can go into your $GOPATH/bin and remove gofmt then try again I think it should work.
Seems like your gofmt in the bin directory is stale and needs to be rebuilt.

You can always rebuild with ./make.bash although that for some reason(but I was able to run it even before the rebuild), it didn't rebuild my gofmt so I built it like this:

$ cd $GOPATH/src/go.googlesource.com/go/src/cmd/gofmt && go build -o $GOPATH/bin/gofmt

and when ready run gofmt on your test, commit then you should be gucci to go!

@pravj

This comment has been minimized.

Show comment
Hide comment
@pravj

pravj Jun 13, 2017

Contributor

@odeke-em, I'm still getting the same error as I enter the git change command. I have tried building gofmt again and again.

Also, the cloned source (that I'm working with) shouldn't be in the $GOPATH as suggested in the contribution guide.

You should checkout the Go source repo anywhere you want as long as it's outside of your $GOPATH

So I'm unable to digest the directory change you did to the source repo but inside $GOPATH.

Didn't expect this while doing my first contribution, getting nervous.

Contributor

pravj commented Jun 13, 2017

@odeke-em, I'm still getting the same error as I enter the git change command. I have tried building gofmt again and again.

Also, the cloned source (that I'm working with) shouldn't be in the $GOPATH as suggested in the contribution guide.

You should checkout the Go source repo anywhere you want as long as it's outside of your $GOPATH

So I'm unable to digest the directory change you did to the source repo but inside $GOPATH.

Didn't expect this while doing my first contribution, getting nervous.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Jun 13, 2017

Contributor

@pravj You need to build gofmt using the tip version of Go, not Go 1.8. The problem is that the future Go 1.9 adds a new language change: type aliases. You need to build a version of gofmt that uses the go/parse package from the future Go 1.9.

Contributor

ianlancetaylor commented Jun 13, 2017

@pravj You need to build gofmt using the tip version of Go, not Go 1.8. The problem is that the future Go 1.9 adds a new language change: type aliases. You need to build a version of gofmt that uses the go/parse package from the future Go 1.9.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Jun 13, 2017

CL https://golang.org/cl/45590 mentions this issue.

@gopherbot gopherbot closed this in 538b3a5 Jun 13, 2017

@dsnet dsnet modified the milestones: Go1.9, Go1.10 Jun 13, 2017

@golang golang locked and limited conversation to collaborators Jun 13, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.