Simplify module structure (redundant re-exports) #788

Bromeon · 2021-09-27T14:21:44Z

Simplifies the module structure and closes #771.

Changes

TLDR:

Every symbol lives in exactly one module (+ optionally the prelude)
No symbols except modules at gdnative crate root (not even macros)
Some module renamings for clarification
No semantic changes -- may break imports however

Detailed list:

Change gdnative::object
- Now official home for Ref, TRef etc.
- Move several verifying traits: object -> object::bounds
- Move/rename gdnative::thread_access -> gdnative::object::ownership
- Move/rename gdnative::ref_kind -> gdnative::object::memory
Change gdnative::core_types
- No more nested modules
- Move gdnative::GodotError to gdnative::object::GodotError
Change gdnative::nativescript
- Rename init -> export
- Rename profiling -> profiler
- export and user_data no longer re-exported to nativescript
- class now private, symbols re-exported to nativescript
- Macro profile_sig! now in profiler
- Improve documentation + fix doc tests
Add gdnative::macros
- replaces crate root for godot_init! etc.
Add gdnative::derive
- contains derive macros and macro attributes
Document feature flags in lib.rs and Variant

Things not (yet) done

Those could be discussed in separate PRs.

Changes to the prelude
Clean up the modules inside gdnative::api
The issue here is that the modules under api are used for grouping related symbols. For example, api::node has 3 structs Node, DuplicateFlags and PauseMode. In C++, they would be represented as enum/struct inside a class Node, however Rust does not support nested types in struct, hence the module is used. An alternative would be to remove api::Node and just use api::node::Node, but this is less ergonomic.
Some very deep nestings, such as gdnative::nativescript::export::property::hint::ArrayHint

jacobsky

Thank you for the hard work. This looks like a really good start to the changes! I've left a bit of my thoughts as well as some confirmation points. I think I understand where we are going with the changes. It should make it a bit easier to validate.

Have you tried generating the docs with this PR?

jacobsky · 2021-09-27T14:40:40Z

gdnative/src/lib.rs

@@ -53,12 +53,12 @@



The goal of this change is to remove the workaround that was previously put in place. To confirm, the reason for this change is that the workaround is no longer needed, correct? If so, I think removing all the references as well as the comment may be the best option.

Definitely, this is still a WIP and I need to first confirm that the workaround is no longer needed (at least on stable Rust).

I built the docs from your branch and currently it looks like regardless of what we do we're still getting the multiple module paths when searching the docs.

Your changes:
https://godot-rust.github.io/docs/gdnative/

jacobsky · 2021-09-27T14:44:06Z

gdnative-core/src/core_types/error.rs

@@ -82,3 +82,6 @@ impl std::fmt::Display for GodotError {
 }

 impl std::error::Error for GodotError {}
+
+/// Result type with [GodotError]
+pub type GodotResult = Result<(), GodotError>;


I take it the rationale behind moving GodotResult to core_types is that it's the Rust-side implementation for the Godot Error codes, correct? If so, I like this change. It makes a lot of sense to put GodotResult here.

Yes, and to be honest I didn't know a module that would fit better. But your reason sounds more sophisticated 😅

jacobsky · 2021-09-27T14:47:11Z

gdnative-core/src/core_types/mod.rs

@@ -1,11 +1,16 @@
-//! Types that represent core-datatypes of Godot.
+//! Types that represent core data types of Godot.


Just confirming, most of the changes in here are for organizational purposes, correct? If so, looks good!

Yes, I don't want to introduce any semantic changes in this PR.
There might be breaking changes due to new module structure, but functionality-wise, we should not lose anything.

jacobsky · 2021-09-27T14:47:53Z

gdnative-core/src/core_types/mod.rs

@@ -43,7 +44,7 @@ pub use rid::*;
 pub use string::*;
 pub use string_array::*;
 pub use transform2d::*;
-pub use typed_array::{Element, TypedArray};


Element is intended to be an internal implementation detail? Or was this initially made public (in the past) by mistake?

I wondered the same thing. A lot of stuff is public and sometimes even appears in the documentation, though I don't think anyone uses it. Probably we should locate some of those cases.

