Directory must be open before using remove (3.2.3 rc1)

[bitwes]

Godot version:
3.2.3 rc1

OS/device including version:
macOS 10.15.5

Issue description:
As of 3.2.3 rc1 you must open a directory before using remove. Prior to 3.2.3 rc1 you didn't have to open up a directory to delete it, which makes sense. If you are attempting to delete a directory, then you probably don't care if it doesn't exist. If you did, you' d check for it first. The new functionality might be the "right" way but I would argue it is a breaking change and shouldn't be introduced in a patch.

Steps to reproduce:
Assuming a directory user://foo exists and it is empty.

var d = Directory.new()
d.remove('user://foo') # this causes error, the directory is not removed.  this works in 3.2.2

Minimal reproduction project:
See code. Should be enough.

[akien-mga]

The new behavior is correct, the previous one was quite dangerous. Directory auto-opens res:// when it's instantiated, so if you did:

var dir = Directory.new()
dir.remove("test")

It would remove the res://test folder, and this is unexpected for many users as the implementation detail of Directory starting in res:// by default is not known to all.

If you want to delete a folder in e.g. user://, you should open user:// (and thus have the Directory configured for access to the user:// filesystem), and then you can remove anything you want in user://.

[akien-mga]

See #40146.

[bitwes]

That is probably a bigger issue than not deleting a directory (at least old me thought so per #24149).

Is there any documentation on how the new Directory object should work? It seems a little inconsistent.

For example, should remove fail if the open call failed? It currently does not. This works:

# Assuming a directory user://foo exists and it is empty.
var d = Directory.new()
var result = d.open('res://does_not_exist/') # returns 31
d.remove('user://foo') # does not cause error

Here's some other questions. Sorry if this has been discussed elswhere. Should a Directory instance:

• be able to remove the directory it opened?
• be able to remove paths that are not sub-paths of what it opened (i.e. opening res:// but deleting from user://)?
• require a valid open call before performing any actions?
• what happens when whatever you have opened with open becomes invalid?

[Xrayez]

@bitwes yeah I've stumbled upon this while using Gut, see also #40234.

[akien-mga]

CC @nathanfranke

[nathanfranke]

As akien said this is pretty much intended #40763 (comment)

---

# Assuming a directory user://foo exists and it is empty.
var d = Directory.new()
var result = d.open('res://does_not_exist/') # returns 31
d.remove('user://foo') # does not cause error

However, this is definitely not intended so this should be fixed

[nathanfranke]

I'm not going to be at my workspace for a while, so someone else can make a PR (Junior Job?)

Here is the current function

godot/core/bind/core_bind.cpp

Lines 1611 to 1625 in 210ccb3

	Error _Directory::open(const String &p_path) {
	Error err;
	DirAccess *alt = DirAccess::open(p_path, &err);
	if (!alt) {
	return err;
	}
	if (d) {
	memdelete(d);
	}
	d = alt;
	dir_open = true;
	return OK;
	}

Below

if (!alt) {
    return err;
}

Add

if (err != OK) {
    return err;
}

(Plus a blank line in my opinion)

P.S.: Dang, sorry so many issues arose from my pull request :(

[bitwes]

There appears to be a lot of edge cases with the new changes to Directory. Maybe, with all these additional requirements, the Directory object should be deprecated and a new one should be created that has the more complicated behavior. This would allow for the introduction of more consistent but breaking changes.

[nathanfranke]

The new behavior isn't complicated, just different, and the behavior was intended by the start. All I did was fix the error condition.

[akien-mga]

Well as it breaks compat, I think it might be best for 3.2 to revert to the previous state (all methods can be used without opening a directory manually, even if it's confusing as a directory is open by default), and redesign the Directory API for 4.0 (which is starting to show its age and non-optimal design).

On the other hand, there should be better handling of bad input when trying to remove non-existing paths.

[nathanfranke]

IMO fix up the current directory stuff for 3.2 and redesign it for 4.0

[bitwes]

The new behavior isn't complicated, just different, and the behavior was intended by the start. All I did was fix the error condition.

Your PR isn't complicated, but the repercussions of it are. Should a Directory instance:

• be able to remove the directory it opened?
• be able to remove paths that are not sub-paths of what it opened (i.e. opening res://foo but deleting from res://bar)?
• what happens when whatever you have opened with open becomes invalid? For example you open res://foo in one Directory object, then delete res://foo in another Directory object. The first no longer is "open" to a valid directory.
• Should each operation that requires a call to open verify that the path is still valid before each action?

[nathanfranke]

@bitwes I don't think that's really in the scope of this issue though. Those edge cases should have been considered when this system was designed in the first place.

We can add checks to make Directory more safe to use (for example if you delete something outside of Godot) but that would be against Akien's proposal to just revert these changes for 3.2.

IMO:

• Keep the changes we have so far since the old behaviour can be dangerous (see Deleting a user:// folder with an invalid path deletes top level project files #40146)
• Ignore the edge cases until 4.0. It doesn't matter what happens as long as it is the same as it was before.
• Test the current system thoroughly before 3.2.3 is released and make sure it is expected behaviour.
• Redesign the File/Directory workflow for 4.0

[akien-mga]

Test the current system thoroughly before 3.2.3 is released and make sure it is expected behaviour.

3.2.3 is meant to be released this week, so we don't have much time.

So I'm more for preserving compatibility in 3.2.3, and we can take the time to make things work well for 3.2.4, possibly with a slight compat breakage if it really makes sense - but as it stands while the compat breakage does prevent a dangerous case, it's unclear whether the current API is really a good improvement.

I would keep the changes in master though, it's good to start changing this API to make it more robust.

[akien-mga]

Fixed by #40803 for now. Let's see how to handle the various issues that remain with Directory to make it safer and without better input validation.