From 318c5d6c963d0e5d8ece89e804b4e696c6011955 Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Mon, 8 Jul 2019 21:34:36 -0700 Subject: [PATCH 01/18] Clarify `Box` representation and its use in FFI This officializes what was only shown as a code example in [the unsafe code guidelines](https://rust-lang.github.io/unsafe-code-guidelines/layout/function-pointers.html?highlight=box#use) and follows [the discussion](https://github.com/rust-lang/unsafe-code-guidelines/issues/157) in the corresponding repository. It is also related to [the issue](https://github.com/rust-lang/rust/issues/52976) regarding marking `Box` `#[repr(transparent)]`. --- src/liballoc/boxed.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/liballoc/boxed.rs b/src/liballoc/boxed.rs index 01dee0a394337..50174c3f279a2 100644 --- a/src/liballoc/boxed.rs +++ b/src/liballoc/boxed.rs @@ -63,6 +63,28 @@ //! T` obtained from `Box::::into_raw` may be deallocated using the //! [`Global`] allocator with `Layout::for_value(&*value)`. //! +//! `Box` has the same representation as `*mut T`. In particular, when +//! `T: Sized`, this means that `Box` has the same representation as +//! a C pointer, making the following code valid in FFI: +//! +//! ```c +//! /* C header */ +//! struct Foo* foo(); /* Returns ownership */ +//! void bar(struct Foo*); /* `bar` takes ownership */ +//! ``` +//! +//! ``` +//! #[repr(C)] +//! pub struct Foo; +//! +//! #[no_mangle] +//! pub extern "C" fn foo() -> Box { +//! Box::new(Foo) +//! } +//! +//! #[no_mangle] +//! pub extern "C" fn bar(_: Option>) {} +//! ``` //! //! [dereferencing]: ../../std/ops/trait.Deref.html //! [`Box`]: struct.Box.html From aea94230c476fea2ee073a1bc672125e4586d4b5 Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Sun, 25 Aug 2019 23:25:56 -0700 Subject: [PATCH 02/18] Update Box representation comment based on reviews --- src/liballoc/boxed.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/liballoc/boxed.rs b/src/liballoc/boxed.rs index 50174c3f279a2..b2b23cc9671b1 100644 --- a/src/liballoc/boxed.rs +++ b/src/liballoc/boxed.rs @@ -63,9 +63,8 @@ //! T` obtained from `Box::::into_raw` may be deallocated using the //! [`Global`] allocator with `Layout::for_value(&*value)`. //! -//! `Box` has the same representation as `*mut T`. In particular, when -//! `T: Sized`, this means that `Box` has the same representation as -//! a C pointer, making the following code valid in FFI: +//! `Box` has the same ABI as `&mut T`. In particular, when `T: Sized`, +//! this allows using `Box` in FFI: //! //! ```c //! /* C header */ From 812ec6a3bf775c1564ed3b12374c4ee81bfa94b8 Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Wed, 30 Oct 2019 09:43:04 -0700 Subject: [PATCH 03/18] Update FFI example - Use meaningful names - Clarify comments - Fix C function declaration --- src/liballoc/boxed.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/liballoc/boxed.rs b/src/liballoc/boxed.rs index b2b23cc9671b1..a502a5b0a0b3a 100644 --- a/src/liballoc/boxed.rs +++ b/src/liballoc/boxed.rs @@ -68,8 +68,8 @@ //! //! ```c //! /* C header */ -//! struct Foo* foo(); /* Returns ownership */ -//! void bar(struct Foo*); /* `bar` takes ownership */ +//! struct Foo* foo_new(void); /* Returns ownership to the caller */ +//! void foo_delete(struct Foo*); /* Takes ownership from the caller */ //! ``` //! //! ``` @@ -77,12 +77,12 @@ //! pub struct Foo; //! //! #[no_mangle] -//! pub extern "C" fn foo() -> Box { +//! pub extern "C" fn foo_new() -> Box { //! Box::new(Foo) //! } //! //! #[no_mangle] -//! pub extern "C" fn bar(_: Option>) {} +//! pub extern "C" fn foo_delete(_: Option>) {} //! ``` //! //! [dereferencing]: ../../std/ops/trait.Deref.html From 3aa99020250e1369dfac10952625d4ad6d9d8190 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Tue, 3 Dec 2019 12:35:09 +0100 Subject: [PATCH 04/18] Fix #66295 --- src/librustc_lint/unused.rs | 8 +++++-- src/test/ui/lint/lint-unnecessary-parens.rs | 9 ++++++++ .../ui/lint/lint-unnecessary-parens.stderr | 22 +++++++++---------- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index f7de7ec7e18f4..25c351f623488 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -17,7 +17,7 @@ use syntax::print::pprust; use syntax::symbol::{kw, sym}; use syntax::symbol::Symbol; use syntax::util::parser; -use syntax_pos::{Span, BytePos}; +use syntax_pos::{MultiSpan, Span, BytePos}; use log::debug; @@ -355,7 +355,11 @@ impl UnusedParens { match value.kind { ast::ExprKind::Paren(ref inner) => { if !Self::is_expr_parens_necessary(inner, followed_by_block) && - value.attrs.is_empty() { + value.attrs.is_empty() && + !MultiSpan::from(value.span).primary_span() + .map(|span| span.from_expansion()) + .unwrap_or(false) + { let expr_text = if let Ok(snippet) = cx.sess().source_map() .span_to_snippet(value.span) { snippet diff --git a/src/test/ui/lint/lint-unnecessary-parens.rs b/src/test/ui/lint/lint-unnecessary-parens.rs index 9f42b855a870d..12ffb6d3c6655 100644 --- a/src/test/ui/lint/lint-unnecessary-parens.rs +++ b/src/test/ui/lint/lint-unnecessary-parens.rs @@ -25,6 +25,12 @@ fn passes_unused_parens_lint() -> &'static (dyn Trait) { panic!() } +macro_rules! baz { + ($($foo:expr),+) => { + ($($foo),*) + } +} + fn main() { foo(); bar((true)); //~ ERROR unnecessary parentheses around function argument @@ -55,4 +61,7 @@ fn main() { let mut _a = (0); //~ ERROR unnecessary parentheses around assigned value _a = (0); //~ ERROR unnecessary parentheses around assigned value _a += (1); //~ ERROR unnecessary parentheses around assigned value + + let _a = baz!(3, 4); + let _b = baz!(3); } diff --git a/src/test/ui/lint/lint-unnecessary-parens.stderr b/src/test/ui/lint/lint-unnecessary-parens.stderr index adc1069b64d62..541ae7aa4b54a 100644 --- a/src/test/ui/lint/lint-unnecessary-parens.stderr +++ b/src/test/ui/lint/lint-unnecessary-parens.stderr @@ -23,25 +23,25 @@ LL | fn unused_parens_around_return_type() -> (u32) { | ^^^^^ help: remove these parentheses error: unnecessary parentheses around function argument - --> $DIR/lint-unnecessary-parens.rs:30:9 + --> $DIR/lint-unnecessary-parens.rs:36:9 | LL | bar((true)); | ^^^^^^ help: remove these parentheses error: unnecessary parentheses around `if` condition - --> $DIR/lint-unnecessary-parens.rs:32:8 + --> $DIR/lint-unnecessary-parens.rs:38:8 | LL | if (true) {} | ^^^^^^ help: remove these parentheses error: unnecessary parentheses around `while` condition - --> $DIR/lint-unnecessary-parens.rs:33:11 + --> $DIR/lint-unnecessary-parens.rs:39:11 | LL | while (true) {} | ^^^^^^ help: remove these parentheses warning: denote infinite loops with `loop { ... }` - --> $DIR/lint-unnecessary-parens.rs:33:5 + --> $DIR/lint-unnecessary-parens.rs:39:5 | LL | while (true) {} | ^^^^^^^^^^^^ help: use `loop` @@ -49,43 +49,43 @@ LL | while (true) {} = note: `#[warn(while_true)]` on by default error: unnecessary parentheses around `match` head expression - --> $DIR/lint-unnecessary-parens.rs:35:11 + --> $DIR/lint-unnecessary-parens.rs:41:11 | LL | match (true) { | ^^^^^^ help: remove these parentheses error: unnecessary parentheses around `let` head expression - --> $DIR/lint-unnecessary-parens.rs:38:16 + --> $DIR/lint-unnecessary-parens.rs:44:16 | LL | if let 1 = (1) {} | ^^^ help: remove these parentheses error: unnecessary parentheses around `let` head expression - --> $DIR/lint-unnecessary-parens.rs:39:19 + --> $DIR/lint-unnecessary-parens.rs:45:19 | LL | while let 1 = (2) {} | ^^^ help: remove these parentheses error: unnecessary parentheses around method argument - --> $DIR/lint-unnecessary-parens.rs:53:24 + --> $DIR/lint-unnecessary-parens.rs:59:24 | LL | X { y: false }.foo((true)); | ^^^^^^ help: remove these parentheses error: unnecessary parentheses around assigned value - --> $DIR/lint-unnecessary-parens.rs:55:18 + --> $DIR/lint-unnecessary-parens.rs:61:18 | LL | let mut _a = (0); | ^^^ help: remove these parentheses error: unnecessary parentheses around assigned value - --> $DIR/lint-unnecessary-parens.rs:56:10 + --> $DIR/lint-unnecessary-parens.rs:62:10 | LL | _a = (0); | ^^^ help: remove these parentheses error: unnecessary parentheses around assigned value - --> $DIR/lint-unnecessary-parens.rs:57:11 + --> $DIR/lint-unnecessary-parens.rs:63:11 | LL | _a += (1); | ^^^ help: remove these parentheses From cc4ced88404d417fadb94d706b184f01d6064fbe Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Tue, 3 Dec 2019 14:07:45 +0000 Subject: [PATCH 05/18] Apply suggestions from code review Co-Authored-By: lzutao --- src/librustc_lint/unused.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 25c351f623488..ee644acd09974 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -357,8 +357,7 @@ impl UnusedParens { if !Self::is_expr_parens_necessary(inner, followed_by_block) && value.attrs.is_empty() && !MultiSpan::from(value.span).primary_span() - .map(|span| span.from_expansion()) - .unwrap_or(false) + .map_or(false, |span| span.from_expansion()) { let expr_text = if let Ok(snippet) = cx.sess().source_map() .span_to_snippet(value.span) { From 9598cf416dcfba4e15770f57447b3996d1853fc1 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Fri, 6 Dec 2019 15:09:01 +0100 Subject: [PATCH 06/18] Remove failing test case --- ...e-47775-nested-macro-unnecessary-parens-arg.rs | 3 --- ...775-nested-macro-unnecessary-parens-arg.stderr | 15 --------------- 2 files changed, 18 deletions(-) delete mode 100644 src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.stderr diff --git a/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.rs b/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.rs index ab9baa79b8b77..0a951cfa91c58 100644 --- a/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.rs +++ b/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.rs @@ -17,10 +17,7 @@ macro_rules! the_worship_the_heart_lifts_above { macro_rules! and_the_heavens_reject_not { () => { - // ↓ But let's test that we still lint for unused parens around - // function args inside of simple, one-deep macros. #[allow(dead_code)] fn the_night_for_the_morrow() -> Option { Some((2)) } - //~^ WARN unnecessary parentheses around function argument } } diff --git a/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.stderr b/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.stderr deleted file mode 100644 index 57cdcd70e9db4..0000000000000 --- a/src/test/ui/lint/issue-47775-nested-macro-unnecessary-parens-arg.stderr +++ /dev/null @@ -1,15 +0,0 @@ -warning: unnecessary parentheses around function argument - --> $DIR/issue-47775-nested-macro-unnecessary-parens-arg.rs:22:83 - | -LL | #[allow(dead_code)] fn the_night_for_the_morrow() -> Option { Some((2)) } - | ^^^ help: remove these parentheses -... -LL | and_the_heavens_reject_not!(); - | ------------------------------ in this macro invocation - | -note: lint level defined here - --> $DIR/issue-47775-nested-macro-unnecessary-parens-arg.rs:3:9 - | -LL | #![warn(unused_parens)] - | ^^^^^^^^^^^^^ - From ead115949017533de244049c58f4b6886243eda7 Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Mon, 9 Dec 2019 22:49:59 -0800 Subject: [PATCH 07/18] Use Niko's wording --- src/liballoc/boxed.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/liballoc/boxed.rs b/src/liballoc/boxed.rs index a502a5b0a0b3a..a7e09d72b4a92 100644 --- a/src/liballoc/boxed.rs +++ b/src/liballoc/boxed.rs @@ -63,14 +63,25 @@ //! T` obtained from `Box::::into_raw` may be deallocated using the //! [`Global`] allocator with `Layout::for_value(&*value)`. //! -//! `Box` has the same ABI as `&mut T`. In particular, when `T: Sized`, -//! this allows using `Box` in FFI: +//! So long as `T: Sized`, a `Box` is guaranteed to be represented as a +//! single pointer and is also ABI-compatible with C pointers (i.e. the C type +//! `T*`). This means that you have Rust code which passes ownership of a +//! `Box` to C code by using `Box` as the type on the Rust side, and +//! `T*` as the corresponding type on the C side. As an example, consider this +//! C header which declares functions that create and destroy some kind of +//! `Foo` value: //! //! ```c //! /* C header */ //! struct Foo* foo_new(void); /* Returns ownership to the caller */ //! void foo_delete(struct Foo*); /* Takes ownership from the caller */ //! ``` +//! +//! These two functions might be implemented in Rust as follows. Here, the +//! `struct Foo*` type from C is translated to `Box`, which captures +//! the ownership constraints. Note also that the nullable argument to +//! `foo_delete` is represented in Rust as `Option>`, since `Box` +//! cannot be null. //! //! ``` //! #[repr(C)] @@ -84,6 +95,14 @@ //! #[no_mangle] //! pub extern "C" fn foo_delete(_: Option>) {} //! ``` +//! +//! Even though `Box` has the same representation and C ABI as a C pointer, +//! this does not mean that you can convert an arbitrary `T*` into a `Box` +//! and expect things to work. `Box` values will always be fully aligned, +//! non-null pointers. Moreover, the destructor for `Box` will attempt to +//! free the value with the global allocator. In general, the best practice +//! is to only use `Box` for pointers that originated from the global +//! allocator. //! //! [dereferencing]: ../../std/ops/trait.Deref.html //! [`Box`]: struct.Box.html From fe6ddd5d15c4f0108c32fb731b688f34fbeba788 Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Mon, 9 Dec 2019 22:56:37 -0800 Subject: [PATCH 08/18] Specify behavior when passed a null pointer --- src/liballoc/boxed.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/liballoc/boxed.rs b/src/liballoc/boxed.rs index a7e09d72b4a92..2f24086bf2c98 100644 --- a/src/liballoc/boxed.rs +++ b/src/liballoc/boxed.rs @@ -73,8 +73,12 @@ //! //! ```c //! /* C header */ -//! struct Foo* foo_new(void); /* Returns ownership to the caller */ -//! void foo_delete(struct Foo*); /* Takes ownership from the caller */ +//! +//! /* Returns ownership to the caller */ +//! struct Foo* foo_new(void); +//! +//! /* Takes ownership from the caller; no-op when invoked with NULL */ +//! void foo_delete(struct Foo*); //! ``` //! //! These two functions might be implemented in Rust as follows. Here, the From 1a26df77272d3bafc9e9e689a5a4a314896050f5 Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Tue, 10 Dec 2019 00:00:25 -0800 Subject: [PATCH 09/18] Remove trailing whitespace --- src/liballoc/boxed.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/liballoc/boxed.rs b/src/liballoc/boxed.rs index 2f24086bf2c98..c3732b245f420 100644 --- a/src/liballoc/boxed.rs +++ b/src/liballoc/boxed.rs @@ -73,14 +73,14 @@ //! //! ```c //! /* C header */ -//! +//! //! /* Returns ownership to the caller */ //! struct Foo* foo_new(void); -//! +//! //! /* Takes ownership from the caller; no-op when invoked with NULL */ //! void foo_delete(struct Foo*); //! ``` -//! +//! //! These two functions might be implemented in Rust as follows. Here, the //! `struct Foo*` type from C is translated to `Box`, which captures //! the ownership constraints. Note also that the nullable argument to @@ -99,7 +99,7 @@ //! #[no_mangle] //! pub extern "C" fn foo_delete(_: Option>) {} //! ``` -//! +//! //! Even though `Box` has the same representation and C ABI as a C pointer, //! this does not mean that you can convert an arbitrary `T*` into a `Box` //! and expect things to work. `Box` values will always be fully aligned, From c681841ca078d6f29776d961ba8e44196662413e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 11 Dec 2019 14:17:41 +1100 Subject: [PATCH 10/18] Fix `-Z print-type-sizes`'s handling of zero-sized fields. Currently, the type `struct S { x: u32, y: u32, tag: () }` is incorrectly described like this: ``` print-type-size type: `S`: 8 bytes, alignment: 4 bytes print-type-size field `.x`: 4 bytes print-type-size field `.tag`: 0 bytes, offset: 0 bytes, alignment: 1 bytes print-type-size padding: 4 bytes print-type-size field `.y`: 4 bytes, alignment: 4 bytes ``` Specifically: - The `padding` line is wrong. (There is no padding.) - The `offset` and `alignment` on the `.tag` line shouldn't be printed. The problem is that multiple fields can end up with the same offset, and the printing code doesn't handle this correctly. This commit fixes it by adjusting the field sorting so that zero-sized fields are dealt with before non-zero-sized fields. With that in place, the printing code works correctly. The commit also corrects the "something is very wrong" comment. The new output looks like this: ``` print-type-size type: `S`: 8 bytes, alignment: 4 bytes print-type-size field `.tag`: 0 bytes print-type-size field `.x`: 4 bytes print-type-size field `.y`: 4 bytes ``` --- src/librustc_session/code_stats.rs | 9 ++-- .../ui/print_type_sizes/zero-sized-fields.rs | 46 +++++++++++++++++++ .../print_type_sizes/zero-sized-fields.stdout | 16 +++++++ 3 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 src/test/ui/print_type_sizes/zero-sized-fields.rs create mode 100644 src/test/ui/print_type_sizes/zero-sized-fields.stdout diff --git a/src/librustc_session/code_stats.rs b/src/librustc_session/code_stats.rs index 5baf0c5948f28..764d6d8eaee30 100644 --- a/src/librustc_session/code_stats.rs +++ b/src/librustc_session/code_stats.rs @@ -132,9 +132,12 @@ impl CodeStats { let mut min_offset = discr_size; - // We want to print fields by increasing offset. + // We want to print fields by increasing offset. We also want + // zero-sized fields before non-zero-sized fields, otherwise + // the loop below goes wrong; hence the `f.size` in the sort + // key. let mut fields = fields.clone(); - fields.sort_by_key(|f| f.offset); + fields.sort_by_key(|f| (f.offset, f.size)); for field in fields.iter() { let FieldInfo { ref name, offset, size, align } = *field; @@ -146,7 +149,7 @@ impl CodeStats { } if offset < min_offset { - // if this happens something is very wrong + // If this happens it's probably a union. println!("print-type-size {}field `.{}`: {} bytes, \ offset: {} bytes, \ alignment: {} bytes", diff --git a/src/test/ui/print_type_sizes/zero-sized-fields.rs b/src/test/ui/print_type_sizes/zero-sized-fields.rs new file mode 100644 index 0000000000000..2ad488e8d8fb9 --- /dev/null +++ b/src/test/ui/print_type_sizes/zero-sized-fields.rs @@ -0,0 +1,46 @@ +// compile-flags: -Z print-type-sizes +// build-pass (FIXME(62277): could be check-pass?) + +// At one point, zero-sized fields such as those in this file were causing +// incorrect output from `-Z print-type-sizes`. + +#![feature(start)] + +struct S1 { + x: u32, + y: u32, + tag: (), +} + +struct Void(); +struct Empty {} + +struct S5 { + tagw: TagW, + w: u32, + unit: (), + x: u32, + void: Void, + y: u32, + empty: Empty, + z: u32, + tagz: TagZ, +} + +#[start] +fn start(_: isize, _: *const *const u8) -> isize { + let _s1: S1 = S1 { x: 0, y: 0, tag: () }; + + let _s5: S5<(), Empty> = S5 { + tagw: (), + w: 1, + unit: (), + x: 2, + void: Void(), + y: 3, + empty: Empty {}, + z: 4, + tagz: Empty {}, + }; + 0 +} diff --git a/src/test/ui/print_type_sizes/zero-sized-fields.stdout b/src/test/ui/print_type_sizes/zero-sized-fields.stdout new file mode 100644 index 0000000000000..72f59c4bb57bf --- /dev/null +++ b/src/test/ui/print_type_sizes/zero-sized-fields.stdout @@ -0,0 +1,16 @@ +print-type-size type: `S5<(), Empty>`: 16 bytes, alignment: 4 bytes +print-type-size field `.tagw`: 0 bytes +print-type-size field `.unit`: 0 bytes +print-type-size field `.void`: 0 bytes +print-type-size field `.empty`: 0 bytes +print-type-size field `.tagz`: 0 bytes +print-type-size field `.w`: 4 bytes +print-type-size field `.x`: 4 bytes +print-type-size field `.y`: 4 bytes +print-type-size field `.z`: 4 bytes +print-type-size type: `S1`: 8 bytes, alignment: 4 bytes +print-type-size field `.tag`: 0 bytes +print-type-size field `.x`: 4 bytes +print-type-size field `.y`: 4 bytes +print-type-size type: `Empty`: 0 bytes, alignment: 1 bytes +print-type-size type: `Void`: 0 bytes, alignment: 1 bytes From cb1cc1181e884d0f53c444af0b6a21189af83bea Mon Sep 17 00:00:00 2001 From: Stephane Raux Date: Tue, 10 Dec 2019 22:18:17 -0800 Subject: [PATCH 11/18] Fix description based on review --- src/liballoc/boxed.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/liballoc/boxed.rs b/src/liballoc/boxed.rs index c3732b245f420..d50b4b8dc4a7b 100644 --- a/src/liballoc/boxed.rs +++ b/src/liballoc/boxed.rs @@ -65,7 +65,7 @@ //! //! So long as `T: Sized`, a `Box` is guaranteed to be represented as a //! single pointer and is also ABI-compatible with C pointers (i.e. the C type -//! `T*`). This means that you have Rust code which passes ownership of a +//! `T*`). This means that you can have Rust code which passes ownership of a //! `Box` to C code by using `Box` as the type on the Rust side, and //! `T*` as the corresponding type on the C side. As an example, consider this //! C header which declares functions that create and destroy some kind of From ab3f4fd709fc2fcb19cda89305da08d9ae989c47 Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Wed, 11 Dec 2019 10:10:41 +0100 Subject: [PATCH 12/18] Apply review suggestions --- src/librustc_lint/unused.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index ee644acd09974..e6f39cca6dc3a 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -17,7 +17,7 @@ use syntax::print::pprust; use syntax::symbol::{kw, sym}; use syntax::symbol::Symbol; use syntax::util::parser; -use syntax_pos::{MultiSpan, Span, BytePos}; +use syntax_pos::{Span, BytePos}; use log::debug; @@ -356,8 +356,7 @@ impl UnusedParens { ast::ExprKind::Paren(ref inner) => { if !Self::is_expr_parens_necessary(inner, followed_by_block) && value.attrs.is_empty() && - !MultiSpan::from(value.span).primary_span() - .map_or(false, |span| span.from_expansion()) + !value.span.from_expansion() { let expr_text = if let Ok(snippet) = cx.sess().source_map() .span_to_snippet(value.span) { From fafa4897985f932490960e90ddd2ff39134e967e Mon Sep 17 00:00:00 2001 From: Nicholas Matsakis Date: Wed, 11 Dec 2019 10:32:11 -0500 Subject: [PATCH 13/18] clarify that `Box` should only be used when defined *in Rust* --- src/liballoc/boxed.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/liballoc/boxed.rs b/src/liballoc/boxed.rs index d50b4b8dc4a7b..c25495bec41ee 100644 --- a/src/liballoc/boxed.rs +++ b/src/liballoc/boxed.rs @@ -63,13 +63,14 @@ //! T` obtained from `Box::::into_raw` may be deallocated using the //! [`Global`] allocator with `Layout::for_value(&*value)`. //! -//! So long as `T: Sized`, a `Box` is guaranteed to be represented as a -//! single pointer and is also ABI-compatible with C pointers (i.e. the C type -//! `T*`). This means that you can have Rust code which passes ownership of a -//! `Box` to C code by using `Box` as the type on the Rust side, and -//! `T*` as the corresponding type on the C side. As an example, consider this -//! C header which declares functions that create and destroy some kind of -//! `Foo` value: +//! So long as `T: Sized`, a `Box` is guaranteed to be represented +//! as a single pointer and is also ABI-compatible with C pointers +//! (i.e. the C type `T*`). This means that if you have extern "C" +//! Rust functions that will be called from C, you can define those +//! Rust functions using `Box` types, and use `T*` as corresponding +//! type on the C side. As an example, consider this C header which +//! declares functions that create and destroy some kind of `Foo` +//! value: //! //! ```c //! /* C header */ @@ -108,6 +109,14 @@ //! is to only use `Box` for pointers that originated from the global //! allocator. //! +//! **Important.** At least at present, you should avoid using +//! `Box` types for functions that are defined in C but invoked +//! from Rust. In those cases, you should directly mirror the C types +//! as closely as possible. Using types like `Box` where the C +//! definition is just using `T*` can lead to undefined behavior, as +//! described in [rust-lang/unsafe-code-guidelines#198][ucg#198]. +//! +//! [ucg#198]: https://github.com/rust-lang/unsafe-code-guidelines/issues/198 //! [dereferencing]: ../../std/ops/trait.Deref.html //! [`Box`]: struct.Box.html //! [`Global`]: ../alloc/struct.Global.html From e4852da576bda4a5d26341961e0be7d6626ff5c0 Mon Sep 17 00:00:00 2001 From: chansuke Date: Thu, 12 Dec 2019 01:00:37 +0900 Subject: [PATCH 14/18] Remove irelevant comment on `register_dtor` --- src/libstd/sys/unix/fast_thread_local.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libstd/sys/unix/fast_thread_local.rs b/src/libstd/sys/unix/fast_thread_local.rs index dfb9307daea9f..0861432f8a923 100644 --- a/src/libstd/sys/unix/fast_thread_local.rs +++ b/src/libstd/sys/unix/fast_thread_local.rs @@ -8,8 +8,6 @@ // Note, however, that we run on lots older linuxes, as well as cross // compiling from a newer linux to an older linux, so we also have a // fallback implementation to use as well. -// -// Due to rust-lang/rust#18804, make sure this is not generic! #[cfg(any( target_os = "linux", target_os = "fuchsia", From 914c9aa78d7d43d268690e2cf1ae634795cd29aa Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 11 Dec 2019 21:58:00 +0300 Subject: [PATCH 15/18] resolve: Always resolve visibilities on impl items --- src/librustc_resolve/build_reduced_graph.rs | 24 +++++++++--------- .../ui/resolve/impl-items-vis-unresolved.rs | 25 +++++++++++++++++++ .../resolve/impl-items-vis-unresolved.stderr | 9 +++++++ 3 files changed, 46 insertions(+), 12 deletions(-) create mode 100644 src/test/ui/resolve/impl-items-vis-unresolved.rs create mode 100644 src/test/ui/resolve/impl-items-vis-unresolved.stderr diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 566ba12907437..e2578d67e730c 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -647,8 +647,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { self.r.define(parent, ident, TypeNS, imported_binding); } - ItemKind::GlobalAsm(..) => {} - ItemKind::Mod(..) if ident.name == kw::Invalid => {} // Crate root ItemKind::Mod(..) => { @@ -667,9 +665,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { self.parent_scope.module = module; } - // Handled in `rustc_metadata::{native_libs,link_args}` - ItemKind::ForeignMod(..) => {} - // These items live in the value namespace. ItemKind::Static(..) => { let res = Res::Def(DefKind::Static, self.r.definitions.local_def_id(item.id)); @@ -765,12 +760,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { self.insert_field_names_local(def_id, vdata); } - ItemKind::Impl(.., ref impl_items) => { - for impl_item in impl_items { - self.resolve_visibility(&impl_item.vis); - } - } - ItemKind::Trait(..) => { let def_id = self.r.definitions.local_def_id(item.id); @@ -785,6 +774,9 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { self.parent_scope.module = module; } + // These items do not add names to modules. + ItemKind::Impl(..) | ItemKind::ForeignMod(..) | ItemKind::GlobalAsm(..) => {} + ItemKind::MacroDef(..) | ItemKind::Mac(_) => unreachable!(), } } @@ -1118,7 +1110,6 @@ macro_rules! method { } impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> { - method!(visit_impl_item: ast::ImplItem, ast::ImplItemKind::Macro, walk_impl_item); method!(visit_expr: ast::Expr, ast::ExprKind::Mac, walk_expr); method!(visit_pat: ast::Pat, ast::PatKind::Mac, walk_pat); method!(visit_ty: ast::Ty, ast::TyKind::Mac, walk_ty); @@ -1202,6 +1193,15 @@ impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> { visit::walk_trait_item(self, item); } + fn visit_impl_item(&mut self, item: &'b ast::ImplItem) { + if let ast::ImplItemKind::Macro(..) = item.kind { + self.visit_invoc(item.id); + } else { + self.resolve_visibility(&item.vis); + visit::walk_impl_item(self, item); + } + } + fn visit_token(&mut self, t: Token) { if let token::Interpolated(nt) = t.kind { if let token::NtExpr(ref expr) = *nt { diff --git a/src/test/ui/resolve/impl-items-vis-unresolved.rs b/src/test/ui/resolve/impl-items-vis-unresolved.rs new file mode 100644 index 0000000000000..9b4fe498239b6 --- /dev/null +++ b/src/test/ui/resolve/impl-items-vis-unresolved.rs @@ -0,0 +1,25 @@ +// Visibilities on impl items expanded from macros are resolved (issue #64705). + +macro_rules! perftools_inline { + ($($item:tt)*) => ( + $($item)* + ); +} + +mod state { + pub struct RawFloatState; + impl RawFloatState { + perftools_inline! { + pub(super) fn new() {} // OK + } + } +} + +pub struct RawFloatState; +impl RawFloatState { + perftools_inline! { + pub(super) fn new() {} //~ ERROR failed to resolve: there are too many initial `super`s + } +} + +fn main() {} diff --git a/src/test/ui/resolve/impl-items-vis-unresolved.stderr b/src/test/ui/resolve/impl-items-vis-unresolved.stderr new file mode 100644 index 0000000000000..8e285e5312400 --- /dev/null +++ b/src/test/ui/resolve/impl-items-vis-unresolved.stderr @@ -0,0 +1,9 @@ +error[E0433]: failed to resolve: there are too many initial `super`s. + --> $DIR/impl-items-vis-unresolved.rs:21:13 + | +LL | pub(super) fn new() {} + | ^^^^^ there are too many initial `super`s. + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0433`. From 7f87dd1bc008d49cc27bee8d5a866e3c2de463b3 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 11 Dec 2019 20:49:46 +0100 Subject: [PATCH 16/18] Some small readability improvements --- src/libcore/char/methods.rs | 15 +++++---------- src/libcore/str/pattern.rs | 7 +------ 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/src/libcore/char/methods.rs b/src/libcore/char/methods.rs index 1ec614edbe2eb..5c63eebf595f9 100644 --- a/src/libcore/char/methods.rs +++ b/src/libcore/char/methods.rs @@ -553,8 +553,7 @@ impl char { pub fn is_alphabetic(self) -> bool { match self { 'a'..='z' | 'A'..='Z' => true, - c if c > '\x7f' => derived_property::Alphabetic(c), - _ => false, + c => c > '\x7f' && derived_property::Alphabetic(c), } } @@ -585,8 +584,7 @@ impl char { pub fn is_lowercase(self) -> bool { match self { 'a'..='z' => true, - c if c > '\x7f' => derived_property::Lowercase(c), - _ => false, + c => c > '\x7f' && derived_property::Lowercase(c), } } @@ -617,8 +615,7 @@ impl char { pub fn is_uppercase(self) -> bool { match self { 'A'..='Z' => true, - c if c > '\x7f' => derived_property::Uppercase(c), - _ => false, + c => c > '\x7f' && derived_property::Uppercase(c), } } @@ -646,8 +643,7 @@ impl char { pub fn is_whitespace(self) -> bool { match self { ' ' | '\x09'..='\x0d' => true, - c if c > '\x7f' => property::White_Space(c), - _ => false, + c => c > '\x7f' && property::White_Space(c), } } @@ -744,8 +740,7 @@ impl char { pub fn is_numeric(self) -> bool { match self { '0'..='9' => true, - c if c > '\x7f' => general_category::N(c), - _ => false, + c => c > '\x7f' && general_category::N(c), } } diff --git a/src/libcore/str/pattern.rs b/src/libcore/str/pattern.rs index a494274118a74..1037da14b5f62 100644 --- a/src/libcore/str/pattern.rs +++ b/src/libcore/str/pattern.rs @@ -296,12 +296,7 @@ unsafe impl<'a> Searcher<'a> for CharSearcher<'a> { fn next_match(&mut self) -> Option<(usize, usize)> { loop { // get the haystack after the last character found - let bytes = if let Some(slice) = self.haystack.as_bytes() - .get(self.finger..self.finger_back) { - slice - } else { - return None; - }; + let bytes = self.haystack.as_bytes().get(self.finger..self.finger_back)?; // the last byte of the utf8 encoded needle let last_byte = unsafe { *self.utf8_encoded.get_unchecked(self.utf8_size - 1) }; if let Some(index) = memchr::memchr(last_byte, bytes) { From 24227977852b22eb0ab3a228b7135611ae49bb8d Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 11 Dec 2019 21:01:33 +0100 Subject: [PATCH 17/18] Small Cow improvements --- src/liballoc/borrow.rs | 18 +++++------------- src/liballoc/tests/cow_str.rs | 3 +++ 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/liballoc/borrow.rs b/src/liballoc/borrow.rs index d2bdda83fa998..fc96045196846 100644 --- a/src/liballoc/borrow.rs +++ b/src/liballoc/borrow.rs @@ -195,14 +195,10 @@ impl Clone for Cow<'_, B> { } fn clone_from(&mut self, source: &Self) { - if let Owned(ref mut dest) = *self { - if let Owned(ref o) = *source { - o.borrow().clone_into(dest); - return; - } + match (self, source) { + (&mut Owned(ref mut dest), &Owned(ref o)) => o.borrow().clone_into(dest), + (t, s) => *t = s.clone(), } - - *self = source.clone(); } } @@ -449,9 +445,7 @@ impl<'a> AddAssign<&'a str> for Cow<'a, str> { fn add_assign(&mut self, rhs: &'a str) { if self.is_empty() { *self = Cow::Borrowed(rhs) - } else if rhs.is_empty() { - return; - } else { + } else if !rhs.is_empty() { if let Cow::Borrowed(lhs) = *self { let mut s = String::with_capacity(lhs.len() + rhs.len()); s.push_str(lhs); @@ -467,9 +461,7 @@ impl<'a> AddAssign> for Cow<'a, str> { fn add_assign(&mut self, rhs: Cow<'a, str>) { if self.is_empty() { *self = rhs - } else if rhs.is_empty() { - return; - } else { + } else if !rhs.is_empty() { if let Cow::Borrowed(lhs) = *self { let mut s = String::with_capacity(lhs.len() + rhs.len()); s.push_str(lhs); diff --git a/src/liballoc/tests/cow_str.rs b/src/liballoc/tests/cow_str.rs index 6f357eda9b83b..62a5c245a5429 100644 --- a/src/liballoc/tests/cow_str.rs +++ b/src/liballoc/tests/cow_str.rs @@ -138,4 +138,7 @@ fn check_cow_clone_from() { let c2: Cow<'_, str> = Cow::Owned(s); c1.clone_from(&c2); assert!(c1.into_owned().capacity() >= 25); + let mut c3: Cow<'_, str> = Cow::Borrowed("bye"); + c3.clone_from(&c2); + assert_eq!(c2, c3); } From ab3afc0f0426ea7e2801043a874dd96b2831a1eb Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 11 Dec 2019 21:05:28 +0100 Subject: [PATCH 18/18] Make TinyList::remove iterate instead of recurse --- src/librustc_data_structures/tiny_list.rs | 40 +++++++++-------------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/src/librustc_data_structures/tiny_list.rs b/src/librustc_data_structures/tiny_list.rs index 371f0f6fa0b44..78cbc1240b182 100644 --- a/src/librustc_data_structures/tiny_list.rs +++ b/src/librustc_data_structures/tiny_list.rs @@ -16,41 +16,29 @@ mod tests; #[derive(Clone)] pub struct TinyList { - head: Option> + head: Option>, } impl TinyList { #[inline] pub fn new() -> TinyList { - TinyList { - head: None - } + TinyList { head: None } } #[inline] pub fn new_single(data: T) -> TinyList { - TinyList { - head: Some(Element { - data, - next: None, - }) - } + TinyList { head: Some(Element { data, next: None }) } } #[inline] pub fn insert(&mut self, data: T) { - self.head = Some(Element { - data, - next: self.head.take().map(Box::new) - }); + self.head = Some(Element { data, next: self.head.take().map(Box::new) }); } #[inline] pub fn remove(&mut self, data: &T) -> bool { self.head = match self.head { - Some(ref mut head) if head.data == *data => { - head.next.take().map(|x| *x) - } + Some(ref mut head) if head.data == *data => head.next.take().map(|x| *x), Some(ref mut head) => return head.remove_next(data), None => return false, }; @@ -88,12 +76,16 @@ struct Element { impl Element { fn remove_next(&mut self, data: &T) -> bool { - let new_next = match self.next { - Some(ref mut next) if next.data == *data => next.next.take(), - Some(ref mut next) => return next.remove_next(data), - None => return false, - }; - self.next = new_next; - true + let mut n = self; + loop { + match n.next { + Some(ref mut next) if next.data == *data => { + n.next = next.next.take(); + return true; + } + Some(ref mut next) => n = next, + None => return false, + } + } } }