Beta ICE: ItemLocalIds not assigned densely

[matklad]

Hi! I am seeing an ICE in rust-analyzer, when updating rustc from rustc 1.31.0-beta.14 to rustc 1.31.0-beta.15 (so, seems pretty bad?).

Error message:

   Compiling ra_syntax v0.1.0 (/home/matklad/projects/rust-analyzer/crates/ra_syntax)               
thread 'main' panicked at 'librustc/hir/map/hir_id_validator.rs:31:                                 
ItemLocalIds not assigned densely in ::grammar[0]::expressions[0]::{{?}}[1]. Max ItemLocalId = 4, missing IDs = ["[local_id: 1, node:unknown node (id=28193)]", "[local_id: 2, node:path segment super (id=28192)]"]; seens IDs = ["(NodeId(28191) use grammar::expressions::{{?}} (id=28191))", "(NodeId(28195) path segment atom (id=28195))", "(NodeId(28194) path segment self (id=28194))"]
HirIdValidator: The recorded owner of path segment super (id=28192) is ::grammar[0]::expressions[0]::{{?}}[1] instead of ::grammar[0]::expressions[0]::{{?}}[2]
HirIdValidator: Same HirId ::grammar[0]::expressions[0]::{{?}}[2]/2 assigned for nodes path segment super (id=28192) and path segment atom (id=92512)
HirIdValidator: The recorded owner of path segment super (id=28192) is ::grammar[0]::expressions[0]::{{?}}[1] instead of ::grammar[0]::expressions[0]::{{?}}[3]
HirIdValidator: Same HirId ::grammar[0]::expressions[0]::{{?}}[3]/2 assigned for nodes path segment super (id=28192) and path segment atom (id=92515)', librustc/util/bug.rs:47:26
note: Run with `RUST_BACKTRACE=1` for a backtrace.                                                  
                                                                                                    
error: internal compiler error: unexpected panic                                                    
                                                                                                    
note: the compiler unexpectedly panicked. this is a bug.                                            
                                                                                                    
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
                                                                                                    
note: rustc 1.31.0-beta.15 (4b3a1d911 2018-11-20) running on x86_64-unknown-linux-gnu               
                                                                                                    
note: compiler flags: -C debuginfo=2 -C incremental --crate-type lib                                
                                                                                                    
note: some of the compiler flags provided by cargo are hidden                                       
                                                                                                    
error: Could not compile `ra_syntax`.                                                               
warning: build failed, waiting for other jobs to finish...
thread 'main' panicked at 'librustc/hir/map/hir_id_validator.rs:31:                                 
ItemLocalIds not assigned densely in ::grammar[0]::expressions[0]::{{?}}[1]. Max ItemLocalId = 4, missing IDs = ["[local_id: 1, node:unknown node (id=28200)]", "[local_id: 2, node:path segment super (id=28199)]"]; seens IDs = ["(NodeId(28198) use grammar::expressions::{{?}} (id=28198))", "(NodeId(28202) path segment atom (id=28202))", "(NodeId(28201) path segment self (id=28201))"]
HirIdValidator: The recorded owner of path segment super (id=28199) is ::grammar[0]::expressions[0]::{{?}}[1] instead of ::grammar[0]::expressions[0]::{{?}}[2]
HirIdValidator: Same HirId ::grammar[0]::expressions[0]::{{?}}[2]/2 assigned for nodes path segment super (id=28199) and path segment atom (id=104341)
HirIdValidator: The recorded owner of path segment super (id=28199) is ::grammar[0]::expressions[0]::{{?}}[1] instead of ::grammar[0]::expressions[0]::{{?}}[3]
HirIdValidator: Same HirId ::grammar[0]::expressions[0]::{{?}}[3]/2 assigned for nodes path segment super (id=28199) and path segment atom (id=104344)', librustc/util/bug.rs:47:26
note: Run with `RUST_BACKTRACE=1` for a backtrace.                                                  
                                                                                                    
error: internal compiler error: unexpected panic                                                    
                                                                                                    
