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

Fix copying TypeParams #5

Merged
merged 1 commit into from
Jan 21, 2023
Merged

Fix copying TypeParams #5

merged 1 commit into from
Jan 21, 2023

Conversation

samuel
Copy link
Contributor

@samuel samuel commented Jan 20, 2023

There was a bug where the TypeParams weren't being properly copied but rather continuing the use the source value.

The code:

cp := *x
if typeParams := typeparams.ForFuncType(x); typeParams != nil {
	*typeparams.ForFuncType(&cp) = *FieldList(typeParams)
}

cp.TypeParams which typeparams.ForFyncType returns is still x.TypeParams. Then the *typeparams.ForFuncType(&cp) overwrites it a copy thus modifying the original slice.

This issue causes a data race which showed up in golangci-lint and often caused a panic.

The only way around this I found was to have copies of the function for different versions of Go since the typeparam package doesn't have a way to set the TypeParams field.

There was a bug where the TypeParams weren't being properly copied but
rather continuing the use the source value.

The code:

```
cp := *x
if typeParams := typeparams.ForFuncType(x); typeParams != nil {
	*typeparams.ForFuncType(&cp) = *FieldList(typeParams)
}
```

cp.TypeParams which typeparams.ForFyncType returns is still
x.TypeParams. Then the `*typeparams.ForFuncType(&cp)` overwrites it a
copy thus modifying the original slice.

This issue causes a data race which showed up in golangci-lint and often
caused a panic.

The only way around this I found was to have copies of the function for
different versions of Go since the typeparam package doesn't have a way
to set the TypeParams field.
Copy link
Member

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@cristaloleg
Copy link
Member

Thanks for the PR! Not sure should we care about older Go versions but as you've fixed let's keep it.

@quasilyte quasilyte merged commit a4c4beb into go-toolsmith:master Jan 21, 2023
@samuel samuel deleted the fix-func-typeparams branch January 21, 2023 09:11
@ldez
Copy link

ldez commented Feb 3, 2023

hello @quasilyte and @cristaloleg do you plan to create a release with the fix in the following days?

@cristaloleg
Copy link
Member

Thanks @quasilyte

@ldez
Copy link

ldez commented Feb 3, 2023

thank you @quasilyte ❤️

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 this pull request may close these issues.

None yet

4 participants