Fix issue 17394 - mkdirRecurse isn't @safe by wilzbach · Pull Request #5386 · dlang/phobos

wilzbach · 2017-05-12T14:08:30Z

There are a lot of lot hanging fruits in std.file and in general imho we should invest a lot more work in making Phobos work nicely with @safe because currently it's really an empty promise as every second standard library function needs to be @trusted ...

dlang-bot · 2017-05-12T14:08:32Z

Fix	Bugzilla	Description
✓	17394	std.file.mkdirRecurse isn't @safe

wilzbach · 2017-05-12T14:11:52Z

std/file.d

+if (isConvertibleToString!R)
+{
+    return mkdirRecurse!(StringTypeOf!R)(pathname);
+}


mkdir accepts auto ref, so I guessed that it was probably an oversight that mkdirRecurse doesn't?

mkdirRecurse accepted a char array only - not much point for auto ref there

Well there's the following API for mkdir:

void mkdir(R)(auto ref R pathname) if (isConvertibleToString!R) { return mkdir!(StringTypeOf!R)(pathname); }

wilzbach · 2017-05-12T14:12:24Z

std/file.d


        immutable basepath = deleteme ~ "_dir";
-        scope(exit) rmdirRecurse(basepath);
+        scope(exit) () @trusted { rmdirRecurse(basepath); }();


This was done to keep this PR small and go in steady baby steps

CyberShadow · 2017-05-12T14:15:33Z

std/file.d

-void mkdirRecurse(in char[] pathname)
+void mkdirRecurse(R)(R pathname)
+if (isInputRange!R && !isInfinite!R && isSomeChar!(ElementEncodingType!R) &&
+    !isConvertibleToString!R)


mkdirRecurse accepts an arbitrary range then tries to call ensureDirExists which accepts only in char[]. ensureDirExists thus either needs to be changed to accept a range or mkdirRecurse's signature change undone.

Oh good catch! I went with changing ensureDirExists, but I don't have a strong feeling about this.

wilzbach · 2017-05-12T14:15:38Z

std/file.d

    version(Windows)
    {
-        if (CreateDirectoryW(pathname.tempCStringW(), null))
+        if (() @trusted { return CreateDirectoryW(pathname.tempCStringW(), null); }())


in mkdir static @trusted functions are used:

static auto trustedCreateDirectoryW(const(FSChar)* pathz) @trusted { return CreateDirectoryW(pathz, null); }

I like the temporary lambdas more, but AFAIK they do create an overhead. Does by chance anyone know whether this overhead is negligible after optimization?

Doesn't matter as long as they don't create garbage.

Does by chance anyone know whether this overhead is negligible after optimization?

Depends on the in-liner. Chances are DMD won't in-line this, but I wouldn't change this, as optimizing for DMD's in-liner is a deep rabbit hole we don't want to go down.

I'm a bit uneasy on this, as you are trusting the pathname's range operations when you do tempCStringW (as well as assuming the range doesn't have such a member name and you aren't calling the wrong thing). Is it possible to do this outside the trusted lambda?

mkdir does auto pathz = pathname.tempCString!FSChar();, same could be done here

CyberShadow · 2017-05-12T14:23:22Z

std/file.d

-private bool ensureDirExists(in char[] pathname)
+private bool ensureDirExists(R)(R pathname)
+if (isInputRange!R && !isInfinite!R && isSomeChar!(ElementEncodingType!R) &&
+    !isConvertibleToString!R)


I don't think you want !isConvertibleToString!R as unlike mkdir there is no overload with if(isConvertibleToString!R).

schveiguy · 2017-05-12T14:51:26Z

std/file.d

        import std.conv : octal;

-        if (core.sys.posix.sys.stat.mkdir(pathname.tempCString(), octal!777) == 0)
+        if (() @trusted { return core.sys.posix.sys.stat.mkdir(pathname.tempCString(), octal!777); }() == 0)


Same thing here, try and make the tempCString outside the lambda.

wilzbach · 2017-05-13T11:38:18Z

I removed the API unification to avoid any controversial issues - the unification between mkdir and mkdirRecurse can be done in a separate PR.

schveiguy · 2017-05-14T13:39:40Z

Thanks, LGTM.

dnadlinger · 2017-05-14T14:44:22Z

std/file.d

 */

-void mkdirRecurse(in char[] pathname)
+void mkdirRecurse()(in char[] pathname)


Why did this become a template? Templatizing things increases user code compilation times and is, strictly speaking, a breaking API change, so shouldn't be done without good reason.

It will need to become a template anyway to accept arbitrary char ranges as all the other stuff in std.file.

YAGNI for now and change it once it's actually required? If you are happy with the change, feel free to merge, though.

@klickverbot This can't be merged with your request for changes still pending

https://help.github.com/articles/dismissing-a-pull-request-review/?

@klickverbot the other option is to manually add the attributes, and then remove them all later when we change to a range interface. Neither path is super-appealing.

Can you explain why this is a breaking API change?

The first thing that comes to mind is that you can't take its address anymore. When it's not a template, the function name can be used on its own, but once it is, it has to be instantiated. When calling it, that's not a big deal, because IFTI takes care of the instantiation for you, but for other cases, it has to be done manually.

Of course, even changing a single attribute is an API change, so we're actually really terrible in general about maintaining API compatibility. We usually do a fairly good job with avoiding breaking the common cases, but we frequently break the corner cases.

Of course, even changing a single attribute is an API change

Being pedantic here, but isn't adding something like nothrow an ABI and not an API change?

I suppose that that depends on whether it affects who can call the code if it's recompiled rather than simply linked. I don't know. I tend to think of the API as being anything in the function signature, whereas the ABI has to do with the resulting binary, and changing the attributes changes the function signature.

Regardless, we routinely make changes that break compatibility in small ways, and the way that D is designed (particularly with attribute inference), you'd have to be pretty disciplined about it to never break any code with changes.

Removed concern

schveiguy · 2017-05-17T12:33:59Z

@wilzbach I just removed the template for now. In the next go-around when you want to add range support, just remove the @safe tag.

dlang-bot added the Severity:Bug Fix label May 12, 2017

wilzbach commented May 12, 2017

View reviewed changes

CyberShadow reviewed May 12, 2017

View reviewed changes

wilzbach commented May 12, 2017

View reviewed changes

wilzbach force-pushed the safe-file branch from 7f65752 to fec10d8 Compare May 12, 2017 14:18

wilzbach mentioned this pull request May 12, 2017

fix Issue 15771 - FileLogger should create the output directory if it… #5370

Closed

CyberShadow reviewed May 12, 2017

View reviewed changes

schveiguy reviewed May 12, 2017

View reviewed changes

wilzbach force-pushed the safe-file branch 2 times, most recently from 7fe771e to e50531d Compare May 13, 2017 11:33

Fix issue 17394 - mkdirRecurse isn't @safe

e50531d

CyberShadow approved these changes May 13, 2017

View reviewed changes

schveiguy added the Merge:auto-merge label May 14, 2017

dnadlinger removed the Merge:auto-merge label May 14, 2017

dnadlinger previously requested changes May 14, 2017

View reviewed changes

Defer addition of template until we need it for ranges.

fabf19a

schveiguy added the Merge:auto-merge label May 17, 2017

dlang-bot merged commit 22799ff into dlang:master May 17, 2017

wilzbach deleted the safe-file branch October 1, 2017 15:27

Uh oh!

Conversation

wilzbach commented May 12, 2017

Uh oh!

dlang-bot commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented May 13, 2017

Uh oh!

schveiguy commented May 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schveiguy commented May 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants