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.

Sign up for GitHub

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

tyencode: Make sure that projection bounds are handled in stable order. #34805

Merged
merged 1 commit into from
Jul 14, 2016
Merged

tyencode: Make sure that projection bounds are handled in stable order. #34805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/librustc_metadata/tyencode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,14 @@ pub fn enc_existential_bounds<'a,'tcx>(w: &mut Cursor<Vec<u8>>,

enc_region(w, cx, bs.region_bound);

for tp in &bs.projection_bounds {
// Encode projection_bounds in a stable order
let mut projection_bounds: Vec<_> = bs.projection_bounds
.iter()
.map(|b| (b.item_name().as_str(), b))
.collect();
projection_bounds.sort_by_key(|&(ref name, _)| name.clone());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newbie question (and total nit): why couldn't this basically be the following (or something close to it)?

let mut projection_bounds = bs.projection_bounds
                              .clone()
                              .sort_by_key(|&b| b.item_name().as_str());

Because then it seems we'd be able to keep the next line just for tp in &projection_bounds { ... }, avoiding the extra call to map.

And even if we didn't do this the next line still seems like it could stay for tp in &projection_bounds with s/tp.0/tp.1.0 in the loop body.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since sort_by_key() returns nothing, projection_bounds with be the unit value (). It would have to look at least like:

let mut projection_bounds = bs.projection_bounds.clone();
projection_bounds.sort_by_key(|&b| b.item_name().as_str());

And why didn't I do it like the above? In order to avoid that b.item_name().as_str() gets called more than once per item. Although, I somehow mistakenly assumed that b.item_name().as_str() would need to hash the string each time when looking it up in the interner, which it doesn't, so this "optimization" is even more premature than I thought :)

So, your version, when fixed as indicated, would actually be nicer than mine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, because sort_by_key would make basically O(n log n) calls to its closure, i.e. to b.item_name().as_str()? Makes much more sense now (notwithstanding the question of whether it's a premature optimization :). And yeah I overlooked the fact that sort_by_key returns unit, so your fix makes sense. Thanks for the explanation! Up to you what changes you want to make, if any.


for tp in projection_bounds.iter().map(|&(_, tp)| tp) {
write!(w, "P");
enc_projection_predicate(w, cx, &tp.0);
}
Expand Down
30 changes: 30 additions & 0 deletions src/test/run-pass/auxiliary/issue34796aux.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![crate_type = "lib"]
pub trait Future {
type Item;
type Error;
}

impl Future for u32 {
type Item = ();
type Error = Box<()>;
}

fn foo() -> Box<Future<Item=(), Error=Box<()>>> {
Box::new(0u32)
}

pub fn bar<F, A, B>(_s: F)
where F: Fn(A) -> B,
{
foo();
}
36 changes: 36 additions & 0 deletions src/test/run-pass/issue34796.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// This test case exposes conditions where the encoding of a trait object type
// with projection predicates would differ between this crate and the upstream
// crate, because the predicates were encoded in different order within each
// crate. This led to different symbol hashes of functions using these type,
// which in turn led to linker errors because the two crates would not agree on
// the symbol name.
// The fix was to make the order in which predicates get encoded stable.

// aux-build:issue34796aux.rs
extern crate issue34796aux;

fn mk<T>() -> T { loop {} }

struct Data<T, E> {
data: T,
error: E,
}

fn main() {
issue34796aux::bar(|()| {
Data::<(), std::io::Error> {
data: mk(),
error: mk(),
}
})
}