Skip to content

Update to Rust 1.53.0 #559

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

Merged
merged 9 commits into from
Jun 19, 2021
Merged

Update to Rust 1.53.0 #559

merged 9 commits into from
Jun 19, 2021

Conversation

iceiix
Copy link
Owner

@iceiix iceiix commented Jun 18, 2021

No description provided.

@iceiix
Copy link
Owner Author

iceiix commented Jun 18, 2021

New clippy warnings. Fixed most of the easy ones, but some will require more thought. For example this one:

warning: all if blocks contain the same code at the end
   --> src/world/mod.rs:671:13
    |
671 | /                 self.chunks.get_mut(&cpos).unwrap()
672 | |             };
    | |_____________^
    |
    = note: `#[warn(clippy::branches_sharing_code)]` on by default
    = note: The end suggestion probably needs some adjustments to use the expression result correctly
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code
help: consider moving the end statements out like this
    |
671 |             }
672 |             self.chunks.get_mut(&cpos).unwrap();
    |

This occurs because the code is duplicated, it wants to pull out the last line of the if and else bodies:

            let chunk = if new {
                self.chunks.insert(cpos, Chunk::new(cpos));
                self.chunks.get_mut(&cpos).unwrap()
            } else {
                if !self.chunks.contains_key(&cpos) {
                    return Ok(());
                }
                self.chunks.get_mut(&cpos).unwrap()
            };

but of course, naively extracting self.chunks.get_mut(&cpos).unwrap() like this won't work: (actually…)

            if new {
                self.chunks.insert(cpos, Chunk::new(cpos));
            } else {
                if !self.chunks.contains_key(&cpos) {
                    return Ok(());
                }
            };
            let chunk = self.chunks.get_mut(&cpos).unwrap()

because the if statement is used as an expression to assign to chunk, and return Ok(()) is also supposed to get assigned to this variable (correction: it returns to the function, not evaluates), if !new && !self.chunks.contains_key(&cpos). The same pattern is repeated in load_chunk18, load_uncompressed_chunk17, and load_chunk19_or_115. Need to think about how to write this more idiomatically.

@iceiix
Copy link
Owner Author

iceiix commented Jun 18, 2021

The other clippy issue, this time an error, is:

error: this call for this type may be undefined behavior
   --> src/ecs/mod.rs:604:34
    |
604 |                 let mut val: T = mem::MaybeUninit::uninit().assume_init();
    |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[deny(clippy::uninit_assumed_init)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
impl ComponentMem {
    fn new<T>() -> ComponentMem {
        ComponentMem {
            data: vec![],
            component_size: mem::size_of::<T>(),
            drop_func: Box::new(|data| unsafe {
                let mut val: T = mem::MaybeUninit::uninit().assume_init();
                ptr::copy(data as *mut T, &mut val, 1);
                mem::drop(val);
            }),
        }
    }

git blame shows this code was last changed in 518b6a07 Reformat all source with cargo fmt. Before reformatting, mem::MaybeUninit::uninit was added in 8502c03 "Update to Rust 1.40.0. Closes #253"

--- a/src/ecs/mod.rs
+++ b/src/ecs/mod.rs
@@ -481,7 +481,7 @@ impl ComponentMem {
             component_size: mem::size_of::<T>(),
             drop_func: Box::new(|data| {
                 unsafe {
-                    let mut val: T = mem::uninitialized();
+                    let mut val: T = mem::MaybeUninit::uninit().assume_init();
                     ptr::copy(data as *mut T, &mut val, 1);
                     mem::drop(val);
                 }

This is in unsafe, but https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init says "For now, we accept empty tuples and tuples / arrays of MaybeUninit. There may be other types that allow uninitialized data, but those are not yet rigorously defined."

The lint was added in rust-lang/rust-clippy#4272

GitHub search brings up a total of 62 matches even for a fairly restrictive search query: https://github.com/search?q=%22uninit%28%29%3A%3Aassume_init%28%29%22&type=Code

Problem: It seems people are using this as an easy way out of the deprecation warnings for std::mem::uninitialized, failing to understand the reason why it was deprecated (it is, to my current understanding, universally UB, always!).

Sure enough, I added this in #253 (comment) for this reason, and the GitHub query results for this anti-pattern includes this project. The author continues:

The correct solution is to use MaybeUninit<T> where you were formerly using T until the value is known to be initialized. The MaybeUninit<T> docs even give examples for what are by far the two most common use cases (out pointers and partially-initialized arrays).

@iceiix iceiix force-pushed the rust1.53 branch 2 times, most recently from ce75a0b to dbdbaba Compare June 19, 2021 01:14
@iceiix iceiix merged commit 72f5fdf into master Jun 19, 2021
@iceiix iceiix deleted the rust1.53 branch June 19, 2021 02:47
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.

1 participant