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

ICE: MIR const-checker found novel structural match violation #73431

Closed
jamesbornholt opened this issue Jun 17, 2020 · 7 comments
Closed

ICE: MIR const-checker found novel structural match violation #73431

jamesbornholt opened this issue Jun 17, 2020 · 7 comments
Labels
C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jamesbornholt
Copy link

This code causes an ICE on both beta and nightly, but not stable. bisect-rustc pointed me to #67343.

It seems to be an issue with the generic Wrapper<T> -- manually implementing Zero for Wrapper<usize> doesn't trigger the issue.

Code

pub trait Zero {
    const ZERO: Self;
}

impl Zero for usize {
    const ZERO: Self = 0;
}

impl<T: Zero> Zero for Wrapper<T> {
    const ZERO: Self = Wrapper(T::ZERO);
}

#[derive(Debug, PartialEq, Eq)]
pub struct Wrapper<T>(T);

fn is_zero(x: Wrapper<usize>) -> bool {
    match x {
        Zero::ZERO => true,
        _ => false,
    }
}

Meta

rustc --version --verbose:

rustc 1.45.0-beta.2 (1dc0f6d8e 2020-06-15)
binary: rustc
commit-hash: 1dc0f6d8ef4ff19cfbe468ede40a1a6596f48957
commit-date: 2020-06-15
host: x86_64-apple-darwin
release: 1.45.0-beta.2
LLVM version: 10.0

Error output

error: internal compiler error: src/librustc_mir_build/hair/pattern/const_to_pat.rs:111: MIR const-checker found novel structural match violation

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:907:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

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.45.0-beta.2 (1dc0f6d8e 2020-06-15) running on x86_64-apple-darwin

note: compiler flags: -C embed-bitcode=no -C debuginfo=2 -C incremental --crate-type lib
Backtrace

stack backtrace:
   0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
   1: core::fmt::write
   2: std::io::Write::write_fmt
   3: std::panicking::default_hook::{{closure}}
   4: std::panicking::default_hook
   5: rustc_driver::report_ice
   6: std::panicking::rust_panic_with_hook
   7: std::panicking::begin_panic
   8: rustc_errors::HandlerInner::bug
   9: rustc_errors::Handler::bug
  10: rustc_middle::util::bug::opt_span_bug_fmt::{{closure}}
  11: rustc_middle::ty::context::tls::with_opt::{{closure}}
  12: rustc_middle::ty::context::tls::with_opt
  13: rustc_middle::util::bug::opt_span_bug_fmt
  14: rustc_middle::util::bug::bug_fmt
  15: rustc_mir_build::hair::pattern::const_to_pat::ConstToPat::to_pat
  16: rustc_infer::infer::InferCtxtBuilder::enter
  17: rustc_mir_build::hair::pattern::PatCtxt::lower_path
  18: rustc_mir_build::hair::pattern::PatCtxt::lower_pattern
  19: rustc_mir_build::hair::pattern::check_match::MatchVisitor::lower_pattern
  20: <core::iter::adapters::Map<I,F> as core::iter::traits::iterator::Iterator>::fold
  21: <rustc_mir_build::hair::pattern::check_match::MatchVisitor as rustc_hir::intravisit::Visitor>::visit_expr
  22: <rustc_mir_build::hair::pattern::check_match::MatchVisitor as rustc_hir::intravisit::Visitor>::visit_expr
  23: rustc_mir_build::hair::pattern::check_match::check_match
  24: rustc_middle::ty::query::<impl rustc_query_system::query::config::QueryAccessors<rustc_middle::ty::context::TyCtxt> for rustc_middle::ty::query::queries::check_match>::compute
  25: rustc_middle::dep_graph::<impl rustc_query_system::dep_graph::DepKind for rustc_middle::dep_graph::dep_node::DepKind>::with_deps
  26: rustc_query_system::dep_graph::graph::DepGraph<K>::with_task_impl
  27: rustc_data_structures::stack::ensure_sufficient_stack
  28: rustc_query_system::query::plumbing::get_query_impl
  29: rustc_query_system::query::plumbing::ensure_query_impl
  30: rustc_session::utils::<impl rustc_session::session::Session>::time
  31: rustc_session::utils::<impl rustc_session::session::Session>::time
  32: rustc_interface::passes::analysis
  33: rustc_middle::ty::query::<impl rustc_query_system::query::config::QueryAccessors<rustc_middle::ty::context::TyCtxt> for rustc_middle::ty::query::queries::analysis>::compute
  34: rustc_middle::dep_graph::<impl rustc_query_system::dep_graph::DepKind for rustc_middle::dep_graph::dep_node::DepKind>::with_deps
  35: rustc_query_system::dep_graph::graph::DepGraph<K>::with_task_impl
  36: rustc_query_system::query::plumbing::get_query_impl
  37: rustc_middle::ty::context::tls::enter_global
  38: rustc_interface::interface::run_compiler_in_existing_thread_pool
  39: rustc_ast::attr::with_globals

