Skip to content
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

Update rust nightly version and fix builds #22

Merged
merged 4 commits into from
Dec 7, 2021
Merged

Conversation

ErnWong
Copy link
Owner

@ErnWong ErnWong commented Dec 6, 2021

proc_macro_is_available is now stabilised in rust 1.57, and updated 3rd party libraries have started using them, causing them to break with old rust compiler versions.

When updating the rust version, the generic associated types in NetworkResource::ConenctionType now requires additional trait bounds.

The test_env_log is also deprecated and renamed to test_log.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2021

Codecov Report

Merging #22 (7fcbf78) into master (a0190ba) will decrease coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   77.44%   77.18%   -0.27%     
==========================================
  Files          15       15              
  Lines         838      837       -1     
==========================================
- Hits          649      646       -3     
- Misses        189      191       +2     
Impacted Files Coverage Δ
src/fixed_timestepper.rs 75.00% <ø> (ø)
src/network_resource.rs 76.92% <ø> (ø)
src/client.rs 77.43% <0.00%> (-1.05%) ⬇️
src/clocksync.rs 85.71% <0.00%> (+1.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0190ba...7fcbf78. Read the comment docs.

@ErnWong
Copy link
Owner Author

ErnWong commented Dec 6, 2021

I've blindly followed rustc's hint at adding the trait bounds for NetworkResource::ConnectionType, but I'm not sure those are the right types and they are not automatically satisfied by crystalorb_bevy_networking_turbulence::WrappedNetworkResource's usage, suggesting that my change is wrong.

Let's step back and think what the lifetime means on a ConnectionType. Here, we are essentially asking the implementation of a NetworkResource to give us a type constructor ConnectionType<'a> that we can then use and substitute our own 'a lifetime under the hood when we reborrow from a given NetworkResource.

Now, why are additional bounds required on ConnectionType?

Let's try and replicate this error with a minimal example. My first attempt does not produce the error: https://docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows#scheduled-events

#![feature(generic_associated_types)]

trait NetworkResource<WorldType> {
    type ConnectionType<'a> : Connection<WorldType>;
}

trait Connection<WorldType> {}

Could it be triggered by the use of this associated type in the trait methods?

@ErnWong
Copy link
Owner Author

ErnWong commented Dec 6, 2021

Indeed, using this generic associated type when reborrowing gives us the error:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=5dc029fa779f85d477f891af3f77da17

#![feature(generic_associated_types)]

trait NetworkResource {
    type ConnectionType<'a> : Connection;
    
    fn get(&self) -> Self::ConnectionType<'_>;
}

trait Connection {}
error: Missing required bounds on ConnectionType
 --> src/lib.rs:4:5
  |
4 |     type ConnectionType<'a> : Connection;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
  |                                         |
  |                                         help: add the required where clauses: `where Self: 'a`

error: could not compile `playground` due to previous error
Warnings

@ErnWong
Copy link
Owner Author

ErnWong commented Dec 6, 2021

See rust-lang/rust#87479
and https://users.rust-lang.org/t/associated-type-bounds-for-the-user-of-for-the-implementer/63161

If I'm understanding this correctly, the following bounds means that we are guaranteeing that the type constructor ConnectionType<'a> will never be called with inputs 's that exceeds the lifetimes of Self and WorldType:

type ConnectionType<'a>
where
    Self: 'a,
    WorldType: 'a;

This prevents some surprising and confusing errors downstream where certain implementations of ConnectionType are not allowed by the compiler, because the compiler can't guarantee that the type lives long enough.

If we didn't have these bounds added, even though we're only using ConnectionType as a reborrow of Self, the compiler doesn't know that and there's nothing stopping other users from using that ConnectionType manually with a larger lifetime. The associated type ConnectionType without those bounds imposes restrictions on what ConnectionType can be. By putting these bounds on how we can use ConnectionType, our ConnectionType now supports more possible implementations.

@ErnWong ErnWong merged commit e4b477c into master Dec 7, 2021
@ErnWong ErnWong deleted the maintenance/update branch December 7, 2021 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants