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

Use associated constants in std::num::{Zero,One} #25684

Closed
wants to merge 1 commit into from

Conversation

tamird
Copy link
Contributor

@tamird tamird commented May 21, 2015

This refactor causes the following ICE:

$ /usr/bin/make -j3 check-stage1 RUST_BACKTRACE=1
cfg: version 1.2.0-dev (72e9aaa7a 2015-05-21) (built 2015-05-21)
cfg: build triple x86_64-apple-darwin
cfg: host triples x86_64-apple-darwin
cfg: target triples x86_64-apple-darwin
cfg: disabling rustc optimization (CFG_DISABLE_OPTIMIZE)
cfg: enabling debug assertions (CFG_ENABLE_DEBUG_ASSERTIONS)
cfg: enabling debuginfo (CFG_ENABLE_DEBUGINFO)
cfg: host for x86_64-apple-darwin is x86_64
cfg: os for x86_64-apple-darwin is apple-darwin
cfg: disabling C++ optimization (CFG_DISABLE_OPTIMIZE_CXX)
cfg: good valgrind for x86_64-apple-darwin is
cfg: using CC=clang (CFG_CC)
cfg: using CXX=clang++ (CFG_CXX)
cfg: disabling valgrind run-pass tests
cfg: no xelatex found, disabling LaTeX docs
cfg: no pandoc found, omitting PDF and EPUB docs
cfg: including test rules
cfg: javac not available, skipping lexer test...
rustc: x86_64-apple-darwin/stage0/lib/rustlib/x86_64-apple-darwin/lib/libcore
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: run with `RUST_BACKTRACE=1` for a backtrace
thread 'rustc' panicked at 'path not fully resolved: PathResolution { base_def: DefTyParam(TypeSpace, 0, DefId { krate: 0, node: 115188 }, "A"(326)), last_private: LastMod(AllPublic), depth: 1 }', /Users/rustbuild/src/rust-buildbot/slave/snap3-mac/build/src/librustc/middle/def.rs:81

stack backtrace:
   1:        0x1088cb99f - sys::backtrace::write::ha6a0ef69d836f288KVr
   2:        0x1088d04b2 - panicking::on_panic::h79a3ac05487064ffNVv
   3:        0x1088aca75 - rt::unwind::begin_unwind_inner::h53568b1e4e1d5c49wDv
   4:        0x1088ad5cc - rt::unwind::begin_unwind_fmt::h42754663daaabff7CCv
   5:        0x1076ff7f8 - middle::ty::resolve_expr::h01be9bdb60bc5d07Tg6
   6:        0x10784c2c2 - middle::ty::expr_kind::he9ffd29dd326a14bZi6
   7:        0x10784c06d - middle::ty::expr_is_lval::h55b067779b7ace9bWh6
   8:        0x1074b5244 - check::check_expr_with_unifier::h11317710005126736159
   9:        0x10747bc0d - check::op::check_binop::h63cfeed1e166bc9asYm
  10:        0x1074b32ce - check::check_expr_with_unifier::h11317710005126736159
  11:        0x1074c965f - check::check_expr_with_unifier::h1631999723096368142
  12:        0x107487f3a - check::check_block_with_expected::h959a715da7c43d7bvds
  13:        0x10749ca0d - check::check_expr_with_unifier::check_then_else::h5fd509e0bd507d79TCq
  14:        0x1074b35b0 - check::check_expr_with_unifier::h11317710005126736159
  15:        0x10748829b - check::check_block_with_expected::h959a715da7c43d7bvds
  16:        0x10746b828 - check::check_fn::heab4df53bfe14a32pSn
  17:        0x1074837e5 - check::check_bare_fn::hc942e99ce3f1c922YHn
  18:        0x10748f107 - check::check_method_body::h8bf3cf4630998608Zjo
  19:        0x10748168b - check::CheckItemBodiesVisitor<'a, 'tcx>.Visitor<'tcx>::visit_item::h9347c778160154f51En
  20:        0x107481aee - check::CheckItemBodiesVisitor<'a, 'tcx>.Visitor<'tcx>::visit_item::h9347c778160154f51En
  21:        0x107545c5a - check_crate::closure.38461
  22:        0x107541087 - check_crate::h28a26a622d6b3e59tCC
  23:        0x1070c7d54 - driver::phase_3_run_analysis_passes::he5b5c42951feb035tGa
  24:        0x1070aa378 - driver::compile_input::h78293093b86c5c68Qba
  25:        0x107168623 - run_compiler::h1e722a2786102a3765b
  26:        0x107165d8a - boxed::F.FnBox<A>::call_box::h15307864961899385171
  27:        0x1071652d7 - rt::unwind::try::try_fn::h3375314414886690993
  28:        0x1088d2688 - rust_try_inner
  29:        0x1088d2675 - rust_try
  30:        0x1071655ae - boxed::F.FnBox<A>::call_box::h7666835191292233835
  31:        0x1088cefad - sys::thread::Thread::new::thread_start::h41e207b511aa1bfbvYu
  32:     0x7fff8ea0c267 - _pthread_body
  33:     0x7fff8ea0c1e4 - _pthread_start

