Changes to catch_fatal_errors in rustc driver

[jolson88]

A recent PR changed the function name from report_ices_to_stderr_if_any to catch_fatal_errors. This PR changes to using the new function name.

changelog: none

[phansch]

@bors r+

[bors]

📌 Commit bf4a467 has been approved by phansch

[bors]

⌛ Testing commit bf4a467 with merge 388f53d...

[bors]

💔 Test failed - checks-travis

[phansch]

It looks like this or some other change causes an ICE in chalk, hyper and serde:

error: internal compiler error: src/librustc/ty/context.rs:211: node type <T>::Assoc (hir_id=HirId { owner: DefIndex(5117), local_id: 1 })
with HirId::owner DefId(0:5117 ~ test_gen[7a33]::test_gen[1]::AssocDerive[0]) 
cannot be placed in TypeckTables with local_id_root DefId(0:77 ~ test_gen[7a33]::test_gen[1])

I don't think it was caused by this change, though. Probably by another Rust PR.

[phansch]

Do we maybe need to call install_ice_hook like miri? https://github.com/rust-lang/miri/pull/954/files

[mati865]

@phansch it's surely different Rust PR, I guess it's failing this line in Clippy: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/types.rs#L243

Do we maybe need to call install_ice_hook like miri?

If you look at the ICE, it's missing this part:

Click me

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.39.0-nightly (fba38ac27 2019-08-31) 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 `rexlog`.

This is what install_ice_hook would add, it would be nice to take advantage of rust-lang/rust#60584 and make ICEs point to Clippy repo for bug reporting but it can be done in another PR.

[flip1995]

I wasn't able to reproduce this locally 🤔 Let's just retry, since the queue is empty anyway

@bors try

[bors]

⌛ Trying commit bf4a467 with merge a744518...

[bors]

💔 Test failed - checks-travis

[flip1995]

This is why Clippy ICEs: rust-lang/rust@b456c82

[jolson88]

Did you figure out a way to repro locally @flip1995?

As I'm new to Clippy and Rust internals, any ideas on what change needs to happen to fix this? I'm more than willing to continue to shepherd this PR forward, just may need some tips as I'm still learning about MIR and late passes.

Is it as simple as adding install_ice_hook?

[jolson88]

I've added install_ice_hook for now to be inline with miri. Should we go ahead and open a new issue for customizing the ICE message to point to Clippy's repo as @mati865 mentioned?

[flip1995]

Did you figure out a way to repro locally @flip1995?

I currently only have a few minutes a day for Clippy, due to my master thesis, so not yet. I will try to find a reproducer today. See this comment, for my findings: rust-lang/rust#64250 (comment). Like @mati865 pointed out, the line at fault is this one:

rust-clippy/clippy_lints/src/types.rs

Line 243 in 535bc1d

let res = cx.tables.qpath_res(qpath, hir_id);

open a new issue for customizing the ICE message

Yes, can you do this?

[flip1995]

The problem seems to be associated types.

[flip1995]

ping @Xanewok. Is this a rustc bug, related to rust-lang/rust#64250 or are we doing something wrong on the Clippy side?

Error message

error: internal compiler error: src/librustc/ty/context.rs:211: node type <T>::Bar (hir_id=HirId { owner: DefIndex(15), local_id: 1 }) with HirId::owner DefId(0:15 ~ ice_4545[317d]::repro[0]::Baz[0]) cannot be placed in TypeckTables with local_id_root DefId(0:12 ~ ice_4545[317d]::repro[0])

[Xanewok]

The OP fix is unrelated to the ICE (afaict) and corresponds to rust-lang/rls@39d617b on the RLS side.

Having said that, the ICE mentioned by @flip1995 is caused by rust-lang/rust#64250, which promoted a debug assertion to a regular one, probably uncovering some bugs.

To give a quick background information (to the best of my knowledge), the definitions are identified by a def path, where each segment and the definition itself are identifiable by a def ID.

