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

typeck: use a TypeVisitor in ctp #35143

Merged
merged 1 commit into from
Aug 1, 2016
Merged

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jul 31, 2016

Use a TypeVisitor in ctp instead of ty::walk

This fixes a few cases where a region could be projected out of a trait while not being constrained by the type parameters, violating rust-lang/rfcs#447 and breaking soundness. As such, this is a [breaking-change].

Fixes #35139

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Jul 31, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jul 31, 2016

📌 Commit 0a128f3 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Jul 31, 2016

⌛ Testing commit 0a128f3 with merge 7333c4a...

bors added a commit that referenced this pull request Jul 31, 2016
typeck: use a TypeVisitor in ctp

Use a TypeVisitor in ctp instead of `ty::walk`

This fixes a few cases where a region could be projected out of a trait while not being constrained by the type parameters, violating rust-lang/rfcs#447 and breaking soundness. As such, this is a [breaking-change].

Fixes #35139

r? @eddyb
@eddyb
Copy link
Member

eddyb commented Jul 31, 2016

Crater report shows 1 root regression: owning_ref.

cc @rust-lang/lang

@bors bors merged commit 0a128f3 into rust-lang:master Aug 1, 2016
diwic added a commit to diwic/dbus-rs that referenced this pull request Aug 2, 2016
This is due to a soundness fix in rustc:
rust-lang/rust#35143

Signed-off-by: David Henningsson <diwic@ubuntu.com>
@nikomatsakis
Copy link
Contributor

Given the minimal impact, this doesn't require a warning period, but we still ought to do our best to minimize impact before landing and help users. e.g. give an explanatory note on the error indicating that there was a recent bug-fix (it'd be ok, I think, for that note to appear even if the error would have happened before), and/or submit a PR to fix the problem. Still, given that this landed, I'm not inclined to roll it back. Just saying.

cc @Kimundi -- this change fixes a hole that the "owning-ref" crate was relying on.

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.

Unsound: Borrowck missing that closure needs to be "move"
4 participants