Skip to content

[FEATURE REQUEST] Don't re-export dependencies #966

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

Closed
JMS55 opened this issue Mar 9, 2022 · 20 comments
Closed

[FEATURE REQUEST] Don't re-export dependencies #966

JMS55 opened this issue Mar 9, 2022 · 20 comments
Labels
enhancement New feature or request

Comments

@JMS55
Copy link

JMS55 commented Mar 9, 2022

gtk-rs (and libadwaita-rs) reexport glib, gio, etc, along with things like the adw/gtk::subclass::prelude module rexporting glib::subclass::prelude. This leads to weird issues where Rust Analyzer has a hard time deciding which import to use, and ends up suggesting a strange one like adw::subclass::prelude::ObjectImpl or gio::subclass::prelude::ObjectSubclass.

In general, I think that we can come up with a better module system than the weird widget/trait/subclass::prelude splits, and things occasionally being inconsistent like the prelude not having everything in the parent subclass module, or the ExtManual traits being in crate::prelude instead of crate::traits, or InitializingObject not being in glib::subclass::prelude.

@JMS55 JMS55 added the enhancement New feature or request label Mar 9, 2022
@bilelmoussaoui
Copy link
Member

what is crate::traits? that is basically an implementation detail used for the automatically generated traits. But I guess, there isn't much we can do about it from gtk4-rs itself and needs a broader discussion.

@jf2048
Copy link
Member

jf2048 commented Mar 10, 2022

Not sure what can be done about this, the macros depend on having glib public, or at least parts of it anyway. Any of those traits that we refer to in a macro will need to be re-exported somehow. Would love to be able to avoid that though...

@jf2048
Copy link
Member

jf2048 commented Mar 11, 2022

I would be open to hide them away in a module called _private or something, that should actually fix some of the issues with proc-macro-crate. Don't know if that would have any effect on rust-analyzer though

@JMS55
Copy link
Author

JMS55 commented Mar 13, 2022

Quick side note: maybe this issue should be moved to gtk-rs-core or gir or somewhere more appropriate.


I opened a rust-analyzer issue to prioritize imports based on the crate that they were originally defined in rust-lang/rust-analyzer#11698. That should help for part of it.


As an example, here is the imports for subclass I made (ignoring non-gtk crates, and after manually fixing the items that RA tried to import weirdly):

use adw::subclass::prelude::AdwApplicationWindowImpl;
use adw::{TabBar, TabPage, TabView, WindowTitle};
use gio::{ActionGroup, ActionMap};
use glib::subclass::prelude::{ObjectImpl, ObjectSubclass};
use glib::subclass::types::InitializingObject;
use glib::{clone, object_subclass, IsA, Object, ObjectExt, ParamSpec};
use gtk::prelude::{GObjectPropertyExpressionExt, InitializingWidgetExt};
use gtk::subclass::prelude::{
    ApplicationWindowImpl, CompositeTemplateCallbacksClass, CompositeTemplateClass, ObjectImplExt,
    ObjectSubclassExt, ObjectSubclassIsExt, TemplateChild, WidgetClassSubclassExt, WidgetImpl,
    WindowImpl,
};
use gtk::traits::{GtkWindowExt, WidgetExt};
use gtk::{
    template_callbacks, Accessible, Application, Buildable, Button, CompositeTemplate,
    ConstraintTarget, Native, Root, ShortcutManager, Stack, Widget, Window,
};

I think on gtk-rs/libadwaita-rs's side, the following re-organization would help:

  • Widgets / Objects / consts / static_functions => gtk/adw/glib/gio (root module)
  • *Ext / *ExtManual => root::traits
  • IsA / Cast / object_subclass / CompositeTemplateCallbacksClass / ObjectSubclass (anything dealing with types, and not the types/methods themselves) => root::meta
  • *Impl => root::impls
  • Do not re-export dependency crates
  • Do not expose sub-modules, such as gtk::widgets, or glib::objects, etc
  • Do not have a prelude module (the crate itself is basically the prelude)
  • If a type from another crate is required to be re-exported for macros (assuming we can't get around this), expose only that specific type in root::_private or something similar.

That would turn my example into the following:

use adw::{TabBar, TabPage, TabView, WindowTitle};
use adw::impls::AdwApplicationWindowImpl;
use gio::{ActionGroup, ActionMap};
use glib::{clone, object_subclass, Object, ParamSpec};
use glib::impls::{ObjectImpl};
use glib::traits::ObjectExt;
use glib::meta::{InitializingObject, ObjectSubclass, IsA};
use gtk::{
    Accessible, Application, Buildable, Button,
    ConstraintTarget, Native, Root, ShortcutManager, Stack, Widget, Window,
};
use gtk::traits::{
    GtkWindowExt, WidgetExt, GObjectPropertyExpressionExt, InitializingWidgetExt, WidgetClassSubclassExt,
    ObjectImplExt, ObjectSubclassExt, ObjectSubclassIsExt
};
use gtk::impls::{ApplicationWindowImpl, WidgetImpl, WindowImpl};
use gtk::meta::{template_callbacks, CompositeTemplate, CompositeTemplateClass, CompositeTemplateCallbacksClass, TemplateChild};

Yes, this requires having all of glib/gio/gtk/etc in your Cargo.toml, which is why the re-exports were originally added. However, I think in hindsight, that that decision has made it more difficult to figure out where items are defined and how to import them, and we should reverse it.

Definitely needs more work, bikeshedding of names, and I'm unsure of the technical details having never used gir, but this is an initial proposal of what I'd like to see as a user. If you're familiar with how the bindings are generated, please weigh in!

@bilelmoussaoui
Copy link
Member

Yes, this requires having all of glib/gio/gtk/etc in your Cargo.toml, which is why the re-exports were originally added. However, I think in hindsight, that that decision has made it more difficult to figure out where items are defined and how to import them, and we should reverse it.

Definitely don't agree with this, I don't think most people care where a trait comes from

@JMS55
Copy link
Author

JMS55 commented Mar 13, 2022

I disagree, but we could leave it in. Hiding them from the docs (already done it looks like), and if rust-analyzer becomes smarter about suggesting imports, probably fixes most issues with re-exporting dependency crates.

However, I feel that having more consistent paths is still valuable. Too many things are randomly/only in the prelude/subclass::prelude, or in random modules that aren't consistent with other items.

Regardless of the exact reason, I'm definitely spending a lot of time (mostly when subclassing) tracking down which imports I need, and where I can import them from. I suppose that this could be resolved by a macro like gtk-rs/gtk-rs-core#540.

@bilelmoussaoui
Copy link
Member

I'm definitely spending a lot of time (mostly when subclassing) tracking down which imports I need, and where I can import them from.

Usually all you need is use gtk::subclass::prelude::*;

Too many things are randomly/only in the prelude/subclass::prelude, or in random modules that aren't consistent with other items.

Any specific examples ? there are probably things we missed or situation we could improve but without concrete situations, it is all just theoretical talking and there is not something actionable to work on.

@JMS55
Copy link
Author

JMS55 commented Mar 14, 2022

Any specific examples ?

Hard to remember, but I'll keep track of things next time I write a class.

@jf2048
Copy link
Member

jf2048 commented Mar 14, 2022

A class macro could reduce some of the imports related to ObjectSubclass but probably not much else. All this would still be needed outside the macro:

use adw::subclass::prelude::AdwApplicationWindowImpl;
use adw::{TabBar, TabPage, TabView, WindowTitle};
use gio::{ActionGroup, ActionMap};
use glib::subclass::prelude::{
    ObjectImpl, ObjectImplExt, ObjectSubclassExt, ObjectSubclassIsExt
};
use glib::{class, clone, IsA, Object, ObjectExt};
use gtk::prelude::{GtkWindowExt, WidgetExt};
use gtk::subclass::prelude::{
    ApplicationWindowImpl, TemplateChild, WidgetImpl, WindowImpl,
};
use gtk::{
    Accessible, Application, Buildable, Button, ConstraintTarget,
    Native, Root, ShortcutManager, Stack, Widget, Window,
};

Those traits modules probably shouldn't be public, those are unnecessary re-exports of stuff in the prelude. I wonder if we can just make those pub(crate) mod traits, if rust-analyzer really can't do anything about re-exports. Every extension trait should be in the prelude anyway

@bilelmoussaoui
Copy link
Member

@jf2048 yes the traits module shouldn't be public at all. It is only used for re-exporting in the prelude. I will fix that on gir's side and do the same for functions

@jf2048
Copy link
Member

jf2048 commented Mar 15, 2022

BTW disregarding all the widgets, all you have to put at the top is this:

use adw::prelude::*;
use adw::subclass::prelude::*;

Because libadwaita-rs includes all the other preludes.

The only other thing I think could be moved is some stuff in the submodules in glib::subclass:: (not the prelude), those are a little hard to remember

@bilelmoussaoui
Copy link
Member

The only other thing I think could be moved is some stuff in the submodules in glib::subclass:: (not the prelude), those are a little hard to remember

Indeed, stuff like glib::subclass::Signal but ideally those should no longer be "manually" needed once we have macros for generating them

@ebassi
Copy link

ebassi commented Mar 29, 2022

Usually all you need is use gtk::subclass::prelude::*;

This is only even remotely true if all you're using is GTK.

Once you start using libadwaita and gstreamer—in other words, once you start writing a useful application instead of an example—you get really deep into the woods, and you need to go down a Choose Your Own Adventure path:

  • split everything into separate modules and then pay the price of maintaining a complex internal API surface
  • break apart the prelude::* glob and go down an ever increasing atomisation of the use directives
  • give up, and move to another language

@sdroege
Copy link
Member

sdroege commented Mar 29, 2022

This is only even remotely true if all you're using is GTK.

Once you start using libadwaita and gstreamer [...]

Can you provide an example project where this causes problems? I'm not entirely sure I understand the situation.

There is quite some code out there that imports preludes of GTK, Adwaita and GStreamer inside the same module and that's working fine. Re-exports of the same type/trait are handled as equivalent, no matter where they come from.

The only case where this causes problems is if preludes re-export types (bad idea) or if you have multiple versions of a crate that provides a trait (e.g. glib). In case of the latter you'll run into problems sooner or later anyway as different parts would e.g. disagree what a glib::Object is.

@bilelmoussaoui
Copy link
Member

Closing as there is nothing actionable on the issue itself, feel free to re-open if someone has a proper "broken" use case so we can see what needs to be fixed/improved.

@sdroege
Copy link
Member

sdroege commented Oct 11, 2022 via email

@ebassi
Copy link

ebassi commented Oct 12, 2022

[Side note: we ended up discussing about this on Matrix]

The main issue is exactly the case where the prelude ends up loading different versions of the dependencies. Yes: bad things may happen anyway down the line, but the whole thing is quite unergonomic and hard to debug. It's also a side effect of using one module from Git and an unrelated module from a stable release—for instance, using libadwaita from Git and GStreamer from a stable tag. Nominally, those two are completely different leaves of the dependency tree, but of course they end up interacting. Forcing to switch everything to Git or to stable releases is quite annoying because it removes a lot of the granularity nominally available.

Of course, I don't really have any real solution, but it would be nice to have a better error message instead of a dump of weird conflicts.

@sdroege
Copy link
Member

sdroege commented Oct 12, 2022

What's the confusing error you mean? X does not implement IsA<Object> or expected X but got X or similar?

@ebassi
Copy link

ebassi commented Oct 12, 2022

Something to that effect, yes. It's been more than 6 months, and I was able to figure out what the issue was.

At the very least, I'd be happy with a FAQ somewhere, explaining that if you mix dependencies and they bring their own versions of the dependencies, then you might get this kind of errors during compilation; the solution is to either use Git for every dependency involved with glib-rs/gtk-rs, or to use stable releases everywhere.

@Hofer-Julian
Copy link
Collaborator

At the very least, I'd be happy with a FAQ somewhere, explaining that if you mix dependencies and they bring their own versions of the dependencies, then you might get this kind of errors during compilation; the solution is to either use Git for every dependency involved with glib-rs/gtk-rs, or to use stable releases everywhere.

Sounds like a good issue for: https://github.com/gtk-rs/gtk-rs.github.io/issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants