-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
[WIP] Make Debruijn indices zero-based #49840
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
This fails on my local machine when I run the tests with
Any suggestions on how to tackle this would be appreciated :). |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@@ -1156,7 +1156,6 @@ impl<'a, 'tcx, 'gcx> PolyExistentialProjection<'tcx> { | |||
|
|||
impl DebruijnIndex { | |||
pub fn new(depth: u32) -> DebruijnIndex { | |||
assert!(depth > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does DebruijnIndex::from_depth
need to be changed to remove the shift by 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll give it a go locally.
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
df3c32c
to
8cdfe8c
Compare
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what's causing the failures, looks like some binder not catching. If you want me to help debug, I can, but I don't know what to do beyond dumping RUST_LOGs and trying to see where things are going astray.
@@ -137,14 +137,14 @@ impl Region { | |||
match self { | |||
Region::LateBound(debruijn, id, origin) => Region::LateBound( | |||
ty::DebruijnIndex { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre-existing, but from_depth
sure needs a comment =) let me try to remember what it's purpose is...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it seems like it is basically a variant of shifted
, but it goes in the opposite direction. I think it should probably be renamed; chalk calls this shift_down
, I think. In any case, a comment would be good:
Removes depth
level of binders from this region. region
must not be bound within those binders or else this will panic.
To that end, up/down are not especially intuitive names. Maybe something like shift_out
or pull_out
(from binders)? We would then rename shifted
to shift_in
or push_down
(through binders)? Maybe remove_binders
and in_binders
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look and rename these :D Thanks for the hints.
Thanks for looking into this @nikomatsakis :). I'll keep working on this, and I'll let you know if I need help. |
@muscar ok -- I didn't do any digging yet, but let me know. |
@nikomatsakis I'll dig further during the weekend. Keep you posted. |
@nikomatsakis Hi, I'm a bit stuck on this one. It panics on this line: https://github.com/muscar/rust/blob/master/src/librustc/infer/region_constraints/mod.rs#L694 with
Since the left hand side is a late bound region (with a debruijn index of 0), I assume it wasn't resolved to another type of region in one of the revious passes. Since this happens after the index change, I guess some other piece of code assumes debruijn indexes are 1-based so it doesn't catch the late bound region. I don't know enough about the code to say if this is the case or not, it's just a guess. I'd be happy to work on this if you can point me in the right direction (I'm afraid I don't have a lot of time to familiarise myself with the code right now :(). |
Printing some debug information shows this:
(Don't know what that means. Just want to help :) ) Edit: The information is generated here. |
Thanks @fabric-and-ink. I ran the compiler with extra logs, and got the same errors. I'll try to add extra logging, and see if I can make sense of this. |
@muscar indeed, I'm not sure which code is wrong, but clearly something is. I'll try and do a local build and see if I can spot it. |
OK -- I started digging in and I think i've found the problem. A number of bits of the code prefer to use the term "depth", which basically means "number of enclosing binders (inclusive)". This is handy because Some of the code then does things like I think actually we should probably convert the code that currently tracks "present depth" so that it is tracking a Anyway i'm trying to decide how much of this to hack and how much to leave to you @muscar =) this kind of change is sort of hard to do piecemeal, you lose track of what you have changed and what the other has done. |
@muscar so -- I'm concerned that if I push much more on this, I will wind up kind of "taking over" this PR. I was wondering if you'd be up for scheduling a slot to "pair program" on this remotely for a bit? |
I pushed my WIP commit here: https://github.com/nikomatsakis/rust/tree/zero-based-debruijn-indices As you'll see if you browse that commit, I also started doing some renamings and refactorings (e.g., renaming I sort of remember that I had decided to take a slightly different approach than that commit, though, in particular eliminating the idea of "depth". |
Ping from triage @muscar ! Will you have time to work on this soon or to schedule time with @nikomatsakis ? |
@nikomatsakis First of all sorry for the lack of response. I got tied up at work, and looking for a new flat. Thanks for all the help with this--even if it didn't go anywhere. I can see why people say the Rust community is so welcoming. I hope this gets fixed. I'll ping you if I get more time. |
This changes the
DebruijnIndex
type to be zero-based (resolves #49813).