I added the Element re-export in the serde PR. It's not technically needed anymore, but it was discussed here: #743 (comment)

Ah interesting, thanks!

Element is documented at the moment, and it's nice for the user to see which element types are supported. If we want to keep the core_types::typed_array module hidden, we should also keep re-exporting Element, no?

I think it could potentially be useful to allow users to implement their own traits generically for all TypedArrays.

jacobsky · 2021-09-27T14:51:12Z

gdnative-core/src/lib.rs

@@ -56,7 +55,7 @@ pub mod private;
 //

 pub use init::{InitializeInfo, TerminateInfo};
-pub use new_ref::NewRef;


This is a good change. It's probably a good idea to keep all the similar stuff together, as such merging new_ref and object is a really good change.

Yes, my idea is that all helper types related to object and reference handling would become part of the object module. I also moved object.rs to object/mod.rs, which contains the large part of the symbols.

Bromeon

Thanks for the review! From your feedback, there are still these tasks left (in addition to those in the initial post):

what needs to be public -- Element and possibly others
#[doc(inline)] workaround in gdnative/src/lib.rs
what exactly belongs to object module
check resulting documentation after changes

Bromeon · 2021-09-27T16:45:17Z

gdnative/src/lib.rs

@@ -53,12 +53,12 @@



Definitely, this is still a WIP and I need to first confirm that the workaround is no longer needed (at least on stable Rust).

Bromeon · 2021-09-27T16:47:09Z

gdnative-core/src/lib.rs

@@ -56,7 +55,7 @@ pub mod private;
 //

 pub use init::{InitializeInfo, TerminateInfo};
-pub use new_ref::NewRef;


Yes, my idea is that all helper types related to object and reference handling would become part of the object module. I also moved object.rs to object/mod.rs, which contains the large part of the symbols.

Bromeon · 2021-09-27T16:48:13Z

gdnative-core/src/core_types/mod.rs

@@ -1,11 +1,16 @@
-//! Types that represent core-datatypes of Godot.
+//! Types that represent core data types of Godot.


Yes, I don't want to introduce any semantic changes in this PR.
There might be breaking changes due to new module structure, but functionality-wise, we should not lose anything.

Bromeon · 2021-09-27T16:49:26Z

gdnative-core/src/core_types/mod.rs

@@ -43,7 +44,7 @@ pub use rid::*;
 pub use string::*;
 pub use string_array::*;
 pub use transform2d::*;
-pub use typed_array::{Element, TypedArray};


I wondered the same thing. A lot of stuff is public and sometimes even appears in the documentation, though I don't think anyone uses it. Probably we should locate some of those cases.

jacobsky · 2021-09-27T23:26:20Z

@Bromeon I'm glad to help. That tasklist sounds correct. Most of it is just checking my assumptions about the changes.

I think double-checking whether anything else belongs in object would be a great idea. Then we can keep the breaking changes to just 0.10.0 😄

