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 Issue 18157 - std.file.rmdirRecurse should be usable in @safe #7675
Conversation
Thanks for your pull request and interest in making D better, @ljmf00! 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. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#7675" |
Please tell if I'm thinking in a wrong way and there's logic that need to be added in order to make this properly @safe , but AFAIK this shouldn't be a problem. |
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.
The general idea is to have @trusted
as narrow in scope as possible
std/file.d
Outdated
@@ -4444,6 +4445,7 @@ void rmdirRecurse(scope const(char)[] pathname) | |||
} | |||
|
|||
/// ditto | |||
@trusted |
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.
What needs to be @trusted
in this function?
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.
dirEntries
is @System, so that's why it needs @trusted attribute. In this context, I consider this being safe because dirEntries
returns a RefCounted wrapped variable, that, inside this will always be eliminated, so it doesn't make much sense to be unsafe here, IMHO.
The unsafe part is here:
foreach (DirEntry e; dirEntries(de.name, SpanMode.depth, false))
{
attrIsDir(e.linkAttributes) ? rmdir(e.name) : remove(e.name);
}
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.
is it the call to dirEntries, or the iteration or both? What about rmdir
/remove
?
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.
Can't we use a local @trusted lambda + a comment to make sure nothing unsafe gets added in the future?
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.
is it the call to dirEntries, or the iteration or both? What about
rmdir
/remove
?
both rmdir
and remove
uses a @trusted trustedRmdir
and removeImpl
respectively, so, when called it's both @safe calls.
Can't we use a local @trusted lambda + a comment to make sure nothing unsafe gets added in the future?
Sure, I'll do it.
Also, if you follow this example here,
phobos/std/datetime/timezone.d
Line 2451 in 453faad
foreach (DirEntry de; dirEntries(tzDatabaseDir, SpanMode.depth)) |
dirEntries
in a @trusted function, so maybe it should be a good idea to wrap it too. I'll do it in another PR.
Does this have an issue number? |
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.
.
Yes, https://issues.dlang.org/show_bug.cgi?id=18157 . I'll change the PR title. Also, I don't think the dependency there to https://issues.dlang.org/show_bug.cgi?id=18155 is necessary, because, since |
My fault, I referenced the wrong issue. Already updated the title again. |
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.
Can it also be marked as pure
?
No. This function can't be pure in any way because it calls impure functions and this perform I/O. |
`rmdirRecurse` should be @safe as the cast(string) is safe in this context and dirEntries, even though @System, it uses a RefCounted iterator which inside will always make the reference deleted as the reference will never be passed outside the function scope. Signed-off-by: Luís Ferreira <contact@lsferreira.net>
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.
lgtm
} | ||
// dirEntries is @system because it uses a DirIterator with a | ||
// RefCounted variable, but here, no references to the payload is | ||
// escaped to the outside, so this should be @trusted |
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.
Can't this be verified by the compiler by making the ref DirEntry de
scope
?
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 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.
See this interesting discussion about @safe ref counting on dlang forum: https://forum.dlang.org/thread/r8ilpu$2mk7$1@digitalmars.com
@wilzbach the bot didn't detected the issue Fix even though merged with Fix Issue xxx https://issues.dlang.org/show_bug.cgi?id=18157. Can this be closed? |
The bot only parses the commit messages as they are used to assemble the changelog. That's why it's important to check whether the issue displays in its commit message. We can't do much more than closing the issue manually and likely it will end up in the changelog because of the merge commit title. |
rmdirRecurse
should be @safe as the cast(string) is safe in this context anddirEntries, even though @System, it uses a RefCounted iterator which inside
will always make the reference deleted as the reference will never be passed
outside the function scope.
Signed-off-by: Luís Ferreira contact@lsferreira.net