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.

Sign up for GitHub

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

Jump to bottom

reflect: fix incorrect nil copy #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

reflect: fix incorrect nil copy #5

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions deepcopy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,10 @@ func DisallowTypes(val ...any) DeepCopyOption {
// There are a few exceptions that may result in a deeply copied value not
// deeply equal (asserted by DeepEqual(dst, src)) to the source value:
//
// 1) Func values are still refer to the same function
// 2) Chan values are replaced by newly created channels
// 3) One-way Chan values (receive or read-only) values are still refer
// to the same channel
// 1. Func values are still refer to the same function
// 2. Chan values are replaced by newly created channels
// 3. One-way Chan values (receive or read-only) values are still refer
// to the same channel
//
// Note that while correct uses of DeepCopy do exist, they are not rare.
// The use of DeepCopy often indicates the copying object does not contain
Expand All @@ -115,7 +115,6 @@ func DeepCopy[T any](src T, opts ...DeepCopyOption) (dst T) {
}

func copyAny(src any, ptrs map[uintptr]any, copyConf *copyConfig) (dst any) {

if len(copyConf.disallowCopyTypes) != 0 {
for i := range copyConf.disallowCopyTypes {
if reflect.TypeOf(src) == copyConf.disallowCopyTypes[i] {
Expand Down Expand Up @@ -144,7 +143,7 @@ func copyAny(src any, ptrs map[uintptr]any, copyConf *copyConfig) (dst any) {
dst = copyArray(src, ptrs, copyConf)
case reflect.Map:
dst = copyMap(src, ptrs, copyConf)
case reflect.Ptr, reflect.UnsafePointer:
case reflect.Ptr:
Copy link
Member

Choose a reason for hiding this comment

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

这里很有意思，为什么UnsafePointer不能复制呢？

Copy link
Author

@alresvor alresvor Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

我看 UnsafePointer 不像 Ptr 类型一样可以推出它实际的类型，没办法用 reflect.New 新建一个(也许可以？)
大概只能像 Func 或 Int 类型一样，直接进行复制一份
不知道你是怎么决定这个值怎么复制的，我对反射不太了解，这里的判断如果不去掉，调用 copyPointer() 会报错的，所以就先去掉了 😄

Copy link
Member

Choose a reason for hiding this comment

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

这是个好问题,确实之前没有考虑过unsafe.pointer的实际类型..

是不是能够把 unsafe.pointer 直接当成整数处理呢?

Copy link
Author

Choose a reason for hiding this comment

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

个人认为是不可以的，因为是不正确的，例如 DeepCopy 把 UnsafePointer 当成 Int 复制一份:

a := []int{1, 2, 3}
ap := unsafe.Pointer(&a)
bp := DeepCopy(ap)
b := *(*[]int)(bp)
a[0] = 10
fmt.Println(a)
fmt.Println(b)
// [10 2 3]
// [10 2 3]

那么就会出现这种非常常见的错误，所以复制它毫无意义，因为没有复制底层数据
当然可以当作无事发生，因为这种类型还是比较少见的，不过这样就不算 DeepCopy 了，如果要保证正确性的话，应该忽略处理它，直接报错

不过也许还有其他解决办法，比如现在的问题是 unsafe.Pointer 无法确认真正类型
但是如果把 copyAny 改为泛型，那么遇到 unsafe.Pointer 类型，可以再进行一次转换调用
这样就可以当作一个正常的 Ptrinter 类型走 copyPointer 流程处理了，例如判断 src 是 unsafe.Pointer 后可以：
dst = copyPointer((*T)(src), ptrs, copyConf)
这只是一个想法，我没有验证过

Copy link
Author

Choose a reason for hiding this comment

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

唉 ，我最后说的方法应该不行，我想的太简单了...
因为 DeepCopy(unsafe.Porint(xxxx)) 时，参数就是 unsafe.Pointer 类型，根本没法推断，除非添加一个类型推断参数
但是如果复制结构字段时可不能指定类型，所以无法实现 😔

dst = copyPointer(src, ptrs, copyConf)
case reflect.Struct:
dst = copyStruct(src, ptrs, copyConf)
Expand Down Expand Up @@ -223,6 +222,9 @@ func copyPointer(x any, ptrs map[uintptr]any, copyConf *copyConfig) any {
if v.Kind() != reflect.Pointer {
panic(fmt.Errorf("reflect: internal error: must be a Pointer or Ptr; got %v", v.Kind()))
}
if v.IsNil() {
return x
}
addr := uintptr(v.UnsafePointer())
if dc, ok := ptrs[addr]; ok {
if copyConf.disallowCopyCircular {
Expand All @@ -233,12 +235,10 @@ func copyPointer(x any, ptrs map[uintptr]any, copyConf *copyConfig) any {
t := reflect.TypeOf(x)
dc := reflect.New(t.Elem())
ptrs[addr] = dc.Interface()
if !v.IsNil() {
item := copyAny(v.Elem().Interface(), ptrs, copyConf)
iv := reflect.ValueOf(item)
if iv.IsValid() {
dc.Elem().Set(reflect.ValueOf(item))
}
item := copyAny(v.Elem().Interface(), ptrs, copyConf)
iv := reflect.ValueOf(item)
if iv.IsValid() {
dc.Elem().Set(reflect.ValueOf(item))
}
return dc.Interface()
}
Expand Down Expand Up @@ -279,7 +279,6 @@ func copyChan(x any, ptrs map[uintptr]any, copyConf *copyConfig) any {
if !copyConf.disallowCopyBidirectionalChan {
dc = reflect.MakeChan(t, v.Cap()).Interface()
}
fallthrough
case reflect.SendDir, reflect.RecvDir:
dc = x
}
Expand Down