@jamesbornholt jamesbornholt added C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 17, 2020
@jonas-schievink jonas-schievink added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Jun 17, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 17, 2020
@lcnr
Copy link
Contributor

lcnr commented Jun 17, 2020

cc @ecstatic-morse

@lcnr
Copy link
Contributor

lcnr commented Jun 17, 2020

so afaict, mir_const_qualif checks the polymorphic anonymous const for<T: Zero>: Wrapper(T::ZERO), which is correctly identified as potentially not being structurally match while search_for_structural_match_violation is run on the fully monomorphized Wrapper<usize>, which is correctly identified as structural match.

So in a sense custum_eq is too conservative here, don't really know how to fix this without just disabling this check

@Dylan-DPC-zz
Copy link

Marking it as p-critical as per our discussion

@Dylan-DPC-zz Dylan-DPC-zz added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 17, 2020
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this issue Jun 17, 2020
tmandry added a commit to tmandry/rust that referenced this issue Jun 17, 2020
Make novel structural match violations not a `bug`

Fixes (on master) rust-lang#73431.

Ideally, `CustomEq` would emit a strict subset of the structural match errors that are found by `search_for_structural_match_violation`, since it allows more cases due to value-based reasoning. However, const qualification is more conservative than `search_for_structural_match_violation` around associated constants, since qualification does not try to substitute type parameters.

In the long term, we should probably make const qualification work for generic associated constants, but I don't like extending its capabilities even further.

r? @pnkfelix
@RalfJung
Copy link
Member

So speaking of structure match/equality, was there any progress in documenting what the exact guarantees are here? Withe each bug like this, I feel more unease around the lack of documentation here.^^ And that is even more so when two checks for that property disagree...

The previous Zulip discussion did not really reach any conclusion.

RalfJung added a commit to RalfJung/rust that referenced this issue Jun 18, 2020
Make novel structural match violations not a `bug`

Fixes (on master) rust-lang#73431.

Ideally, `CustomEq` would emit a strict subset of the structural match errors that are found by `search_for_structural_match_violation`, since it allows more cases due to value-based reasoning. However, const qualification is more conservative than `search_for_structural_match_violation` around associated constants, since qualification does not try to substitute type parameters.

In the long term, we should probably make const qualification work for generic associated constants, but I don't like extending its capabilities even further.

r? @pnkfelix
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 18, 2020
Make novel structural match violations not a `bug`

Fixes (on master) rust-lang#73431.

Ideally, `CustomEq` would emit a strict subset of the structural match errors that are found by `search_for_structural_match_violation`, since it allows more cases due to value-based reasoning. However, const qualification is more conservative than `search_for_structural_match_violation` around associated constants, since qualification does not try to substitute type parameters.

In the long term, we should probably make const qualification work for generic associated constants, but I don't like extending its capabilities even further.

r? @pnkfelix
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Jun 26, 2020
@LeSeulArtichaut
Copy link
Contributor

Since #73446 was beta-backported, I will remove the critical priority from this issue.

@LeSeulArtichaut LeSeulArtichaut added I-prioritize Issue: Indicates that prioritization has been requested for this issue. P-medium Medium priority and removed P-critical Critical priority I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 1, 2020
@LeSeulArtichaut
Copy link
Contributor

Assigning P-medium as discussed here.

@lcnr
Copy link
Contributor

lcnr commented Jul 9, 2020

Tracking the remaining work here in #73448, closing this issue

@lcnr lcnr closed this as completed Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants