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

TypeId doesn't match itself in generic function. #73976

Closed
rodrimati1992 opened this issue Jul 2, 2020 · 20 comments · Fixed by #74538
Closed

TypeId doesn't match itself in generic function. #73976

rodrimati1992 opened this issue Jul 2, 2020 · 20 comments · Fixed by #74538
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@rodrimati1992
Copy link
Contributor

I tried this code:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=57bb53139f14a69326e005ea9d9f7cd7

#![feature(const_type_id)]

use std::any::TypeId;

pub struct GetTypeId<T>(T);

impl<T: 'static> GetTypeId<T> {
    pub const VALUE: TypeId = TypeId::of::<T>();
}

const fn check_type_id<T: 'static>() -> bool {
    matches!(GetTypeId::<T>::VALUE, GetTypeId::<T>::VALUE)
}

fn main() {
    assert!(check_type_id::<usize>());
}

I expected the code to run without errors

Instead, the assertion failed.

Meta

Compiler version: 1.46.0-nightly (2020-07-01 f781bab)

@rodrimati1992 rodrimati1992 added the C-bug Category: This is a bug. label Jul 2, 2020
@jonas-schievink jonas-schievink added A-const-eval Area: constant evaluation (mir interpretation) requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 2, 2020
@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 2, 2020

#![feature(const_type_id)]
#![feature(core_intrinsics)]

pub struct GetTypeId<T>(T);

impl<T: 'static> GetTypeId<T> {
    pub const VALUE: u64 = std::intrinsics::type_id::<T>();
}

const fn check_type_id<T: 'static>() -> bool {
    matches!(GetTypeId::<T>::VALUE, GetTypeId::<T>::VALUE)
}

fn main() {
    assert!(check_type_id::<usize>());
}

Seems something wrong with the intrinsics evaluation.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 2, 2020

I think the issue is that TyKind::Param is in place when we try to hash Ty. I am not very familiar with the code here but I think this might be evaluated before monomorphization which causes this issue.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 2, 2020

Another example showing this is pre-monomorphization:

#![feature(const_type_id)]
#![feature(core_intrinsics)]

struct X([u8; std::intrinsics::type_id::<Self>() as usize]);

fn main() {
    let _ = std::mem::MaybeUninit::<X>::uninit();
}

rustc happily compiles this, but I would expect something like cycle detected when const-evaluating + checking X::0::{{constant}}#0.

@sportzer
Copy link

sportzer commented Jul 3, 2020

I was trying to do something somewhat similar with const type_id when I filed #72602. There seem to be some inconsistencies in what combinations of generics and patterns are allowed, which might be contributing to the problem.

@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2020

I have no idea what is going on, but it certainly looks odd... Cc @rust-lang/wg-const-eval @eddyb @nikomatsakis

@oli-obk
Copy link
Contributor

oli-obk commented Jul 3, 2020

sym::type_id => ConstValue::from_u64(tcx.type_id_hash(tp_ty)),
needs to monomorphize the type before evaluating it I'd guess (an assert checking for needs_subst would make sense there, too I'd think).

@eddyb
Copy link
Member

eddyb commented Jul 3, 2020

@oli-obk I wonder if instead of needs_subst we should more explicitly check for "type or const params", but I think it's mostly about aesthetics thanks to miri working with lifetime-erased types.

You said "assert" but I think the type is already monomorphized and what we need to do is return TooGeneric if it's not monomorphic. Same for type_name, actually - any reflection that doesn't rely on layout_of will have this problem.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2020

oh right, this may be an evaluation happening too early, so TooGeneric is totally reasonable to emit here. I'm always wary of emitting TooGeneric.

@oli-obk oli-obk added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jul 4, 2020
@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 4, 2020

oh right, this may be an evaluation happening too early, so TooGeneric is totally reasonable to emit here. I'm always wary of emitting TooGeneric.

Doesn't that mean that the code samples above will be forbidden?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2020

No, just evaluating the consts early will fail. Once everything is monomorphized it will work.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 4, 2020

No, just evaluating the consts early will fail. Once everything is monomorphized it will work.

I have an attempt here: https://github.com/nbdd0121/rust/tree/issue-73976. It prevents OP's snippet from compiling:

error: could not evaluate constant pattern
  --> test.rs:12:37
   |
12 |     matches!(GetTypeId::<T>::VALUE, GetTypeId::<T>::VALUE)
   |                                     ^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

@oli-obk
Copy link
Contributor

oli-obk commented Jul 5, 2020

hmm... maybe const prop is interfering here. Can you try running the same build with -Ztreat-err-as-bug and RUST_BACKTRACE=1?

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 5, 2020

error: could not evaluate constant pattern
  --> test.rs:13:37
   |
13 |     matches!(GetTypeId::<T>::VALUE, GetTypeId::<T>::VALUE)
   |                                     ^^^^^^^^^^^^^^^^^^^^^

