Skip to content

Fix wrong substitution code #4106

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 1 commit into from
Apr 23, 2020
Merged

Fix wrong substitution code #4106

merged 1 commit into from
Apr 23, 2020

Conversation

flodiebold
Copy link
Member

We need to shift in when we're substituting inside a binder.

This should fix #4053 (it doesn't fix the occasional overflow that also occurs on the Diesel codebase though).

@@ -863,7 +863,7 @@ pub trait TypeWalk {
&mut |ty, binders| {
if let &mut Ty::Bound(bound) = ty {
if bound.debruijn >= binders {
*ty = substs.0[bound.index].clone();
*ty = substs.0[bound.index].clone().shift_bound_vars(binders);
Copy link
Member

@matklad matklad Apr 23, 2020

Choose a reason for hiding this comment

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

Is there perhaps some way we could make similar wrong code crash on all examples, and not only just on the bad ones?

Copy link
Member

Choose a reason for hiding this comment

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

Like, if this happens to work most of the time, b/c binders is 0, can we remember a number which tells the nesting/how many times we should shift, and than assert everywhere that nesting levels match?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how -- if I remember to assert, I can just do it correctly in that place. We could validate types to check that the binders are correct, but that will also only crash if we try to bind to indexes that don't exist.


#[test]
fn issue_4053_diesel_where_clauses() {
std::env::set_var("CHALK_DEBUG", "1");
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed? It won't be scoped to a single test.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, yeah

We need to shift in when we're substituting inside a binder.

This should fix rust-lang#4053 (it doesn't fix the occasional overflow that also occurs
on the Diesel codebase though).
@matklad
Copy link
Member

matklad commented Apr 23, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 23, 2020

@bors bors bot merged commit 5eb51c1 into rust-lang:master Apr 23, 2020
@flodiebold flodiebold deleted the diesel-crash branch April 26, 2020 10:07
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.

New weekly build crashs reproducily
2 participants