I am curious if newer versions of cargo doc have removed the need for the workaround. Or perhaps we can just kind of live with the issue? (I'm not actually sure what was actually being fixed with that change). 😅

Bromeon · 2021-10-04T22:58:38Z

Changes so far:

Avoid many duplicate module paths for core_type
Clean up most of gdnative::object
- Move Ref, TRef etc. from top-level namespace
- Rename thread_access -> ownership (but leaving symbol names for now)
- Rename ref_kind -> memory
- Add object::bounds with all the implementation details, so they don't pollute object
Move derive macros from top-level to gdnative::derive

The above are not yet 100% done, needs some double-checking.

Tasks left for consideration:

Move Instance/RefInstance from nativescript to object, so they're next to Ref/TRef.
Check if nativescript can be organized better -- and is its name helpful?
What do do with global macros like godot_init, godot_error etc.
Lots of symbols are #[doc(hidden)] but maybe part of the API; others are implementation details but public.
Hide symbols which the user doesn't get into contact with.
Decide if deprecation of a handful symbols (like gdnative::Ref, gdnative::NativeScript) is useful, or not.
For non-type symbols, this would possibly require code duplication...
Go through prelude symbols and check if their inclusion is justified

jacobsky

These changes look great. I like the new module structure. I think that we can continue down this route.

Regarding your proposals for additional changes,

Tasks left for consideration:

Move Instance/RefInstance from nativescript to object, so they're next to Ref/TRef.

I am 100% in favor of this. Ref/TRef or Instance/RefInstance should be in the same module as they are super closely related.

Check if nativescript can be organized better -- and is its name helpful?

I think nativescript is in a solid enough spot. I'm not sure that I would move anything else out of there.

What do do with global macros like godot_init, godot_error etc.

These might be a good candidate for a macros module. (Not sure 100% where they are currently)

Lots of symbols are #[doc(hidden)] but maybe part of the API; others are implementation details but public.
Hide symbols which the user doesn't get into contact with.
Decide if deprecation of a handful symbols (like gdnative::Ref, gdnative::NativeScript) is useful, or not.

Both of the above are great ideas.
Is this referring only to the re-export? If so, I think this would be a good idea. I think we generally want users to do one of two things when interacting with the API.

a. Only make use of the prelude (use gdnative::prelude::*) which should contain everything necessary for normal use.
b. Use the full canonical name, not use use gdnative::{thing}

For non-type symbols, this would possibly require code duplication...

Which non-type symbols are you specifically considering here? Some code duplication is inevitable, but we could probably manage it by clever use of the include! macro.

Go through prelude symbols and check if their inclusion is justified

This sounds like a good idea, if you would like to ping me on a review, I'd be happy to run through the list and give my thoughts on each item.

Bromeon · 2021-10-05T07:40:45Z

Thanks a lot for your feedback!

What do do with global macros like godot_init, godot_error etc.

These might be a good candidate for a macros module. (Not sure 100% where they are currently)

Currently they're in top-level gdnative module. I agree a nested module might make sense. I don't really see them fit in one of object, nativescript, api or core_types.

Maybe godot_init! can be re-exported in the prelude. The others don't seem very common.

Lots of symbols are #[doc(hidden)] but maybe part of the API; others are implementation details but public.
Hide symbols which the user doesn't get into contact with.
Decide if deprecation of a handful symbols (like gdnative::Ref, gdnative::NativeScript) is useful, or not.

Both of the above are great ideas. Is this referring only to the re-export?

Yes, I wouldn't change/deprecate any functionality as part of this PR, just module names.

For non-type symbols, this would possibly require code duplication...

Which non-type symbols are you specifically considering here? Some code duplication is inevitable, but we could probably manage it by clever use of the include! macro.

Macros/macro attributes like gdnative::NativeClass, gdnative::methods or traits like gdnative::NewRef, for example.

Go through prelude symbols and check if their inclusion is justified

This sounds like a good idea, if you would like to ping me on a review, I'd be happy to run through the list and give my thoughts on each item.

If you have the time, that would be nice!

All the bindings types should definitely stay there, as well as the core types. Probably makes sense to keep important members of object module, too. Maybe with macros, we could be more selective (definitely keep all the common derive-related symbols though).

…from top-level Adjust imports in derive macros, examples and UI tests

jacobsky

Looks good to me. The docs also appear to be building properly. Thank you for all the hard work! :)

jacobsky · 2021-10-12T00:49:30Z

(There did seem to be a couple of clippy lints that were failing in nightly related to two panics though)

Bromeon · 2021-10-12T18:36:55Z

bors try

bors · 2021-10-12T18:48:01Z

try

Build succeeded:

full-ci

Bromeon · 2021-10-12T20:03:55Z

bors try

bors · 2021-10-12T20:12:40Z

try

Build succeeded:

full-ci

Bromeon · 2021-10-13T08:02:12Z

bors r+

