save and try_save functions and IO-utilites refactor in godot_core::engine refactor

[StatisMike]

Initially addressing only #532, but after checking out TODOs and general structure some more proposed changes requiring opinions from maintainers:

• internal godot_core::engine::io module, containing all utility functions and structs regarding general I-O: loading, saving and file read/write,
• try_load returning Result instead of Option, allowing for distinguishing between load failure and casting failure (for the second possible recover, as the Gd<Resource> is encapsulated,
• GFile::try_from_unique returning Error if built from Gd<FileAccess> not in open state.

My main question would be about GdIoError - thematically it seems fitting to gather all errors stemming from try_load, try_save and GFile::try_from_unique under one enum (as in this draft). Functionally I have some doubts, as it could be limiting if we would like in future to extend the errors: GdIoError::ResourceSave would be a prime candidate, as we could potentially try to match Godot's Error to help user distinguish between the cases without requiring them to match it themselves.

On the other hand I don't know if introducing three new error types regarding each of these functionalities: GdLoadError, GdSaveError and GFileError wouldn't be considered a bloat.

Additionally, the currently used IoResult in GFile operations could be also put into GdIoError/GFileError, passing additional information about underlying Godot's Error.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-535

[Bromeon]

Thanks! 🙂

In general, I'd appreciate separation of refactorings and business logic changes/additions, at least on a per-commit basis. Otherwise it's very hard to see what has changed -- especially now where a lot of content is moved across files.

If you do change anything during a refactor (e.g. improve docs), please highlight it with comments. It's impossible for me to see diffs across different files that are not recognized by Git, at least without going through significant effort.

---

As mentioned in #473 (review), the NotUniqueError might see some use outside I/O contexts, so I wonder if we should rather move it somewhere else, maybe godot::obj or so? We do have godot::builtin::meta::ConvertError already elsewhere, but that's not really fitting for a Gd specific error.

---

My main question would be about GdIoError - thematically it seems fitting to gather all errors stemming from try_load, try_save and GFile::try_from_unique under one enum (as in this draft). Functionally I have some doubts, as it could be limiting if we would like in future to extend the errors: GdIoError::ResourceSave would be a prime candidate, as we could potentially try to match Godot's Error to help user distinguish between the cases without requiring them to match it themselves.

On the other hand I don't know if introducing three new error types regarding each of these functionalities: GdLoadError, GdSaveError and GFileError wouldn't be considered a bloat.

For previously introduced error types (#467), we followed an approach of not providing too much detailed API. Such an enum can make sense internally, but if you make it public, there are some caveats:

• It needs to be #[non_exhaustive] for forward compatibility, which limits matching.
• It adds a lot of API surface for a niche use case. GFile and save/load are important parts, but they are a handful of symbols -- while the error API would likely add twice as much symbols, just for handling errors.
• Game developers are typically more interested in the fact that there is an error, rather than knowing all the inner details.

I would thus only provide a surface-level error type, implementing the std::error::Error trait but not offering specific insights. We can expose those when there is concrete demand.

[StatisMike]

In general, I'd appreciate separation of refactorings and business logic changes/additions, at least on a per-commit basis. Otherwise it's very hard to see what has changed -- especially now where a lot of content is moved across files.

Will do! Then the creation of godot_core::engine::io module and move of NotUniqueError would be made in one commit. Should I also separate try_load() refactor, save()/try_save() addition and GFile::try_from_unique() refactor in separate commits afterwards, or one should be enough for these changes?

I would thus only provide a surface-level error type, implementing the std::error::Error trait but not offering specific insights. We can expose those when there is concrete demand.

Ok, in that case a generic IoError or such should be enough :)

[Bromeon]

Will do! Then the creation of godot_core::engine::io module and move of NotUniqueError would be made in one commit. Should I also separate try_load() refactor, save()/try_save() addition and GFile::try_from_unique() refactor in separate commits afterwards, or one should be enough for these changes?

I think it's enough to have 2 commits:

• 1 that moves everything around (changes module structure etc)
• 1 that adds new features

[StatisMike]

I've added ResourceSaver to minimal codegen, as it is needed for save() and try_save() functions. If it's a little too much, it may be possible to add some cfg tag to these functions and tests instead, though I am unsure which one: codegen-full?

[Bromeon]

I've added ResourceSaver to minimal codegen, as it is needed for save() and try_save() functions. If it's a little too much, it may be possible to add some cfg tag to these functions and tests instead, though I am unsure which one: codegen-full?

It's fine.

---

Also, consider my previous statement:

For previously introduced error types (#467), we followed an approach of not providing too much detailed API. Such an enum can make sense internally, but if you make it public, there are some caveats:

GdIoError -- which should btw be called IoError or so, as it's unrelated to Gd -- could take some inspiration of the existing ConvertError type.

Do we really need ErrorKind to be public? I think it's good to have it so we know all the possible error types internally, but it doesn't need to be exposed to the user.

By implementing the Error trait, we get a Display impl, which is likely enough for most cases. The need to inspect concrete errors can always be discussed later, driven by concrete use cases.

[StatisMike]

GdIoError -- which should btw be called IoError

Per rename to IoError I feel that I should remove the alias of std::io::Error and str::io::Result, as it can be confusing. Is it ok to do? Other rename options are not good too: GodotIoError would be also misleading (as its not coming straight from Godot), GdextIoError (feels verbose and too complex)?

Do we really need ErrorKind to be public?

Public ErrorKind allows users to differentiate between error in:

• try_load(), as was written as rationale for TODO: Option<Gd<T>> -> Result<Gd<T>>
  • is it caused from Godot's ResourceLoader not being able to load the the file at all (problem with file itself or ResourceLoader not having correct ResourceFormatLoader registered)
  • is it the convertion from Gd<Resource> to Gd<T> that caused it (so we can try to load it as other T, if possible instead)
• in GFile::try_from_unique:
  • is it caused by non-unique reference to the Gd<FileAccess> (so we need to either drop the other reference or just create brand new Gd<GFile>
  • the FileAccess not being open (either above or just open the FileAccess with correct flags)

I think that noone would need to do a non-exhaustive match as it was before (with whole Error being implemented as enum), so the user experience is greatly improved.

I can revert implementing Default for IoError, so there is no way for the user to construct their own IoError.

[Bromeon]

Per rename to IoError I feel that I should remove the alias of std::io::Error and str::io::Result, as it can be confusing. Is it ok to do?

Good call, yes you can remove that. You could include std::io and then use io::Error/Result.

---

I can revert implementing Default for IoError, so there is no way for the user to construct their own IoError.

Yes, please do. I generally think Default is often overused. It should only be provided if there's an unambiguous default state.

[Bromeon]

Public ErrorKind allows users to differentiate between error in:

• try_load(), as was written as rationale for TODO: Option<Gd<T>> -> Result<Gd<T>>

  • is it caused from Godot's ResourceLoader not being able to load the the file at all (problem with file itself or ResourceLoader not having correct ResourceFormatLoader registered)
  • is it the convertion from Gd<Resource> to Gd<T> that caused it (so we can try to load it as other T, if possible instead)

• in GFile::try_from_unique:

  • is it caused by non-unique reference to the Gd<FileAccess> (so we need to either drop the other reference or just create brand new Gd<GFile>
  • the FileAccess not being open (either above or just open the FileAccess with correct flags)

I think that noone would need to do a non-exhaustive match as it was before (with whole Error being implemented as enum), so the user experience is greatly improved.

The problem is that the current API is misleading.

pub enum ErrorKind {
    ResourceCantLoad,
    ResourceCantCast,
    ResourceCantSave,
    FileAccessNotOpen,
    FileAccessNotUnique,
    Custom,
}

This enum is the union of all possible error types, but each operation can only expose some of the errors.

Let's look at try_load(). Which of the above errors are possible? Definitely not ResourceCantSave, and not FileAccessNotUnique either (that would be an internal bug). Custom probably not... What is the difference between ResourceCantLoad and FileAccessNotOpen?

I don't want you to answer these questions, but my point is: these are not just questions I ask myself when not knowing the implementation, but questions that every user asks. So you end up with:

• 50% of enumerators that are unreachable
• seemingly overlapping enumerators (CantLoad/NotOpen) -- only documentation clears it up
• enumerators that are internal and only leak in case of gdext bugs (NotUnique)

I'm not convinced this is what users want. In practice, the only thing that's typically relevant is to differentiate logic errors (bugs that must be fixed) from runtime errors (file not found). Sometimes they overlap, e.g. if the file is provided by the developer themselves. But throwing all the possible error types at the user is not necessarily a great abstraction.

---

As mentioned, we had this exact discussion for the ConvertError type, and I don't see why we should design one error API in this way and another in a completely different one. This leads to inconsistent experience and is e.g. what I really don't like about InstanceId vs. Rid, and what must be fixed.

I'm not generally opposed to having this discussion, but there are 2 main principles I try to follow:

1. Design APIs in a minimal way and extend them upon concrete demand. This guarantees that the feature we provide is actually useful.
   • This is important because every single symbol added to the API has a cost: maintenance, refactoring, breaking changes on user side, compile time, docs bloat and discoverability.
   • By following this approach, we ensure that features are not added prematurely but only on concrete needs. "The user might want this" is not concrete.
   • Adding features is much easier than removing.
2. Follow consistent API design.
   • Since there is now a precedent for error APIs (ConvertError), that becomes the de-facto role model for designing future error APIs.
   • This does not mean existing APIs cannot be questioned or remodeled; in fact I appreciate if we can improve on those.
   • However, it does mean that when designing new APIs in a different way, there needs to be justification for deviating from the existing model. In particularly, any arguments should not be considered for the new instance on its own, but holistically for all existing APIs.
   • Yes, I know that this brings extra burden of proof on contributors that show up later, but the alternative is that everyone builds their silo solution and it's then left up to maintainers to refactor/clean up everything.

Sorry for insisting on bureaucracy here, but it's important to me that we build APIs with a good, consistent user experience 🙂

[Bromeon]

If you are interested in discussing how to model error APIs, we can gladly take it to a separate issue.

There are also a few other APIs that might need consideration:

• Gd::try_cast()
• FromGodot::try_from_godot + FromVariant::try_from_variant

I'm still not sure what the best approach would be -- domain-specific #[non_exhaustive] enums that allow very precise pin-pointing? How do we keep them forward-compatible (e.g. turning unit enumerator into struct-like ones is a breaking change)? How do we ensure that people who just want to know if it's their bug or a user error can easily extract that?

We can discuss this in #536.

[Bromeon]

I would suggest we do not wait until the error discussion is resolved, this can take months.

For now I would make the error API absolutely minimal (no overlaps as above), then I can merge a first version. We can then individually look at the different cases in future PRs. Sounds good?

[StatisMike]

@Bromeon Yeah, I think it will be for the best - will just keep the Display, no fields exportable from the IoError.

Additionally:

By default, the ResourceSaver::save() take only the Resource to be saved as a single argument. It will then try to save the given resource at its path, already available (via Resource::get_path()), or saving it as a resource local to the current scene.

I was wondering if we should change the try_save and save functions to intake a path as an Option, but felt agains it.

If someone is just updating the resource and saving it back, they could get its path from resource.get_path() and feed it into save()/try_save(). If they are oing something else, like saving resource local to the scene (in which case the path can be also unspecified), they should opt for ResourceSaver::singleton().save(resource). I think the most standard way of saving is explicit stating the path, so we should support this way.

[Bromeon]

If we only go for public Display / Error traits, does the current API with separate APIs for

• path()
• class()
• godot_error()

even make sense? They are basically a "flat" representation of an enum, which seems strictly worse: the user needs to understand which properties are set under which circumstances, but there is no type-level support (enum) to indicate this.

Maybe better to just keep the enum with the strongest type representation internally, and expose just Error/Display.

There are also some merge conflicts.

[StatisMike]

@Bromeon Should be good to go. Underlying IoError data is kept in type-safe, private and (IMO) clear container. It wouldn't be seen by the user, unless with Debug-print. Could impl Debug manually for IoError, to show the data fields as they appear in inner ErrorData enum structs, though I am not sure if it won't be more confusing for end-users.

Display contains enough information as-is, though Debug needs to be implemented for Error trait.

[Bromeon]

Thanks for the update! There are currently merge conflicts, but it looks mostly good 🙂

[Bromeon]

Thanks a lot!

Error APIs will definitely still evolve, anyone interested can participate in #536 🙂