vlang / v

Simple, fast, safe, compiled language for developing maintainable software. Compiles itself in <1s with zero library dependencies. Supports automatic C => V translation. https://vlang.io
MIT License
35.66k stars 2.15k forks source link

Recursive struct with an optional should work #14185

Open medvednikov opened 2 years ago

medvednikov commented 2 years ago
struct Node {
  next ?Node
}

c.v:3:8: error: invalid recursive struct `Node`
    1 | module main
    2 |
    3 | struct Node {
      |        ~~~~
    4 |   next ?Node
    5 | }
ntrel commented 2 years ago

This should not work, when the optional is not empty the struct data should be stored inside it for performance, not on the heap.

medvednikov commented 2 years ago

it wraps owner struct type as a pointer type under the hood, just like with []Node

vincenzopalazzo commented 2 years ago

yeah, I fixed a bug a while ago for recursive definition, maybe I missed the recursive option.

I think this should not two different checks, but we can do all in one single check, maybe we can introduce also a food refactoring

This should not work, when the optional is not empty the struct data should be stored inside it for performance, not on the heap.

I'm not catching this, if you are doing some good design architecture, like a chain of responsibility pattern you have two structures that are completely different. What you are saying to me makes no sense, seems "embedded struct" proposal.

spytheman commented 2 years ago

yeah, I fixed a bug a while ago for recursive definition, maybe I missed the recursive option.

I think this should not two different checks, but we can do all in one single check, maybe we can introduce also a food refactoring

This should not work, when the optional is not empty the struct data should be stored inside it for performance, not on the heap.

I'm not catching this, if you are doing some good design architecture, like a chain of responsibility pattern you have two structures that are completely different. What you are saying to me makes no sense, seems "embedded struct" proposal.

imho what you are writing makes no sense. I agree with @ntrel .

spytheman commented 2 years ago

@medvednikov for:

struct Abc {
        a int
        b int
}

fn abc() ?Abc {
        return Abc{}
}
fn main() {
        a := abc() ?
        dump(a)
}

we currently generate:

struct Option_main__Abc {
    byte state;
    IError err;
    byte data[sizeof(main__Abc) > 0 ? sizeof(main__Abc) : 1];
};

With such an implementation, ?Node is not a pointer, but a struct that is bigger than just Node.

ChAoSUnItY commented 2 years ago

I wonder if there's specific reason to use byte to store data but a pointer?

vincenzopalazzo commented 2 years ago

@spytheman Why a recursive structure should not work in V? How you will write something like a tree? or a handler that has other handlers inside?

I'm not able to catch why we should not be allowed recursive structure, and why we should merge two structs? In this way we miss potential improvements in the future of the struct type like the extension of another struct.

I don't see where the Performace improvement is with this merge stuff, on the other hand, it will introduce confusion that maybe the programmer doesn't want to do. BTW I'm nobody to say that this choice is wrong!

But my thought has a meaning, that simple is introducing some performance improvement at this point can cut some future change in the language!

But again, I'm nobody

spytheman commented 2 years ago

I wonder if there's specific reason to use byte to store data but a pointer?

byte/u8 is 1 byte long, so an array of bytes of length sizeof(main__Abc) will be enough space to store the actual value of the option, in the case it is not error/none.

spytheman commented 2 years ago

@vincenzopalazzo

Why a recursive structure should not work in V? How you will write something like a tree? or a handler that has other handlers inside?

V already has pointers: &T, which serve as a stop gap measure for that.

After the !/? split in https://github.com/vlang/v/pull/14140 , it will also have !T and ?T (they are currently the same, and the ?T is used much more as error | T, which in turn is very common - it is the basis of the error handling in V, so it must be as efficient as possible. Making heap allocations, on every function call that returns ?T will be disastrous, especially without working -autofree).

After the split, the ?Type struct fields, which will then (or after a short transition period) mean none | Type, could be implemented/represented as something different - a non 0 pointer for example, pointing to a heap allocated value, but currently that is prohibitive, since it will affect the option handling case too (error | Type).

spytheman commented 2 years ago

But again, I'm nobody

If that is asking why I unassigned you - I do not think you are nobody, but I do think, that there are much more prepared people, to do that change, that have more experience in that area, and had done similar work in the past - @danieldaeschle , @spaceface777 , @medvednikov for example. Communicating why things are the way they are currently, what is the goal, what should be done etc, will be very inefficient otherwise.

danieldaeschle commented 2 years ago

I tried this a while ago. With the current implementation this is not possible. It would only be possible if it's a pointer as already stated. But you could also do ?&Foo which would be more explicit.

qtc-de commented 2 years ago

So what is the current state of the art for representing a recursive structure with an optional in V? What works for me is using a sum type:

struct None {}

struct Example {
    str string
    reference &MaybeExample
}

type MaybeExample = Example | None

fn main() {
    example1 := Example{ str: 'example1', reference: &None{} }
    mut example2 := Example{ str: 'example2', reference: &example1 }

    for {
        println(example2.str)

        match example2.reference {
            Example {
                example2 = example2.reference
            }
            None {break}
        }
    }
}

However, in some other issues it was said that implementing a custom None type should not be done. Instead it was suggested to use T | none. However, this does currently not seem to work:

struct Example {
    str string
    reference &Example | none
}

fn main() {
    example1 := Example{ str: 'example1', reference: none }
    mut example2 := Example{ str: 'example2', reference: &example1 }

    for {
        println(example2.str)

        match example2.reference {
            Example {
                example2 = example2.reference
            }
            none {break}
        }
    }
}

The compiler is complaining about none not being a reference.

error: reference field must be initialized with reference
    5 |
    6 | fn main() {
    7 |     example1 := Example{ str: 'example1', reference: none }

Using the builtin type none seems to be more correct to me. Is there some way to get this work?