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

Fix Embedded Pointer #358

Merged
merged 2 commits into from Oct 2, 2018

Conversation

Projects
None yet
2 participants
@mathewbyrne
Collaborator

mathewbyrne commented Oct 1, 2018

Fixes the first part of #356

codegen.findField was not accounting for anonymous pointer types and was generating a redundant resolver. This change fixes this so that these pointers are unwrapped and the element type is considered recursively as a match for the field.

fieldType := field.Type()
if ptr, ok := fieldType.(*types.Pointer); ok {
fieldType = ptr.Elem()

This comment has been minimized.

@vektah

vektah Oct 1, 2018

Collaborator

what happens when the embedded pointer is null? Worth adding a test?

@vektah

vektah Oct 1, 2018

Collaborator

what happens when the embedded pointer is null? Worth adding a test?

This comment has been minimized.

@mathewbyrne

mathewbyrne Oct 1, 2018

Collaborator

Yep, good question, nothing will happen here. But I would expect a runtime panic. What do you think we should do?

@mathewbyrne

mathewbyrne Oct 1, 2018

Collaborator

Yep, good question, nothing will happen here. But I would expect a runtime panic. What do you think we should do?

This comment has been minimized.

@vektah

vektah Oct 1, 2018

Collaborator

can it return null and do the normal error bubbling dance?

@vektah

vektah Oct 1, 2018

Collaborator

can it return null and do the normal error bubbling dance?

This comment has been minimized.

@mathewbyrne

mathewbyrne Oct 1, 2018

Collaborator

Yeah ok, I'll come up with a test. Might involve slightly more messing around here since we'd need to know if it was a pointer that we matched to in template.

@mathewbyrne

mathewbyrne Oct 1, 2018

Collaborator

Yeah ok, I'll come up with a test. Might involve slightly more messing around here since we'd need to know if it was a pointer that we matched to in template.

This comment has been minimized.

@mathewbyrne

mathewbyrne Oct 1, 2018

Collaborator

Ok, after reviewing this a bit I think this is going to be a bigger change. Embeds can be pathological when they are more than a single embed deep with a mix of pointers thrown in.

I think for now we merge this and maybe log that as a separate issue.

@mathewbyrne

mathewbyrne Oct 1, 2018

Collaborator

Ok, after reviewing this a bit I think this is going to be a bigger change. Embeds can be pathological when they are more than a single embed deep with a mix of pointers thrown in.

I think for now we merge this and maybe log that as a separate issue.

@vektah

vektah approved these changes Oct 2, 2018

LGTM

@mathewbyrne mathewbyrne merged commit b836a97 into master Oct 2, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@vektah vektah deleted the fix-embedded-pointer branch Oct 2, 2018

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