-
-
Notifications
You must be signed in to change notification settings - Fork 608
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 #20595 - automatically unittest destroy(T.init)
#20598
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @thewilsonator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
|
a5431c3
to
0d1bb53
Compare
static if (is (T == struct) && is(typeof(T.init) == T)) | ||
{ | ||
/// Verify that `destroy` works with `T.init`. | ||
/// TODO: explain why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @pbackus could you please provide a detailed explanation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short answer: I was wrong. Close this.
Long answer: the requirement I was thinking of is that destroying T.init
must not cause undefined behavior, because T.init
is always available in @safe
code. But that does not mean it has to succeed—it could still throw an exception, enter an infinite loop, terminate the process, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long answer: the requirement I was thinking of is that destroying
T.init
must not cause undefined behavior, becauseT.init
is always available in@safe
code. But that does not mean it has to succeed—it could still throw an exception, enter an infinite loop, terminate the process, etc.
I don't think you can even rely on T.init
always being available in @safe
code due to it still being possible to define an init()
method, which may be @system
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if a type defines an init()
method, you can still obtain its .init
value by using something like (T[1]).init
.
There is currently no reliable way to prevent @safe
code from accessing a type's .init
value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is currently no reliable way to prevent
@safe
code from accessing a type's.init
value.
A @system
init field is also legal - struct Foo { @system noreturn init; }
, for example. The (T[1]).init
trick doesn't work around that one. It's only a deprecation right now, so TECHNICALLY it's allowed... for now.
Whoops, testing error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me, even with -preview=systemVariables
enabled:
struct S
{
int n = 123;
@disable this();
@system noreturn init;
}
void main() @safe
{
auto a = (S[1]).init;
assert(a[0].n == 123); // passes
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm:
struct S
{
int n = 123;
//@disable this();
@system static immutable S init;
}
void main() @safe
{
auto a = S.init;
assert(a.n == 123); // passes
}
successfully compiles with -preview=systemVariables
which seems like a bug to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely a bug, but it's also irrelevant to this PR which—again—should be closed.
0d1bb53
to
78d2dc3
Compare
No description provided.