thread 'rustc' panicked at 'aborting due to `-Z treat-err-as-bug=1`', src/librustc_errors/lib.rs:942:13
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::emit_diagnostic
   9: rustc_errors::Handler::emit_diag_at_span
  10: rustc_mir_build::hair::pattern::PatCtxt::lower_path
  11: rustc_mir_build::hair::pattern::PatCtxt::lower_pattern
  12: rustc_mir_build::hair::pattern::check_match::MatchVisitor::lower_pattern
  13: <core::iter::adapters::Map<I,F> as core::iter::traits::iterator::Iterator>::fold
  14: <rustc_mir_build::hair::pattern::check_match::MatchVisitor as rustc_hir::intravisit::Visitor>::visit_expr
  15: <rustc_mir_build::hair::pattern::check_match::MatchVisitor as rustc_hir::intravisit::Visitor>::visit_expr
  16: rustc_mir_build::hair::pattern::check_match::check_match
  17: 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
  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_session::utils::<impl rustc_session::session::Session>::time
  24: rustc_interface::passes::analysis
  25: rustc_middle::ty::query::<impl rustc_query_system::query::config::QueryAccessors<rustc_middle::ty::context::TyCtxt> for rustc_middle::ty::query::queries::analysis>::compute
  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_middle::ty::context::tls::enter_global
  30: rustc_interface::interface::run_compiler_in_existing_thread_pool
  31: scoped_tls::ScopedKey<T>::set
  32: rustc_ast::attr::with_globals
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose 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.46.0-dev running on x86_64-unknown-linux-gnu

note: compiler flags: -Z treat-err-as-bug

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2020

Ah, so this is a different issue. It's the same issue (even if a different symptom) of running

struct Foo<T>(T);
impl<T> Foo<T> {
    const BAR: usize = std::mem::size_of::<T>();
}

fn muh<T>() {
    match 42 {
        Foo::<T>::BAR => {}
        _ => {}
    }
}

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2020

Patterns need to be monomorphic, you can never have a pattern that depends on generic parameters. We may lift this restriction in the future, but it's a separate issue and needs a language RFC afaik

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 6, 2020

struct Foo<T>(T);
impl<T> Foo<T> {
    const BAR: usize = std::mem::size_of::<T>();
}

fn muh<T>() {
    match 42 {
        Foo::<T>::BAR => {}
        _ => {}
    }
}

This gives:

error[E0158]: associated consts cannot be referenced in patterns

in stable and gives

error: could not evaluate constant pattern

in nightly.

We will need a better diagnostics for the TooGeneric error.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 7, 2020

struct Foo<T>(T);
impl<T> Foo<T> {
    const BAR: usize = 42;
}

fn muh<T>() {
    match 42 {
        Foo::<T>::BAR => {}
        _ => {}
    }
}

works on stable, so the old diagnostic was wrong. It's specifically associated constants that depend on generic parameters.

@KodrAus
Copy link
Contributor

KodrAus commented Jul 12, 2020

@oli-obk so is the approach of returning TooGeneric all we need to do here? If so @nbdd0121 would you be happy to open your branch as a PR so we could look at adding some ui tests and whatnot?

@nbdd0121
Copy link
Contributor

@KodrAus The thing I worried is that the error error: could not evaluate constant pattern isn't very helpful. Maybe fixing that diagnostics should be a separate PR?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 13, 2020

Yea, cleaning up the diagnostics can be done after fixing the bug

Manishearth added a commit to Manishearth/rust that referenced this issue Jul 22, 2020
Guard against non-monomorphized type_id intrinsic call

This PR checks whether the type is sufficient monomorphized when calling type_id or type_name intrinsics. If the type is not sufficiently monomorphized, e.g. used in a pattern, the code will be rejected.

Fixes rust-lang#73976
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 22, 2020
Guard against non-monomorphized type_id intrinsic call

This PR checks whether the type is sufficient monomorphized when calling type_id or type_name intrinsics. If the type is not sufficiently monomorphized, e.g. used in a pattern, the code will be rejected.

Fixes rust-lang#73976
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 22, 2020
Guard against non-monomorphized type_id intrinsic call

This PR checks whether the type is sufficient monomorphized when calling type_id or type_name intrinsics. If the type is not sufficiently monomorphized, e.g. used in a pattern, the code will be rejected.

Fixes rust-lang#73976
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 22, 2020
Guard against non-monomorphized type_id intrinsic call

This PR checks whether the type is sufficient monomorphized when calling type_id or type_name intrinsics. If the type is not sufficiently monomorphized, e.g. used in a pattern, the code will be rejected.

Fixes rust-lang#73976
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 22, 2020
Guard against non-monomorphized type_id intrinsic call

This PR checks whether the type is sufficient monomorphized when calling type_id or type_name intrinsics. If the type is not sufficiently monomorphized, e.g. used in a pattern, the code will be rejected.

Fixes rust-lang#73976
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 22, 2020
Guard against non-monomorphized type_id intrinsic call

This PR checks whether the type is sufficient monomorphized when calling type_id or type_name intrinsics. If the type is not sufficiently monomorphized, e.g. used in a pattern, the code will be rejected.

Fixes rust-lang#73976
@bors bors closed this as completed in 4828895 Jul 23, 2020
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 30, 2020
Improve diagnostics when constant pattern is too generic

This PR is a follow-up to PR rust-lang#74538 and issue rust-lang#73976

When constants queries Layout, TypeId or type_name of a generic parameter, instead of emitting `could not evaluate constant pattern`, we will instead emit a more detailed message `constant pattern depends on a generic parameter`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants