ICE: improper_ctypes: Option nonnull optimization not applied?

[steffahn]

Code

use std::cell::UnsafeCell;

extern "C" {
    pub fn foo(_: Option<UnsafeCell<&i32>>);
}

Code from playground linked in this comment. Apparently, since that discussion took place the #[repr(no_niche)] flag got implemented and was added to UnsafeCell, yet the code for checking FFI-safety was perhaps not updated accordingly.

Meta

note: rustc 1.50.0-nightly (5be3f9f10 2020-12-03) running on x86_64-unknown-linux-gnu

note: compiler flags: -C embed-bitcode=no -C codegen-units=1 -C debuginfo=2 --crate-type lib

note: some of the compiler flags provided by cargo are hidden

Error output

error: internal compiler error: compiler/rustc_lint/src/types.rs:774:13: improper_ctypes: Option nonnull optimization not applied?

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

note: the compiler unexpectedly panicked. this is a bug.

Backtrace

error: internal compiler error: compiler/rustc_lint/src/types.rs:774:13: improper_ctypes: Option nonnull optimization not applied?

thread 'rustc' panicked at 'Box<Any>', compiler/rustc_errors/src/lib.rs:958:9
stack backtrace:
   0: std::panicking::begin_panic
   1: rustc_errors::HandlerInner::bug
   2: rustc_errors::Handler::bug
   3: rustc_middle::util::bug::opt_span_bug_fmt::{{closure}}
   4: rustc_middle::ty::context::tls::with_opt::{{closure}}
   5: rustc_middle::ty::context::tls::with_opt
   6: rustc_middle::util::bug::opt_span_bug_fmt
   7: rustc_middle::util::bug::bug_fmt
   8: rustc_lint::types::repr_nullable_ptr
   9: rustc_lint::types::ImproperCTypesVisitor::check_type_for_ffi
  10: rustc_lint::types::ImproperCTypesVisitor::check_type_for_ffi_and_report_errors
  11: rustc_lint::types::ImproperCTypesVisitor::check_foreign_fn
  12: <rustc_lint::types::ImproperCTypesDeclarations as rustc_lint::passes::LateLintPass>::check_foreign_item
  13: rustc_hir::intravisit::Visitor::visit_nested_foreign_item
  14: rustc_hir::intravisit::walk_item
  15: rustc_hir::intravisit::Visitor::visit_nested_item
  16: rustc_lint::late::late_lint_mod
  17: rustc_middle::ty::query::<impl rustc_query_system::query::config::QueryAccessors<rustc_middle::ty::context::TyCtxt> for rustc_middle::ty::query::queries::lint_mod>::compute
  18: rustc_query_system::dep_graph::graph::DepGraph<K>::with_task_impl
  19: rustc_data_structures::stack::ensure_sufficient_stack
  20: rustc_query_system::query::plumbing::get_query_impl
  21: rustc_query_system::query::plumbing::ensure_query_impl
  22: rustc_session::utils::<impl rustc_session::session::Session>::time
  23: rustc_interface::passes::analysis::{{closure}}::{{closure}}
  24: rustc_session::utils::<impl rustc_session::session::Session>::time
  25: rustc_interface::passes::analysis
  26: rustc_middle::ty::query::<impl rustc_query_system::query::config::QueryAccessors<rustc_middle::ty::context::TyCtxt> for rustc_middle::ty::query::queries::analysis>::compute
  27: rustc_query_system::dep_graph::graph::DepGraph<K>::with_eval_always_task
  28: rustc_data_structures::stack::ensure_sufficient_stack
  29: rustc_query_system::query::plumbing::get_query_impl
  30: rustc_interface::passes::QueryContext::enter
  31: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::enter
  32: rustc_span::with_source_map
  33: scoped_tls::ScopedKey<T>::set
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.50.0-nightly (5be3f9f10 2020-12-03) running on x86_64-unknown-linux-gnu

note: compiler flags: -C embed-bitcode=no -C codegen-units=1 -C debuginfo=2 --crate-type lib

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
#0 [lint_mod] linting top-level module
#1 [analysis] running analysis passes on this crate
end of query stack

[apiraino]

Assigning P-medium as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

[matthiaskrgr]

This ICEs since 1.43 and built without error/warning before !
@rustbot modify labels: +regression-from-stable-to-stable

[steffahn]

This ICEs since 1.43 and built without error/warning before !

@matthiaskrgr Please note that this is not the most typical kind of “regression”. The point being: The code is not supposed to compile anymore, since UnsafeCell was changed in a way that makes Option<UnsafeCell<&T>> indeed not FFI-safe. And this change was to fix a soundness issue around UnsafeCell, so it is considered acceptable breakage.

The only problem that remains is that the code produces an ICE instead of a nice error message.

The regression-from-stable-to-stable label describes itself as, “Performance or correctness regression from one stable version to another.” Overall correctness of the compiler has (in my view) only improved over before. One could perhaps argue that the correctness of the FFI-safety check has regressed (as it wasn’t kept up to date with the introduction of repr(no_niche)), so I’ll leave the label for now.

[matthiaskrgr]

IMO even a change from "code was rejected" to "compiler crashes on this code" classifies as a regression.
In this case, there was a stable version which accepted the code, and a later stable version which crashes while dealing with the code.

https://rust-lang.zulipchat.com/#narrow/stream/227806-t-compiler.2Fwg-prioritization/topic/adding.20labels.20to.20stable-to-stable.20regressions/near/220528767

[steffahn]

IMO even a change from "code was rejected" to "compiler crashes on this code" classifies as a regression.
In this case, there was a stable version which accepted the code, and a later stable version which crashes while dealing with the code.

Exactly. I do aggree that a "code was rejected" to "compiler crashes on this code" should classify as a regression.

However, this situation is not "code was rejected" to "compiler crashes on this code", but instead "code was accepted" to "compiler crashes on this code even though it is expected to error now because we had an acceptable breaking chance (bugfix)". There never was an intermediate "code was rejected" stage that we regressed from.