788: Simplify module structure (redundant re-exports) r=Bromeon a=Bromeon Simplifies the module structure and closes #771. ## Changes **TLDR:** * Every symbol lives in exactly one module (+ optionally the prelude) * No symbols except modules at `gdnative` crate root (not even macros) * Some module renamings for clarification * No semantic changes -- may break imports however **Detailed list:** * Change `gdnative::object` * Now official home for `Ref`, `TRef` etc. * Move several verifying traits: `object` -> object::bounds * Move/rename `gdnative::thread_access` -> `gdnative::object::ownership` * Move/rename `gdnative::ref_kind` -> `gdnative::object::memory` * Change `gdnative::core_types` * No more nested modules * Move `gdnative::GodotError` to `gdnative::object::GodotError` * Change `gdnative::nativescript` * Rename `init` -> `export` * Rename `profiling` -> `profiler` * `export` and `user_data` no longer re-exported to `nativescript` * `class` now private, symbols re-exported to `nativescript` * Macro `profile_sig!` now in `profiler` * Improve documentation + fix doc tests * **Add** `gdnative::macros` * replaces crate root for `godot_init!` etc. * **Add** `gdnative::derive` * contains derive macros and macro attributes * Document feature flags in lib.rs and `Variant` ## Things not (yet) done Those could be discussed in separate PRs. * Changes to the prelude * Clean up the modules inside `gdnative::api` The issue here is that the modules under `api` are used for grouping related symbols. For example, `api::node` has 3 structs `Node`, `DuplicateFlags` and `PauseMode`. In C++, they would be represented as enum/struct inside a `class Node`, however Rust does not support nested types in struct, hence the module is used. An alternative would be to remove `api::Node` and just use `api::node::Node`, but this is less ergonomic. * Some very deep nestings, such as `gdnative::nativescript::export::property::hint::ArrayHint` Co-authored-by: Jan Haller <bromeon@gmail.com>

bors · 2021-10-13T08:17:21Z

Build failed:

full-ci

Bromeon · 2021-10-13T08:17:48Z

Windows CI workers did not start -- let's try again.
bors r+

788: Simplify module structure (redundant re-exports) r=Bromeon a=Bromeon Simplifies the module structure and closes #771. ## Changes **TLDR:** * Every symbol lives in exactly one module (+ optionally the prelude) * No symbols except modules at `gdnative` crate root (not even macros) * Some module renamings for clarification * No semantic changes -- may break imports however **Detailed list:** * Change `gdnative::object` * Now official home for `Ref`, `TRef` etc. * Move several verifying traits: `object` -> object::bounds * Move/rename `gdnative::thread_access` -> `gdnative::object::ownership` * Move/rename `gdnative::ref_kind` -> `gdnative::object::memory` * Change `gdnative::core_types` * No more nested modules * Move `gdnative::GodotError` to `gdnative::object::GodotError` * Change `gdnative::nativescript` * Rename `init` -> `export` * Rename `profiling` -> `profiler` * `export` and `user_data` no longer re-exported to `nativescript` * `class` now private, symbols re-exported to `nativescript` * Macro `profile_sig!` now in `profiler` * Improve documentation + fix doc tests * **Add** `gdnative::macros` * replaces crate root for `godot_init!` etc. * **Add** `gdnative::derive` * contains derive macros and macro attributes * Document feature flags in lib.rs and `Variant` ## Things not (yet) done Those could be discussed in separate PRs. * Changes to the prelude * Clean up the modules inside `gdnative::api` The issue here is that the modules under `api` are used for grouping related symbols. For example, `api::node` has 3 structs `Node`, `DuplicateFlags` and `PauseMode`. In C++, they would be represented as enum/struct inside a `class Node`, however Rust does not support nested types in struct, hence the module is used. An alternative would be to remove `api::Node` and just use `api::node::Node`, but this is less ergonomic. * Some very deep nestings, such as `gdnative::nativescript::export::property::hint::ArrayHint` Co-authored-by: Jan Haller <bromeon@gmail.com>

bors · 2021-10-13T09:08:13Z

Build failed:

full-ci

Bromeon · 2021-10-13T09:11:18Z

GitHub confirmed issues with Action runners: https://twitter.com/githubstatus/status/1448208551416041473

Or https://www.githubstatus.com/

Investigating - We are investigating reports of degraded performance for GitHub Actions.
Oct 13, 08:46 UTC