make: *** [x86_64-apple-darwin/stage0/lib/rustlib/x86_64-apple-darwin/lib/stamp.core] Error 101

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@Gankra
Copy link
Contributor

Gankra commented May 21, 2015

What's the status of this in the face of #25687 ?

@tamird
Copy link
Contributor Author

tamird commented May 21, 2015

It's a superset - I'll rebase this when #25687 lands.

@Gankra
Copy link
Contributor

Gankra commented May 21, 2015

The extra bits just ICE, though, right?

@tamird
Copy link
Contributor Author

tamird commented May 21, 2015

Yeah - I'd file an issue but I wasn't able to reduce the ICE down

Manishearth added a commit to Manishearth/rust that referenced this pull request May 22, 2015
@quantheory
Copy link
Contributor

We are in the unhappy position where trait-associated constants are not really useful, because you can't actually use them from type parameters.

For use of trait-associated constants in non-constant expressions, we need something similar to the existing type projections, so that the compiler isn't confused about constants that can't be resolved. For use in constant expressions, see: rust-lang/rfcs#1062.

@eddyb
Copy link
Member

eddyb commented May 23, 2015

Like I told @tamird on IRC, this particular ICE is a mishandling of unresolved paths to associated items.
They can all be treated as if they were RvalueDatum because both methods and associated constants are that.

oli-obk pushed a commit to oli-obk/rust that referenced this pull request May 23, 2015
oli-obk pushed a commit to oli-obk/rust that referenced this pull request May 23, 2015
oli-obk pushed a commit to oli-obk/rust that referenced this pull request May 23, 2015
oli-obk pushed a commit to oli-obk/rust that referenced this pull request May 23, 2015
@tamird tamird force-pushed the num-associated-consts branch from 72e9aaa to 9bef19d Compare May 23, 2015 19:14
@quantheory
Copy link
Contributor

@eddyb Ah, I see. We are asking if it is an lvalue before the type is checked, which is where resolve_ufcs is called. However, if that is addressed, I still don't think that this code will make it past check_const, due to #25046. In fact, #25091 puts in a check to simply reject such code, until we have something better that doesn't just hit an ICE.

@bluss
Copy link
Member

bluss commented May 24, 2015

This makes the trait less general (only available to types that can produce a zero in constants). I suppose that's useful and ok, it just needs to be mentioned.

@tamird
Copy link
Contributor Author

tamird commented May 24, 2015

@bluss
Copy link
Member

bluss commented May 24, 2015

BigNum are allocating and can never impl the new trait (?) I don't think it's important.

@eddyb
Copy link
Member

eddyb commented May 24, 2015

@bluss if the BigInt implementation uses a variant/tags for the non-allocating cases (fitting in u64, usually), you may be able to put that in a constant. It's not a given, though.

@alexcrichton
Copy link
Member

I agree with @bluss that this probably excludes bignum from implementing the trait, but I'm somewhat hesitant on casting it aside as not important. The std::num module got by at 1.0 by moving all methods on primitives to inherent implementations, so there are actually 0 stable traits in std::num. One reason we didn't stabilize anything is that we couldn't strike the right balance between supporting primitives while allowing for other implementors.

These existence of these traits is necessitated by the iterators that use them, and I believe that's needed for inference to work out ok, so these need to stick around. I think it's fine to experiment with them having associated constants instead of methods, but we'd likely want to give everything a rethink still before stabilizing them.

@tamird tamird force-pushed the num-associated-consts branch 2 times, most recently from 7322ac0 to 8d7f2e4 Compare May 26, 2015 19:26
@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 26, 2015
@tamird
Copy link
Contributor Author

tamird commented May 27, 2015

As a temporary measure, #25091 rejects this code, so this PR is blocked behind a future change that allows associated constants to depend on Self.

@tamird tamird closed this May 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants