Cycle detected in struct with multiple trait borrows

I'm working on a pet project involving syncing steam and notion data, partly to improve my understanding of Rust, but I've been a bit lazy about integration testing it and I'm trying to get it into a state where I can properly integration test it.

I have a Sync struct which does a load of scraping from both steam games and notion notes and updates a database. This is achieved with owned Repo, SteamClient and NotionGamesRepo properties respectively:

pub struct Sync {
    steam_account_id: String,
    pub repo: Repo,
    steam: SteamClient,
    notion: NotionGamesRepo,
}

and this has been working fine.

I have SteamClient broken into traits already, so to properly test the sync process I want Sync to take a reference to the trait instead, so I can sub in a mock. Following advice on a stack overflow thread I changed it to this, which also worked fine:

pub struct Sync<'s> {
    steam_account_id: String,
    pub repo: Repo,
    steam: &'s dyn SteamHandling,
    notion: NotionGamesRepo
}

impl<'s> Sync<'s> {
    ...
}

I'm fairly new to Rust so my understanding of lifecycle parameters probably isn't up to scratch, but my understanding is 's is specifying that the &'s dyn SteamHandling borrow must live at least as long as my Sync<'s>.

So far, so good; I treated NotionGamesRepo the same way, creating a trait for it:

pub struct Sync<'s, 'n> {
    steam_account_id: String,
    pub repo: Repo,
    steam: &'s dyn SteamHandling,
    notion: &'n dyn NotionHandling
}

impl<'s, 'n> Sync<'s, 'n> {
    ...
}

Now it all falls apart and my understanding of lifecycles and compiler queries fails me; I can't make much sense out of the compiler error:

error[E0391]: cycle detected when checking effective visibilities
    |
note: ...which requires computing type of `cli::sync::<impl at src/cli/sync.rs:99:1: 99:13>::run::{opaque#0}`...
   --> src/cli/sync.rs:101:5
    |
101 |     pub(super) async fn run(&self) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires computing type of opaque `cli::sync::<impl at src/cli/sync.rs:99:1: 99:13>::run::{opaque#0}`...
   --> src/cli/sync.rs:101:5
    |
101 |     pub(super) async fn run(&self) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires type-checking `cli::sync::<impl at src/cli/sync.rs:99:1: 99:13>::run`...
   --> src/cli/sync.rs:101:5
    |
101 |     pub(super) async fn run(&self) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: ...which again requires checking effective visibilities, completing the cycle
note: cycle used when checking that `db::sync::Sync` is well-formed
   --> src/db/sync.rs:30:1
    |
30  | pub struct Sync<'s, 'n> {
    | ^^^^^^^^^^^^^^^^^^^^^^^
    = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

For more information about this error, try `rustc --explain E0391`.

I can see it's struggling to prove that the type for Sync makes sense during compilation, and I had a quick read of the link it provides, but it doesn't really give me much clue how to properly achieve what I'm attempting here. I also tried using a single lifecycle parameter 'a instead of two distinct ones, but got an identical error that way too.

I've also pushed this draft to a branch: rusteam/src/db/sync.rs at it-test2 · giftig/rusteam · GitHub

I initially made everything owned as it was simpler, and the advice I saw is that a struct should own its members unless there's a good reason not to, and lifecycle rules are well understood: clearly this supports that idea. But taking a trait, it can't own it as it's abstract. I also considered making the struct generic with two type parameters, instead of using borrows, but that seems like it'd get quite complex as well. I also need to reuse the Repo later so I've made it public on the struct so it can be reused from there -- which seems like code smell and implying it should also be borrowed.

So, two questions:
(1) what's causing a cycle here exactly and can it be avoided with a simple change?
(2) Is this the right way to go about allowing DI for testing? Is there a simpler approach which will avoid falling afoul of lifecycle complexity?

Thanks in advance.

Your struct can own a trait object (which isn’t quite the same thing as a trait) by storing Box<dyn Trait> instead of &’a dyn Trait.

2 Likes

Thanks @2e71828, I just simplified this with Box as you suggested:

pub struct Sync {
    steam_account_id: String,
    pub repo: Repo,
    steam: Box<dyn SteamHandling>,
    notion: Box<dyn NotionHandling>
}

but I still get the same cycle error:

error[E0391]: cycle detected when checking effective visibilities
    |
note: ...which requires computing type of `cli::sync::<impl at src/cli/sync.rs:99:1: 99:13>::run::{opaque#0}`...
   --> src/cli/sync.rs:101:5
    |
101 |     pub(super) async fn run(&self) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires computing type of opaque `cli::sync::<impl at src/cli/sync.rs:99:1: 99:13>::run::{opaque#0}`...
   --> src/cli/sync.rs:101:5
    |
101 |     pub(super) async fn run(&self) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires type-checking `cli::sync::<impl at src/cli/sync.rs:99:1: 99:13>::run`...
   --> src/cli/sync.rs:101:5
    |
101 |     pub(super) async fn run(&self) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: ...which again requires checking effective visibilities, completing the cycle
note: cycle used when checking that `db::sync::Sync` is well-formed
   --> src/db/sync.rs:30:1
    |
30  | pub struct Sync {
    | ^^^^^^^^^^^^^^^
    = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

Actually if I leave SteamClient as a concrete type and no Box, but I specify Box<dyn NotionHandling> the error also crops up, but it was fine with a concrete notion struct and a trait for the steam client. So the error doesn't seem to be related to lifetimes or traits at all, there must be some issue with the NotionHandling trait specifically.

I don't see anything different between the way I've implemented the traits, though, and the error above doesn't give me any hints, since it's mostly just pointing to the same line and nothing to do with the trait that seems to be problematic.

Edit:

Ok, I think I have it. My NotionGamesRepo contains an async fn which I put onto the trait when I defined it. I've now discovered async fn can't live in a trait without some workarounds and should produce an error message that the trait is not object-safe.

But it looks like I've also fallen afoul of `cycle detected when checking effective visibility` hides relevant diagnostics message · Issue #119502 · rust-lang/rust · GitHub which makes the error much more obscure and confusing (like I'm seeing above) when it should be clearly stating the object-safe problem.

I'll try using the workaround crate.

Probably unrelated, but I noticed a repeated super trait:

pub trait SteamHandling:
    SteamPlayerServiceHandling +
    SteamAppsServiceHandling +
    SteamAppsServiceHandling +
    SteamAppDetailsHandling {}
2 Likes

I've found a solution: I used async_trait to fix the trait I defined for the notion struct, and that fixed the object-safety issues inherent in traits with async fn. The error was confusing because of a known compiler bug which has since been fixed (I'll update my rust version).

The borrowing wasn't the real problem but thanks for the Box advice @2e71828, that seems like a simpler solution for now.

Also thanks @jumpnbrownweasel I didn't notice that duplication and it seems the compiler is indifferent to it, but I fixed it :+1:

2 Likes

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.