In case it's fixed within the next hour before the timeout:
bors r+

788: Simplify module structure (redundant re-exports) r=Bromeon a=Bromeon Simplifies the module structure and closes #771. ## Changes **TLDR:** * Every symbol lives in exactly one module (+ optionally the prelude) * No symbols except modules at `gdnative` crate root (not even macros) * Some module renamings for clarification * No semantic changes -- may break imports however **Detailed list:** * Change `gdnative::object` * Now official home for `Ref`, `TRef` etc. * Move several verifying traits: `object` -> object::bounds * Move/rename `gdnative::thread_access` -> `gdnative::object::ownership` * Move/rename `gdnative::ref_kind` -> `gdnative::object::memory` * Change `gdnative::core_types` * No more nested modules * Move `gdnative::GodotError` to `gdnative::object::GodotError` * Change `gdnative::nativescript` * Rename `init` -> `export` * Rename `profiling` -> `profiler` * `export` and `user_data` no longer re-exported to `nativescript` * `class` now private, symbols re-exported to `nativescript` * Macro `profile_sig!` now in `profiler` * Improve documentation + fix doc tests * **Add** `gdnative::macros` * replaces crate root for `godot_init!` etc. * **Add** `gdnative::derive` * contains derive macros and macro attributes * Document feature flags in lib.rs and `Variant` ## Things not (yet) done Those could be discussed in separate PRs. * Changes to the prelude * Clean up the modules inside `gdnative::api` The issue here is that the modules under `api` are used for grouping related symbols. For example, `api::node` has 3 structs `Node`, `DuplicateFlags` and `PauseMode`. In C++, they would be represented as enum/struct inside a `class Node`, however Rust does not support nested types in struct, hence the module is used. An alternative would be to remove `api::Node` and just use `api::node::Node`, but this is less ergonomic. * Some very deep nestings, such as `gdnative::nativescript::export::property::hint::ArrayHint` Co-authored-by: Jan Haller <bromeon@gmail.com>

bors · 2021-10-13T09:58:14Z

Build failed:

full-ci

Bromeon · 2021-10-13T21:15:53Z

bors retry

bors · 2021-10-13T21:25:19Z

Build succeeded:

full-ci

811: Simplify module structure II r=Bromeon a=Bromeon This PR is a continuation of #788 and addresses the remaining, somewhat ambitious tasks for module cleanup. Goals: * Every symbol appears at most once in prelude * Every symbol appears at exactly once outside prelude * 2-3 modules (inside the crate) are the maximum nesting depth * Modules named according to related functionality from user point of view Changes: 1. `nativescript` module * Rename to `export`. Rationale: "native script" is quite a wide term, while most the module's current functionality is directly related to _exporting_ symbols from Rust. In practice, godot-rust is always (?) used as a native script. Other potential use cases, as a pure Godot API library or with native_calls, seem to be the exception, if at all. * Along with renaming, the `nativescript` feature is removed. * Nested symbols in `export::{properties, methods}` are moved one level up. * As a result, we can avoid very long qualifiers and multiple candidates for `use` statements. * `nativescript::init::property::hint::EnumHint` -> `export::hint::EnumHint` * `nativescript::export::method::MethodBuilder` -> `export::MethodBuilder` 1. `api` module * Remove inner types like `api::area::Area`, as they are already present in `api`. * Remove all modules which would then become empty. * Create doc links between class (`Camera2D`) and related module (`camera_2d`). 1. Smaller top-level modules * Add `init` (previously part of `nativescript`). * Add `profiler` (previously part of `nativescript`). * Extend `log` with related macros. * Remove `macros` and distribute its symbols to the most fitting API. 1. `prelude` module * Remove a few macros (`godot_gdnative_init` etc.) from the prelude, suggesting that `godot_init` should be used. * `user_data` symbols are accessible through `prelude::user_data` instead of `prelude` directly. This mirrors common usage in examples. 1. `core_types` module No changes in this PR; see PR discussion for reasons and potential alternatives. Co-authored-by: Jan Haller <bromeon@gmail.com>

Bromeon added c: core Component: core (mod core_types, object, log, init, ...) c: bindings Component: GDNative bindings (mod api) labels Sep 27, 2021

Bromeon added this to the v0.10 milestone Sep 27, 2021

Bromeon changed the title ~~771/redundant reexports~~ Simplify module structure (redundant re-exports) Sep 27, 2021

jacobsky reviewed Sep 27, 2021

View reviewed changes

Bromeon commented Sep 27, 2021

View reviewed changes

Bromeon force-pushed the 771/redundant-reexports branch 5 times, most recently from 3295c67 to a308bc7 Compare October 4, 2021 22:57

jacobsky reviewed Oct 5, 2021

View reviewed changes

Bromeon force-pushed the 771/redundant-reexports branch 2 times, most recently from 8b93d92 to 13b966b Compare October 11, 2021 20:50

Bromeon marked this pull request as ready for review October 11, 2021 21:01

Bromeon added 12 commits October 11, 2021 23:44

No longer export intermediate modules in core_types

3733ed6

Move GodotObject, Null, NewRef, Ref, TRef to 'object' module; remove …

380dbc2

…from top-level Adjust imports in derive macros, examples and UI tests

Move thread_access -> object::ownership, ref_kind -> object::memory

b7759fa

Move several verifying traits: object -> object::bounds

4dcf072

Mention features in lib.rs and Variant

e581d01

Move all derive macros and derive macro attributes to gdnative::derive

b9115d6

Do not re-export nativescript::{init, user_data}

f220898

Make nativescript::class private

56db2d6

Improve ClassBuilder doc and enable doc-tests

d9ceda7

Document modules object + nativescript

f58f658

Rename nativescript::profiling -> profiler

d6c5cb5

Rename nativescript::init -> export

0237589

jacobsky approved these changes Oct 12, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Oct 12, 2021

Try #788:

7fb4d78

Bromeon force-pushed the 771/redundant-reexports branch from 2cc56b3 to 4a3aec4 Compare October 12, 2021 19:48

bors bot added a commit that referenced this pull request Oct 12, 2021

Try #788:

6d8e021

Workaround MSRV bump in ahash

eb2c5d6

Bromeon force-pushed the 771/redundant-reexports branch from 4a3aec4 to eb2c5d6 Compare October 12, 2021 20:21

bors bot merged commit b199535 into godot-rust:master Oct 13, 2021

Bromeon deleted the 771/redundant-reexports branch October 14, 2021 15:07

Bromeon mentioned this pull request Oct 19, 2021

Re-export gdnative_core::log again #800

Merged

Bromeon mentioned this pull request Nov 2, 2021

Simplify module structure II #811

Merged

Bromeon added quality-of-life No new functionality, but improves ergonomics/internals and removed feature Adds functionality to the library labels Nov 2, 2021

		@@ -1,11 +1,16 @@
		//! Types that represent core-datatypes of Godot.
		//! Types that represent core data types of Godot.

Simplify module structure (redundant re-exports) #788

Simplify module structure (redundant re-exports) #788

Conversation

Bromeon commented Sep 27, 2021 • edited

Changes

Things not (yet) done

jacobsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobsky commented Sep 27, 2021

Bromeon commented Oct 4, 2021

jacobsky left a comment

Choose a reason for hiding this comment

Bromeon commented Oct 5, 2021

jacobsky left a comment

Choose a reason for hiding this comment

jacobsky commented Oct 12, 2021

Bromeon commented Oct 12, 2021

bors bot commented Oct 12, 2021

try

Bromeon commented Oct 12, 2021

bors bot commented Oct 12, 2021

try

Bromeon commented Oct 13, 2021

bors bot commented Oct 13, 2021

Bromeon commented Oct 13, 2021

bors bot commented Oct 13, 2021

Bromeon commented Oct 13, 2021

bors bot commented Oct 13, 2021

Bromeon commented Oct 13, 2021

bors bot commented Oct 13, 2021

Bromeon commented Sep 27, 2021 •

edited

Bromeon left a comment •

edited