The HIR ID contains an owner (corresponds to a def id) and a local id. Since we use local ids to as the key to the typeck tables (owned by a def), this assertion fires whenever we try to access typeck tables with a HIR ID that don't share the same owner (since it can lead to subtle bugs).

In this specific case it means that the cx.tables and hir_id are mismatched:

• typeck tables are rooted at DefId(0:12 ~ ice_4545[317d]::repro[0])
• while the <T>::Bar with HirId { owner: DefIndex(15), local_id: 1 } is owned by DefId(0:15 ~ ice_4545[317d]::repro[0]::Baz[0])

To fix the issue, technically you need to first query the typeck_tables_of(DefId(0:15)) and then call qpath_res on the resulting tables for the IDs to match.

[flip1995]

Thanks for the insights! We should be able to fix this, with this information.

[flip1995]

So if we're using typeck_tables_of(DefId(0:15)) for the tables, we hit https://github.com/rust-lang/rust/blob/5670d048c0f88af9976b5505c7853b23dd06770d/src/librustc_typeck/check/mod.rs#L829-L832:

let (body_id, body_ty, fn_header, fn_decl) = primary_body_of(tcx, id)
    .unwrap_or_else(|| {
        span_bug!(span, "can't type-check body of {:?}", def_id);
    });

This can be prevented by first checking with has_typeck_tables(DefId), but then we get multiple FN (i.e. lints for Option<Option<_>>, Vec<Box<_>> and LinkedList<_>).

[jolson88]

open a new issue for customizing the ICE message

Yes, can you do this?

Yup, I'll get a new issue opened today.

[jolson88]

This can be prevented by first checking with has_typeck_tables(DefId), but then we get multiple FN (i.e. lints for Option<Option<_>>, Vec<Box<_>> and LinkedList<_>).

Okay, I'm most definitely out of my depth here. I don't want to end up blocking other merges since this seems to be a blocker all-up for Clippy unless I'm mistaken. I'm reading up and learning about Hir and all these stuff now, but not sure how to result (or test whether the changes fix the issue).

I'm willing to continue to give it a go with some more guidance, I just don't want to become a blocker if it needs to get fixed quickly :P.

[llogiq]

I guess part of the issue needs to be fixed on the rustc side.

[flip1995]

@jolson88 Don't worry, you're not a blocker, maintainers can push to this PR.

I guess part of the issue needs to be fixed on the rustc side.

Yes, I also think this is the case here. I think this is the same issue as rust-lang/rust#64250 (comment), just with fields of inner structs, instead of return types. Another ping @Xanewok (sorry for bothering you with this, but you fixed the typeck tables nesting for return types), what's your take on this? Minimal reproducer:

trait Trait {
    type Assoc;
}

fn func() {
    struct A<T: Trait> {
        field: T::Assoc,
    }
}

I think that this function also needs a fix:

    fn process_struct_field_def(&mut self, field: &ast::StructField, parent_id: NodeId) {
        let field_data = self.save_ctxt.get_field_data(field, parent_id);
        if let Some(field_data) = field_data {
            let hir_id = self.tcx.hir().node_to_hir_id(field.id);
            self.dumper.dump_def(&access_from!(self.save_ctxt, field, hir_id), field_data);
        }
    }

(It was untouched by your PR)

[Xanewok]

@flip1995 woops, thanks for catching that! Need to nest the table when processing the struct, will work on the patch now.

However, that bit of code is only executed in -Zsave-analysis and I don't think you run it together with Clippy?

[flip1995]

Thanks!

No we don't run it in Clippy. That means we probably need to find another solution for the qpath_res problem. (?) I think the best thing to do here is just to opt out of linting if the type is an associated type.

[llogiq]

@flip1995 can you push your current state so we can get CI working again, even with false negatives?

[tesuji]

Is this a build bot error? The build is fail but it merged the PR.
cc @pietroalbini

[pietroalbini]

@lzutao no, the commits here were actually merged along with the other changes in #4551, as that PR was built on top of this.

[tesuji]

Oops, thanks. I should have paid more attention to it.