note: the compiler unexpectedly panicked. this is a bug.                                            
                                                                                                    
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
                                                                                                    
note: rustc 1.31.0-beta.15 (4b3a1d911 2018-11-20) running on x86_64-unknown-linux-gnu               
                                                                                                    
note: compiler flags: -C debuginfo=2 -C incremental                                                 
                                                                                                    
note: some of the compiler flags provided by cargo are hidden  

Steps to reproduce:

$ git clone git@github.com:rust-analyzer/rust-analyzer.git
$ cd rust-analyzer
$ git reset --hard 049f8df93cca05af395ce873738dc85d5a25f3fc
$ cargo test

[oli-obk]

probably a backport of whatever is causing #55475

[petrochenkov]

ping @nrc
This is a regression from #54145 that's now in beta.

[matklad]

It's funny how Keep resolved defs in path prefixes is exactly what I was doing in the rust-analyzer's PR that triggered the issue 😆

[nikomatsakis]

Building the beta locally, I get this error, which is a slightly different message

thread 'main' panicked at 'librustc/hir/map/collector.rs:219: inconsistent DepNode for `PathSegment(PathSegment { ident: super#0, id: Some(NodeId(28192)), def: Some(Err), args: None, infer_types: false })`: current_dep_node_owner=::grammar[0]::expressions[0]::{{?}}[2] (DefIndex(0:1648)), hir_id.owner=::grammar[0]::expressions[0]::{{?}}[1] (DefIndex(0:1647))', librustc/util/bug.rs:47:26

[nikomatsakis]

(Though it seems to be related to the same root problem)

[nikomatsakis]

(Ah, this check is only done when debug_assertions are enabled.)

[nikomatsakis]

Minimized reproduction:

mod bar {
    pub(super) use self::baz::{x, y};
//      ^^^^^ assert has to do with this path
//
// the other thing you need is the `{x, y}` part, which I think causes this to expand into
// two copies of the same `pub use` internally

    mod baz {
        pub fn x() { }
        pub fn y() { }
    }
}

fn main() { }

[nikomatsakis]

OK I see the bug I think

[nikomatsakis]

My theory wasn't quite right. Clearly it has something to do with sharing ids. You can see that in some paths, we clone the ids from the visibility kind:

rust/src/librustc/hir/lowering.rs

Lines 3023 to 3028 in 4b3a1d9

	let mut path = path.clone();
	for seg in path.segments.iter_mut() {
	if seg.id.is_some() {
	seg.id = Some(this.next_id().node_id);
	}
	}

But in others, we do not:

rust/src/librustc/hir/lowering.rs

Lines 3111 to 3118 in 4b3a1d9

	hir::VisibilityKind::Restricted { ref path, id: _, hir_id: _ } => {
	let id = this.next_id();
	hir::VisibilityKind::Restricted {
	path: path.clone(),
	id: id.node_id,
	hir_id: id.hir_id,
	}
	}

However, just adding clone operations to both paths didn't quite suffice.

[nikomatsakis]

Oh, actually, it sort of did. It solves my immediate ICE, but now I am getting another ICE, and this one matches the original message ("not dense").

[nikomatsakis]

I think the problem is that we are cloning the restricted paths that are getting duplicated and then never using the original, un-cloned paths.

[nikomatsakis]

OK, so, I have a fix but there is one part I do not understand. In particular, I have to remove this line:

rust/src/librustc/hir/lowering.rs

Line 3142 in 4b3a1d9

*vis = respan(prefix.span.shrink_to_lo(), hir::VisibilityKind::Inherited);

which seems to have been there since @pietroalbini originally added this code. But from what I can tell it is this line that makes the ICE, because the "original" visibility never winds up in the tree anywhere (we always clone it for all the derived HIR paths).

[nikomatsakis]

I'll open my PR and cc the relevant folks.

[nikomatsakis]

@pietroalbini and I have been conversing on Zulip, log here.

[nikomatsakis]

Pending fix in #56143