cogentcore / core

A free and open source framework for building powerful, fast, and elegant 2D and 3D apps that run on macOS, Windows, Linux, iOS, Android, and the Web with a single pure Go codebase, allowing you to Code Once, Run Everywhere.
http://cogentcore.org/core
BSD 3-Clause "New" or "Revised" License
1.31k stars 71 forks source link

fix https://github.com/cogentcore/core/issues/917 and #916 #918

Closed ddkwork closed 4 months ago

ddkwork commented 4 months ago

well done, please check pr.

ddkwork commented 4 months ago

see here

// IsValid reports whether v represents a value.
// It returns false if v is the zero Value.
// If IsValid returns false, all other methods except String panic.
// Most functions and methods never return an invalid Value.
// If one does, its documentation states the conditions explicitly.
func (v Value) IsValid() bool {
    return v.flag != 0
}

// IsZero reports whether v is the zero value for its type.
// It panics if the argument is invalid.                                  ----------------> see here
func (v Value) IsZero() bool {

In reflection code, I suggest calling IsValid before calling IsZero, because only with a value can we determine whether the value is zero. For example, for a variable, define it first, and if it is defined, IsValid is OK. Then we can determine whether the variable has been initialized or assigned, and only then can we call IsZero, because reflection allows variables to be undefined beforehand. This feels a bit like the saying "chicken lays eggs, egg lays chicken", where there is a chicken first and then an egg. If there is a chicken first, IsValid is okay. At this point, if the chicken does not lay eggs, then IsZero is also okay.

kkoreilly commented 4 months ago

I am planning to fix these issues by implementing some solutions based on your ideas, but your ideas by themselves do not fully fix the problems. Thanks for your work on this!

ddkwork commented 4 months ago

okay

---Original--- From: @.> Date: Thu, Feb 29, 2024 12:11 PM To: @.>; Cc: @.>;"State @.>; Subject: Re: [cogentcore/core] fixhttps://github.com/cogentcore/core/issues/917 and #916 (PR #918)

I am planning to fix these issues by implementing some solutions based on your ideas, but your ideas by themselves do not fully fix the problems. Thanks for your work on this!

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: @.***>

ddkwork commented 4 months ago

I am planning to fix these issues by implementing some solutions based on your ideas, but your ideas by themselves do not fully fix the problems. Thanks for your work on this!

I summarized the problem I have been mentioning in the discussion forum, n more panic is the process like this: the example directory constructs the structure pointer variable is passed to the tree and table and other widgets, because the demo we don't care about instantiating the value check, resulting in a series of subsequent panics, which also verifies what you said earlier "the interface asserts that panic never happens", why is this happening? Because in real projects, we will be very strict to check whether all kinds of things instantiate each parameter or field, slice members, etc., because there is the correct value filling, it naturally can't panic. Guess the demo data transfer check is not strict, when it comes to ki to reflect it, part of the code implementation of ki needs new a reflected value and then fill in the value from the demo, at this time the demo parameter is nil, then the problem seen in the two issues has occurred, ctx is nil, call iszero, but it isvalid has not passed, the slice is out of bounds is similar, ignore the interface assertion I feel is the culprit, In the state where the demo parameters mentioned above are like that, it is difficult to assert that there is no panic. Then we all know how to avoid it, but in the real project, there are still some parameters that should not be inexplicably entered, such as the packet capture program, when the packet capture program is run for the first time, the agent is not set up, the table is empty at this time, if the hand is cheap at this time to click on the table header sorting, panic will happen, in addition to this kind of example, there are many more, so my final suggestion is: rectify the interface assertion.

kkoreilly commented 4 months ago

Again, it is much nicer to fix this at the high level instead of adding nil checks to everything. I will continue to work on fixing the crashes in the context of #916, #917, and #679, and if I can not fix them at the high level, then I will follow your suggestions.

ddkwork commented 4 months ago

okay

---Original--- From: @.> Date: Thu, Feb 29, 2024 13:08 PM To: @.>; Cc: @.>;"State @.>; Subject: Re: [cogentcore/core] fixhttps://github.com/cogentcore/core/issues/917 and #916 (PR #918)

Again, it is much nicer to fix this at the high level instead of adding nil checks to everything. I will continue to work on fixing the crashes in the context of #916, #917, and #679, and if I can not fix them at the high level, then I will follow your suggestions.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: @.***>

ddkwork commented 4 months ago

Screenshot_20240229_140222_com.github.android.jpg

this can changed to assert.True() Nice work!

kkoreilly commented 4 months ago

Thanks! It can't be assert.True, since we sometimes want it to be false (that is why we have test.ok in that test).

ddkwork commented 4 months ago

Okay

---Original--- From: @.> Date: Thu, Feb 29, 2024 14:06 PM To: @.>; Cc: @.>;"State @.>; Subject: Re: [cogentcore/core] fixhttps://github.com/cogentcore/core/issues/917 and #916 (PR #918)

Thanks! It can't be assert.True, since we sometimes want it to be false (that is why we have test.ok in that test).

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: @.***>

ddkwork commented 4 months ago

Have you come up with a global fix for this panic?

{8BF307A7-5697-4316-9690-4DC299542B71}
ddkwork commented 4 months ago

//if not this is fix code

func IsZero(v reflect.Value) bool {
    if v.IsValid() {
        return false
    }
    return v.IsZero()
}
kkoreilly commented 4 months ago

I am continuing to think about a fix for those issues, and I will work on implementing it soon. I appreciate your suggestions, and I should have enough information to fix the issues.

ddkwork commented 3 months ago

Ok