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

std.testing.refAllDeclsRecursive: Stop recursive loops #19814

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elijahimmer
Copy link

@elijahimmer elijahimmer commented Apr 30, 2024

I didn't see much on a contributing guide, so I tried to work off of others. Tell me if I missed anything I need. And thanks for reviewing.

Old behavior:

Whenever a type referenced itself, like

pub const A = struct {
    pub const D = A;
};

refAllDeclsRecursive would crash the test with a status code 11 because it would recurs endlessly.

More complex recursion like two things referencing each other would do the same.

pub const A = struct {
    pub const D = A;
    pub const C = B;
};

pub const B = struct {
    pub const C = A;
};

This happens often in things like std.Thread, where the whole file is the struct, so it declares
pub const Thread = @This(), and generally when things link to each other publically.

New Behavior:
This recurs down the whole tree and keeps track of the types as it goes. This will go over types multiple times, but it will not recurs endlessly like now it is currently.
The only way I could think of to get it to only recurs down each tree once was with a array or comptime allocation, and that had more limitations (or wasn't possible yet).

Limitations:
(potentially) slower compile times on large projects using this, but it shouldn't be very much. (and only during tests)
It would slow test compile times, but it would not lead to a compiler crash when it recurses too much like the old method.

I did some tests, but I wasn't able to do too many, so someone please help with that.

I am also not good at zig yet, so any ideas on how to improve it are welcome.

I don't know if this is quite a good fit for the standard library, but it doesn't do much more than the function did before, but it is far more useful and (shouldn't) crash if you have something referencing itself in it's tree.

Tests:
I made a more complicated example, and it seemed to reference everything (I checked with @CompileError()).

pub const A = struct {
    pub const Z = A;
    pub const W = B;
    pub const F = struct {
        pub const U = A;
    };
};

pub const B = struct {
    pub const Z = A;
    pub const W = C;
};

pub const C = struct {
    pub const Z = A;
    pub const W = D;
};

pub const D = struct {
    pub const Z = E;
    pub const W = D;
};

pub const E = struct {
    pub const D = A;
    pub const W = E;
};

Again, if they are very large and deep trees, it would likely be slow to compile the test, but I don't know any way around that, or making this better.

Thanks, this is a wonderful language and I would like to help out where I can.

I also considered just making it so the struct doesn't recurse into itself by doing
if (@field(T, decl.name) != T) refAllDeclsRecursive(...)
which wouldn't require another function, but it wasn't much more do this and stop all infinite recursions (I could think of)

@elijahimmer elijahimmer changed the title std.testing.refAllDeclsRecursive: Stop self-declaring recursion std.testing.refAllDeclsRecursive: Stop recursive loops Apr 30, 2024
@elijahimmer elijahimmer force-pushed the master branch 2 times, most recently from d2db335 to cbd33aa Compare May 5, 2024 01:59
I didn't see much on a contributing guide, so I tried to work off of
others. Tell me if I missed anything I need. And thanks for reviewing.

Old behavior:

Whenever a type referenced itself, like

```zig
pub const A = struct {
    pub const D = A;
};
```

refAllDeclsRecursive would crash the test with a status code 11 because
of the recursion.

More complex recursion like two things referencing each other would do
the same.

```zig
pub const A = struct {
    pub const D = A;
    pub const C = B;
};

pub const B = struct {
    pub const C = A;
};
```

New Behavior:
This recurs down the whole tree and keeps track of the types as it goes.
This will go over types multiple times, but it will not recurs endlessly
like now it is currently.
The only way I could think of to get it to only recurs down each tree
once was with a array or comptime allocation, and that had more
limitations (or wasn't possible yet).

Limitations:
(potentially) slower compile times on large projects using this, but it
shouldn't be very much.
I did some tests, but I wasn't able to do too many, so someone please
help with that.

I am also not good at zig yet, so any ideas on how to improve it are
welcome.

I don't know if this is quite a good fit for the standard library, but
it doesn't do much more than the function did before, but it is far more
useful and (shouldn't) crash if you have something referencing itself.

Tests:
I made a more complicated example, and it seemed to reference everything
(I checked with @CompileError()).

```zig
pub const A = struct {
    pub const Z = A;
    pub const W = B;
    pub const F = struct {
        pub const U = A;
    };
};

pub const B = struct {
    pub const Z = A;
    pub const W = C;
};

pub const C = struct {
    pub const Z = A;
    pub const W = D;
};

pub const D = struct {
    pub const Z = E;
    pub const W = D;
};

pub const E = struct {
    pub const D = A;
    pub const W = E;
};
```
Again, if they are very large and deep trees, it would likely be slow to
compile the test, but I don't know any way around that, or making this
better.

Thanks, this is a wonderful language and I would like to help out where
I can.
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

1 participant