Stream: t-compiler

Topic: #68977: Revert `compute_const` changes in WF


Bastian Kauschke (Mar 18 2020 at 13:54, on Zulip):

I am currently trying to fix https://github.com/rust-lang/rust/issues/68977 and need some mentoring

@varkor @eddyb

eddyb (Mar 18 2020 at 13:56, on Zulip):

well, the first step would be to rename it from compute_array_len back to compute_const and fix the comment

Bastian Kauschke (Mar 18 2020 at 13:56, on Zulip):

as far as I understand the following changes have to be made:

  1. rename compute_array_len to compute_const
  2. in WfPredicates::compute, call compute_const for const parameters of adts
  3. add test mentioned in the issue
eddyb (Mar 18 2020 at 14:00, on Zulip):

in WfPredicates::compute, call compute_const for const parameters of adts

ideally it would be tied to Substs in general, but I guess the WF code is too messy for that

eddyb (Mar 18 2020 at 14:01, on Zulip):

if you add it to nominal_obligations that's the best way to ensure everything that has substs will be checked

eddyb (Mar 18 2020 at 14:01, on Zulip):

funnily enough I just realized compute_const will be able to recurse through nominal_obligations back to compute_const, which is a case I missed

eddyb (Mar 18 2020 at 14:02, on Zulip):

(associated consts, for example, can have their own const generic parameters in scope, so they also need to be checked)

Bastian Kauschke (Mar 18 2020 at 14:08, on Zulip):

thanks.

Is there a good source on what nominal in nominal_obligationsmeans?

eddyb (Mar 18 2020 at 14:08, on Zulip):

nominal vs structural types

eddyb (Mar 18 2020 at 14:09, on Zulip):

literally "has a name". in this case, has a DefId

eddyb (Mar 18 2020 at 14:09, on Zulip):

user-defined types are nominal, a tuple or array type is structural

eddyb (Mar 18 2020 at 14:09, on Zulip):

@Bastian Kauschke https://en.wikipedia.org/wiki/Nominal_type_system

Bastian Kauschke (Mar 18 2020 at 14:34, on Zulip):

is there a reason why WfPredicate::cause requires &mut self?

Bastian Kauschke (Mar 18 2020 at 14:35, on Zulip):
fn cause(&mut self, code: traits::ObligationCauseCode<'tcx>) -> traits::ObligationCause<'tcx> {
    traits::ObligationCause::new(self.span, self.body_id, code)
}
eddyb (Mar 18 2020 at 14:36, on Zulip):

doesn't look like it, lol

eddyb (Mar 18 2020 at 14:36, on Zulip):

probably copy-paste mistake

Bastian Kauschke (Mar 18 2020 at 14:58, on Zulip):

eddyb said:

if you add it to nominal_obligations that's the best way to ensure everything that has substs will be checked

Not quite sure how this should be done.

Bastian Kauschke (Mar 18 2020 at 15:12, on Zulip):

nm, should be solved

Bastian Kauschke (Mar 18 2020 at 15:24, on Zulip):

https://github.com/rust-lang/rust/pull/70107 not yet tested

Bastian Kauschke (Mar 18 2020 at 15:30, on Zulip):

how should I test that the substs must now be const evaluatable?

eddyb (Mar 18 2020 at 15:53, on Zulip):

@Bastian Kauschke you don't need to return anything, you can push to self.out directly

eddyb (Mar 18 2020 at 15:53, on Zulip):

that is, this would be a side-effect of nominal_obligations, not part of what it returns

eddyb (Mar 18 2020 at 16:10, on Zulip):

like, nominal_obligations is just a good place to hijack

Bastian Kauschke (Mar 18 2020 at 16:11, on Zulip):

Isn't that kind of ugly though? returning something that is added to self.out 5 out of 6 times and modifying self.out internally at the same time?

eddyb (Mar 18 2020 at 16:11, on Zulip):

it is but that's because this entire system is using Ty::walk

eddyb (Mar 18 2020 at 16:11, on Zulip):

which is IMO terrible

eddyb (Mar 18 2020 at 16:11, on Zulip):

@nikomatsakis ^^ more evidence Ty::walk is bad :P

eddyb (Mar 18 2020 at 16:12, on Zulip):

if wf used TypeVisitor you'd just add visit_const and that'd be that

eddyb (Mar 18 2020 at 16:13, on Zulip):

frankly this bug would likely be impossible with TypeVisitor

eddyb (Mar 18 2020 at 16:13, on Zulip):

you'd have to go out of your way to get it wrong

Bastian Kauschke (Mar 18 2020 at 16:14, on Zulip):

As tests are breaking compute_const shall be reverted :)

if wf used TypeVisitor you'd just add visit_const and that'd be that

want me to try and do this :thinking:

eddyb (Mar 18 2020 at 16:15, on Zulip):

want me to try and do this :thinking:

you could try but it's probably painful

Bastian Kauschke (Mar 18 2020 at 17:02, on Zulip):

compute const has been reverted. Still has 2 failing ui tests and I don't know whats going on there

Bastian Kauschke (Mar 18 2020 at 17:03, on Zulip):
// check-pass

#![allow(incomplete_features, dead_code, unconditional_recursion)]
#![feature(const_generics)]

fn fact<const N: usize>() {
    fact::<{ N - 1 }>();
}

fn main() {}

fails with

error: internal compiler error: broken MIR in DefId(0:3 ~ issue_66205[317d]::fact[0]) (CanonicalUserTypeAnnotation { user_ty: Canonical { max_universe: U0, variables: [], value: TypeOf(DefId(0:3 ~ issue_66205[317d]::fact[0]), UserSubsts { substs: [Const { ty: usize, val: Unevaluated(DefId(0:5 ~ issue_66205[317d]::fact[0]::{{constant}}[0]), [Const { ty: usize, val: Param(N/#0) }], None) }], user_self_ty: None }) }, span: /home/programming/rust/src/test/ui/const-generics/issues/issue-66205.rs:7:5: 7:22, inferred_ty: fn() {fact::<{ N - 1 }>} }): bad user type AscribeUserType(fn() {fact::<{ N - 1 }>}, DefId(0:3 ~ issue_66205[317d]::fact[0]) UserSubsts { substs: [Const { ty: usize, val: Unevaluated(DefId(0:5 ~ issue_66205[317d]::fact[0]::{{constant}}[0]), [Const { ty: usize, val: Param(N/#0) }], None) }], user_self_ty: None }): NoSolution

thread 'rustc' panicked at 'no errors encountered even though `delay_span_bug` issued', src/librustc_errors/lib.rs:360:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
eddyb (Mar 18 2020 at 17:04, on Zulip):

I uhhh

Bastian Kauschke (Mar 18 2020 at 17:05, on Zulip):

and

// check-pass

#![feature(const_generics)]
//~^ WARN the feature `const_generics` is incomplete and may cause the compiler to crash

struct Const<const N: usize>;

impl<const C: usize> Const<{C}> {
    fn successor() -> Const<{C + 1}> {
        Const
    }
}

fn main() {
    let _x: Const::<2> = Const::<1>::successor();
}

fails with (this might actually be a wrong test? As the error is somewhat sensible

warning: the feature `const_generics` is incomplete and may cause the compiler to crash
  --> /home/programming/rust/src/test/ui/const-generics/issues/issue-61747.rs:3:12
   |
LL | #![feature(const_generics)]
   |            ^^^^^^^^^^^^^^
   |
   = note: `#[warn(incomplete_features)]` on by default

error: constant expression depends on a generic parameter
  --> /home/programming/rust/src/test/ui/const-generics/issues/issue-61747.rs:9:23
   |
LL |     fn successor() -> Const<{C + 1}> {
   |                       ^^^^^^^^^^^^^^
   |
   = note: this may fail depending on what value the parameter takes

error: aborting due to previous error
eddyb (Mar 18 2020 at 17:05, on Zulip):

that one is correct

eddyb (Mar 18 2020 at 17:05, on Zulip):

it's equivalent to the one @varkor linked

eddyb (Mar 18 2020 at 17:05, on Zulip):

as in, it should error

Bastian Kauschke (Mar 18 2020 at 17:06, on Zulip):

great! that leaves me with src/test/ui/const-generics/issues/issue-66205.rs

eddyb (Mar 18 2020 at 17:07, on Zulip):

that one is weird as heck

eddyb (Mar 18 2020 at 17:07, on Zulip):

how does it not fail type-checking?

eddyb (Mar 18 2020 at 17:07, on Zulip):

fact::<{ N - 1 }> shouldn't be WF

Bastian Kauschke (Mar 18 2020 at 17:15, on Zulip):

currently rerunning all tests as I might have started this test while I was not yet finished, even if I am doubtful.

Bastian Kauschke (Mar 18 2020 at 17:40, on Zulip):

seems like this is an actual bug :shrug:

Bastian Kauschke (Mar 19 2020 at 14:00, on Zulip):

do you know where the WF of generic params of expressions are checked?

let a = expr::<Ty>()

eddyb (Mar 19 2020 at 14:08, on Zulip):

@Bastian Kauschke let me look, I forgot about it

eddyb (Mar 19 2020 at 14:09, on Zulip):

so this handles the overall process <https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/mod.rs#L5339>

eddyb (Mar 19 2020 at 14:10, on Zulip):

not seeing any WF checks in any of this https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/mod.rs#L5495-L5611

eddyb (Mar 19 2020 at 14:10, on Zulip):

/me pokes @nikomatsakis - where is WF for value paths supposed to be checked?

eddyb (Mar 19 2020 at 14:11, on Zulip):

@Bastian Kauschke does drop::<Foo<{ N - 1 }>>; (not calling just mentioning the type) cause an error?

nikomatsakis (Mar 19 2020 at 14:13, on Zulip):

mmmmmmm let me remember :)

eddyb (Mar 19 2020 at 14:13, on Zulip):

oh so to_ty handles it https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/mod.rs#L3267-L3271

eddyb (Mar 19 2020 at 14:14, on Zulip):

kind of disturbing that one could accidentally call AstConv::ast_ty_to_ty which will let things pass through

eddyb (Mar 19 2020 at 14:14, on Zulip):

@nikomatsakis thank goodness MIR typeck exists

eddyb (Mar 19 2020 at 14:15, on Zulip):

wat https://github.com/rust-lang/rust/search?q=add_wf_bounds&unscoped_q=add_wf_bounds

Bastian Kauschke (Mar 19 2020 at 14:16, on Zulip):
#![feature(const_generics)]

struct Foo<const N: usize>;

fn test<const N: usize>() {
    let drop_fn = std::mem::drop::<Foo<{N - 1}>>;
}

fn main() {}

returns the desired error

eddyb (Mar 19 2020 at 14:16, on Zulip):

anyway this is where substitutions to value paths get checked https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/expr.rs#L534

eddyb (Mar 19 2020 at 14:17, on Zulip):

@Bastian Kauschke so you'd have to somehow handle consts here https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/mod.rs#L3341-L3348

eddyb (Mar 19 2020 at 14:17, on Zulip):

maybe we can replace the ConstEvaluatable bound with WellFormedConst that takes a &'tcx ty::Const<'tcx>, or something

eddyb (Mar 19 2020 at 14:18, on Zulip):

wait, we have to, otherwise const inference variables wouldn't get checked

eddyb (Mar 19 2020 at 14:18, on Zulip):

@varkor @yodal ^^ oops how did I miss this

eddyb (Mar 19 2020 at 14:19, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_trait_selection/traits/wf.rs#L556-L593 needs a const analogue

varkor (Mar 19 2020 at 14:34, on Zulip):

there were so many things to make sure we replicated for consts; it's not surprising that a few have slipped through, even till now

varkor (Mar 19 2020 at 14:34, on Zulip):

maybe uses of substs.types() are a bit of a red flag

varkor (Mar 19 2020 at 14:35, on Zulip):

or, at the least, we should audit them

eddyb (Mar 19 2020 at 14:41, on Zulip):

yeaaah

eddyb (Mar 19 2020 at 14:41, on Zulip):

@varkor maybe I should go try and get rid of as many uses of Ty::walk as possible

eddyb (Mar 19 2020 at 14:41, on Zulip):

it's cute but Visitor makes more sense - the other option is changing it to give you GenericArgs which... idk

varkor (Mar 19 2020 at 14:42, on Zulip):

I'm sure you'd come across a bug or two if you did

eddyb (Mar 19 2020 at 14:42, on Zulip):

the name GenericArg still bothers me when used outside Substs :P

varkor (Mar 19 2020 at 14:43, on Zulip):

speaking of refactorings, I'd like to go through and make sure that nothing depends on parameter order anywhere

varkor (Mar 19 2020 at 14:43, on Zulip):

but I just don't have enough time at the moment :|

varkor (Mar 19 2020 at 14:44, on Zulip):

I'm reasonably satisfied with the names we have at the moment :P

varkor (Mar 19 2020 at 14:44, on Zulip):

but maybe just because they changed so frequently

Bastian Kauschke (Mar 19 2020 at 14:59, on Zulip):

what is that error? :laughing:

error[E0261]: use of undeclared lifetime name `'txc`
    --> src/librustc/ty/mod.rs:1236:22
     |
1236 |     WellFormedConst(&'txc ty::Const<'tcx>),
     |                      ^^^^ undeclared lifetime
     |
help: consider introducing lifetime `'txc` here
     |
1201 | pub enum Predicate<'txc, 'tcx> {
     |                    ^^^^^
help: consider introducing lifetime `'txc` here
     |
1199 | #[derive(<'txc>, Copy, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
     |          ^^^^^^
Bastian Kauschke (Mar 19 2020 at 14:59, on Zulip):

since when can lifetimes be derived

eddyb (Mar 19 2020 at 15:08, on Zulip):

open an issue about "considering introducing lifetime here" I guess, it shouldn't propose a suggestion at the second span

Bastian Kauschke (Mar 19 2020 at 15:17, on Zulip):

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f20a60d4dcff2873ee83bfa8d4c0f677

And I thought that compilers make some kind of sense.

eddyb (Mar 19 2020 at 15:24, on Zulip):

oh god many of the input_types uses are coupled with Ty::walk, so I have to fix that to iterate over GenericArg first

Bastian Kauschke (Mar 19 2020 at 15:25, on Zulip):

@eddyb should I try and replace ConstEvaluatable with WellFormedConst or would this clash with your current efforts?

eddyb (Mar 19 2020 at 15:27, on Zulip):

it might idk

eddyb (Mar 19 2020 at 15:27, on Zulip):

if I change Ty::walk to be all about GenericArg, we can do the main change of the PR much better

Bastian Kauschke (Mar 19 2020 at 15:51, on Zulip):

hmm,

are there other issues I might help with?

Bastian Kauschke (Mar 19 2020 at 15:52, on Zulip):

Will stop working on this for now until you re done

eddyb (Mar 19 2020 at 19:25, on Zulip):

@Bastian Kauschke okay this is my first step https://github.com/rust-lang/rust/pull/70164

eddyb (Mar 19 2020 at 19:26, on Zulip):

if perf doesn't look too bad, I'll proceed to implement it fully

eddyb (Mar 19 2020 at 19:42, on Zulip):

@Bastian Kauschke I think you could add WellFormedConst today https://github.com/rust-lang/rust/pull/70107#issuecomment-601379924

eddyb (Mar 19 2020 at 19:42, on Zulip):

it would suck a bit but not as much as I thought originally

eddyb (Mar 19 2020 at 19:42, on Zulip):

you'd just need another entry-point in wf.rs that takes a ty::Const and calls compute_const

Bastian Kauschke (Mar 19 2020 at 19:48, on Zulip):

will try

Bastian Kauschke (Mar 19 2020 at 20:02, on Zulip):

am i right that calling shallow_resolve on a ConstInfer would not not do anything?

Am kind of stuck on what I can and should do with a const inference variable

Bastian Kauschke (Mar 19 2020 at 21:15, on Zulip):
/// Registers an obligation for checking later, during regionck, that the type `ty` must
    /// outlive the region `r`.
    pub fn register_wf_obligation(
        &self,
        ty: Ty<'tcx>,
        span: Span,
        code: traits::ObligationCauseCode<'tcx>,
    ) {
        // WF obligations never themselves fail, so no real need to give a detailed cause:
        let cause = traits::ObligationCause::new(span, self.body_id, code);
        self.register_predicate(traits::Obligation::new(
            cause,
            self.param_env,
            ty::Predicate::WellFormed(ty),
        ));
    }

Is this a copy paster error?

eddyb (Mar 19 2020 at 21:52, on Zulip):

Bastian Kauschke said:

am i right that calling shallow_resolve on a InferConst would not not do anything?

Am kind of stuck on what I can and should do with a const inference variable

no, it would resolve the inference variable if it's since been unified

eddyb (Mar 19 2020 at 21:53, on Zulip):

and what's a copy-paste error?

eddyb (Mar 19 2020 at 21:54, on Zulip):

oh, the comment?

eddyb (Mar 19 2020 at 21:54, on Zulip):

linking the lines on github would be clearer btw, without losing context https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/mod.rs#L3324-L3325

eddyb (Mar 19 2020 at 21:54, on Zulip):

and yeah it does look like that lmao

Bastian Kauschke (Mar 19 2020 at 21:59, on Zulip):

linking the lines on github would be clearer btw, without losing context https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/mod.rs#L3324-L3325

will do! First working version is now on github. Will probably still need some cleanup/clarification!!!11

eddyb (Mar 19 2020 at 22:03, on Zulip):

forgot to mention this comment, I hope you saw it https://github.com/rust-lang/rust/pull/70107#issuecomment-601434764

eddyb (Mar 19 2020 at 22:03, on Zulip):

anyway using the blame function, found this: https://github.com/rust-lang/rust/commit/8d98877112d4deb3786de521457418757e010c69#diff-1d1b0d29a2e8da97c6bfb6e364d920c7R1672-R1673

eddyb (Mar 19 2020 at 22:04, on Zulip):

looks like the comment was wrong from the start (cc @nikomatsakis)

eddyb (Mar 19 2020 at 22:04, on Zulip):

and in 5 years, nobody saw

Bastian Kauschke (Mar 19 2020 at 22:06, on Zulip):

and in 5 years, nobody saw

The Biggest Ancient Mysteries That No One Has Ever Been Able To Solve

eddyb (Mar 19 2020 at 23:02, on Zulip):

@Bastian Kauschke just went through all instances of WellFormed( in the codebase and reviewed https://github.com/rust-lang/rust/pull/70107/commits/49b5cdb5785e736a17e0216268bca2c0d1f7934d

eddyb (Mar 19 2020 at 23:05, on Zulip):

can you update the PR description to mention compute_const, wf::const_obligations and Predicate::WellFormedConst?

eddyb (Mar 19 2020 at 23:06, on Zulip):

I changed the PR title, hope you don't mind

varkor (Mar 19 2020 at 23:07, on Zulip):

thanks for tackling this @Bastian Kauschke!

Bastian Kauschke (Mar 19 2020 at 23:08, on Zulip):

can you update the PR description to mention compute_const, wf::const_obligations and Predicate::WellFormedConst?

y

eddyb (Mar 19 2020 at 23:10, on Zulip):

WF is such a stressful part of the compiler (because of how important it is) and the cowboy way the implementation looks to be makes my skin crawl

eddyb (Mar 19 2020 at 23:11, on Zulip):

@Bastian Kauschke you're brave :P

Bastian Kauschke (Mar 19 2020 at 23:11, on Zulip):

or stupid ^^

eddyb (Mar 19 2020 at 23:12, on Zulip):

the size of the PR disagrees :)

Bastian Kauschke (Mar 19 2020 at 23:12, on Zulip):

Considering that WellFormedConst only adds requirements, I at least don't really worry that I cause UB

eddyb (Mar 19 2020 at 23:12, on Zulip):

oh yeah you're only removing bugs :P

eddyb (Mar 19 2020 at 23:13, on Zulip):

I'm just being as strict as I can to make sure we remove all the bugs not just some

Bastian Kauschke (Mar 19 2020 at 23:33, on Zulip):

tbh I really want a good way to test this. I just improved like 5 different things and don't know if and when any of them actually helps :shock:

eddyb (Mar 19 2020 at 23:34, on Zulip):

it's... tricky

eddyb (Mar 19 2020 at 23:34, on Zulip):

@nikomatsakis is pretty good at this but he also doesn't have a lot of free time. I might have to hang on him the fact that this is WF and we need to get it right

eddyb (Mar 19 2020 at 23:58, on Zulip):

@Bastian Kauschke with the changes in https://github.com/rust-lang/rust/pull/70170 it should be much easier to make the entire wf module correct wrt inference variables

Bastian Kauschke (Mar 20 2020 at 09:23, on Zulip):

seems like we are slightly too strict now.

Bastian Kauschke (Mar 20 2020 at 12:22, on Zulip):

@eddyb can you help me out here?

The compiler currently fails in libcore due to ambiguity: https://github.com/rust-lang/rust/blob/4cdfa8a5c162f35058bc53526989741b639a6389/src/libcore/array/iter.rs#L193

The error gets reported here: https://github.com/rust-lang/rust/pull/70107/files#r395598086

and was introduced in these changes: https://github.com/rust-lang/rust/pull/70107/files/49b5cdb5785e736a17e0216268bca2c0d1f7934d..bba3a8faa43f7664fe2f36ef61904dca147982ad
Do you have an idea why this fails?
I don't really make any progress here atm, it is probably caused by wf::const_obligations not always causing a ProcessResult::Changed but that seems like it should be the correct behavior.

Bastian Kauschke (Mar 20 2020 at 13:08, on Zulip):

bug was introduced in https://github.com/rust-lang/rust/pull/70107/files/49b5cdb5785e736a17e0216268bca2c0d1f7934d..81ee6613311bf2ad5970b43f02cf718bad65a03b

99 % certain that its caused by

ty::Predicate::WellFormedConst(constant) => match wf::const_obligations(
                self.selcx.infcx(),
                obligation.param_env,
                obligation.cause.body_id,
                constant,
                obligation.cause.span,
            ) {
                Ok(predicates) => ProcessResult::Changed(mk_pending(predicates)),
                Err(wf::NoProgress) => ProcessResult::Unchanged,
            },
Bastian Kauschke (Mar 20 2020 at 13:21, on Zulip):

commit: https://github.com/rust-lang/rust/pull/70107/commits/81ee6613311bf2ad5970b43f02cf718bad65a03b

eddyb (Mar 20 2020 at 14:02, on Zulip):

@Bastian Kauschke I opened an issue https://github.com/rust-lang/rust/issues/70180

Bastian Kauschke (Mar 20 2020 at 14:10, on Zulip):

I found a different error in my diffs. which should be the reason why core failed to compile. https://github.com/rust-lang/rust/issues/70180 is probably still relevant for const WF though. :sweat_smile:

eddyb (Mar 20 2020 at 14:11, on Zulip):

basically if you've had to adapt stalled_on code to be different for consts than for types, it was likely wrong :P

eddyb (Mar 20 2020 at 14:11, on Zulip):

it should be very symmetrical, as inference variables of each can equally stall solving obligations

Bastian Kauschke (Mar 20 2020 at 14:19, on Zulip):

My fix works! :happy: I didn't touch stalled_on at all, so there might be some snippets which cannot be resolved!

eddyb (Mar 20 2020 at 14:22, on Zulip):

and so it begins https://github.com/rust-lang/rust/pull/70181#issuecomment-601726100

eddyb (Mar 20 2020 at 14:23, on Zulip):

will probably spend a day on just perf testing

Bastian Kauschke (Mar 20 2020 at 14:23, on Zulip):

ok

warning: the feature `const_generics` is incomplete and may cause the compiler to crash
  --> $DIR/cannot-infer-const-args.rs:1:12
   |
LL | #![feature(const_generics)]
   |            ^^^^^^^^^^^^^^
   |
   = note: `#[warn(incomplete_features)]` on by default

error[E0282]: type annotations needed
  --> $DIR/cannot-infer-const-args.rs:9:5
   |
LL |     foo();
   |     ^^^ cannot infer type for type `usize`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0282`.
bjorn3 (Mar 20 2020 at 14:24, on Zulip):

cannot infer type for type :laughter_tears:

Bastian Kauschke (Mar 20 2020 at 14:24, on Zulip):

big brain move

Bastian Kauschke (Mar 20 2020 at 14:27, on Zulip):

@eddyb https://github.com/rust-lang/rust/pull/70107 should once again be ready for review. All ui tests pass

eddyb (Mar 20 2020 at 14:48, on Zulip):

@Bastian Kauschke are you okay with me blocking it on fixing walk and stalled_on?

eddyb (Mar 20 2020 at 14:48, on Zulip):

I want to make sure your PR can do the correct thing

Bastian Kauschke (Mar 20 2020 at 15:02, on Zulip):

are you okay with me blocking it on fixing walk and stalled_on?

Yeah, as long as I have something else to work on in the meantime :thinking:

eddyb (Mar 20 2020 at 15:03, on Zulip):

that I don't know about

eddyb (Mar 20 2020 at 15:03, on Zulip):

heck I don't know what to do either, while I'm waiting for perf results

eddyb (Mar 20 2020 at 15:03, on Zulip):

if they're bad I'd have to redo my approach altogether

Bastian Kauschke (Mar 20 2020 at 15:20, on Zulip):

@eddyb I believe that this should compile: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=74462bd2658e83e8cfeb6c2bf80d47d9 (minimized version of https://github.com/rust-lang/rust/issues/70167)

eddyb (Mar 20 2020 at 15:21, on Zulip):

lmao

eddyb (Mar 20 2020 at 15:21, on Zulip):

also you have to use round parens, square brackets will break links

Bastian Kauschke (Mar 20 2020 at 15:23, on Zulip):

will just work on this then :shrug:

eddyb (Mar 20 2020 at 15:24, on Zulip):

from glancing at it, I'm guessing rustc_typeck::astconv is to blame here

Bastian Kauschke (Mar 20 2020 at 17:14, on Zulip):

ast_ty_to_ty seems fine afaict, will look backwards from where the error originated. Seems like a good oportunity to familiarize myself with the compiler :laughing:

Bastian Kauschke (Mar 20 2020 at 19:45, on Zulip):

where are the mir fns for const params created?

Bastian Kauschke (Mar 20 2020 at 21:19, on Zulip):

afaict type check does not emit an error but returns ty::Error. @eddyb any tips on checking where this might happen?

mark-i-m (Mar 21 2020 at 02:34, on Zulip):

@Bastian Kauschke if that's true, the there is a compiler bug. ty::Error should never be returned unless an error is either emitted or queued.

eddyb (Mar 21 2020 at 02:49, on Zulip):

@Bastian Kauschke damn I was right to not keep working on that branch: https://github.com/rust-lang/rust/pull/70181#issuecomment-601982865

eddyb (Mar 21 2020 at 08:11, on Zulip):

this is the (hopefully faster) second take https://github.com/rust-lang/rust/pull/70213

Bastian Kauschke (Mar 21 2020 at 08:34, on Zulip):

@mark-i-m well it is... I just don't know where

eddyb (Mar 21 2020 at 08:48, on Zulip):

presumably the thing that computes the type of an AnonConst?

eddyb (Mar 21 2020 at 08:49, on Zulip):

AAAAAAAH https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/collect/type_of.rs#L199

eddyb (Mar 21 2020 at 08:49, on Zulip):

how does this not trigger? https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/collect/type_of.rs#L297-L303

eddyb (Mar 21 2020 at 08:50, on Zulip):

wait no it's handled? https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/collect/type_of.rs#L217

eddyb (Mar 21 2020 at 08:51, on Zulip):

oooh it's a QPath

eddyb (Mar 21 2020 at 08:57, on Zulip):

@Bastian Kauschke I think I found it https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/collect/type_of.rs#L285-L287

Bastian Kauschke (Mar 21 2020 at 08:59, on Zulip):

well, even if it is not, it might be a good idea to add a delay span bug! Thanks, will look into it

eddyb (Mar 21 2020 at 08:59, on Zulip):

https://github.com/rust-lang/rust/issues/70167#issuecomment-602016086

eddyb (Mar 21 2020 at 08:59, on Zulip):

yeah, delay_span_bug! there would be good, and I also mentioned a potential fix

eddyb (Mar 21 2020 at 09:04, on Zulip):

https://github.com/rust-lang/rust/issues/70167#issuecomment-602016539

Bastian Kauschke (Mar 21 2020 at 09:12, on Zulip):

jup, the delay span_bug is triggered in allr relevant tests. You're a beast!

eddyb (Mar 21 2020 at 09:19, on Zulip):

note that my method example triggers an existing delay_span_bug already

Bastian Kauschke (Mar 21 2020 at 09:22, on Zulip):

is hir pretty print of AnonConst broken? Struct<>::method::<>

eddyb (Mar 21 2020 at 09:22, on Zulip):

likely yeah

eddyb (Mar 21 2020 at 09:23, on Zulip):

it's possible the parameters are injected with no name

Bastian Kauschke (Mar 21 2020 at 09:35, on Zulip):

do I have to worry about AssocOpaqueTy as well?

eddyb (Mar 21 2020 at 09:36, on Zulip):

uhhh maybe not? there are two bug fixes btw

eddyb (Mar 21 2020 at 09:37, on Zulip):

either special-casing associated types, or using the segment Res

eddyb (Mar 21 2020 at 09:37, on Zulip):

the latter means you don't need the former

eddyb (Mar 21 2020 at 09:37, on Zulip):

(if it works)

Bastian Kauschke (Mar 21 2020 at 09:37, on Zulip):

segment Res seems cleaner I guess. So I might just try this and fix the fallout

eddyb (Mar 21 2020 at 09:38, on Zulip):

it should be a very local change

eddyb (Mar 21 2020 at 09:39, on Zulip):

basically get the right Res at the same time you're getting the index inside the segment

Bastian Kauschke (Mar 21 2020 at 09:44, on Zulip):

are all path segments expected to have a res, considering that it's Option<Res>?https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/struct.PathSegment.html

eddyb (Mar 21 2020 at 09:47, on Zulip):

that I'm not sure about

eddyb (Mar 21 2020 at 09:51, on Zulip):

@Bastian Kauschke wait I'm an idiot, the method example can't be solved for a while, so you should go with the associated type special-casing

eddyb (Mar 21 2020 at 09:51, on Zulip):

or maybe use segment_res.unwrap_or(path.res)

eddyb (Mar 21 2020 at 09:53, on Zulip):

ooh, I see, the M in the method example hits QPath::TypeRelative

eddyb (Mar 21 2020 at 09:59, on Zulip):

@Bastian Kauschke edited https://github.com/rust-lang/rust/issues/70167#issuecomment-602016539

Bastian Kauschke (Mar 21 2020 at 10:09, on Zulip):

fixed the issue! :laughing: even if I don't quite know what I am doing. Should I open a new issue for the method call?

eddyb (Mar 21 2020 at 10:12, on Zulip):

I guess?

Bastian Kauschke (Mar 21 2020 at 10:26, on Zulip):

nm, breaking core again

Bastian Kauschke (Mar 21 2020 at 12:08, on Zulip):

fffff, directly using Res of path segments doesnt work, As the res of path segments in types are Some(Res::Err).

The currently fix ended up being exactly 1 line :cry: That's kinda disappointing

Bastian Kauschke (Mar 21 2020 at 12:24, on Zulip):

@eddyb https://github.com/rust-lang/rust/pull/70223

eddyb (Mar 22 2020 at 21:03, on Zulip):

wheee I finally reached the last walk change, in fulfill

eddyb (Mar 23 2020 at 05:23, on Zulip):

@Bastian Kauschke okay https://github.com/rust-lang/rust/pull/70164 should be done

eddyb (Mar 23 2020 at 20:01, on Zulip):

@Bastian Kauschke https://github.com/rust-lang/rust/pull/70213#issuecomment-602816331

eddyb (Mar 23 2020 at 20:01, on Zulip):

you can rebase over my PR if you want btw

eddyb (Mar 23 2020 at 20:02, on Zulip):

it might unbreak some things in your PR (I forget if there are any tests that still don't work)

eddyb (Mar 23 2020 at 20:02, on Zulip):

even without my walk changes, it would let you handle WellFormedConst like WellFormed in terms of stalled_on

Bastian Kauschke (Mar 25 2020 at 12:00, on Zulip):

@eddyb I looked a bit more into src/test/ui/consts/associated_const_generic.rs of https://github.com/rust-lang/rust/pull/70107.

As far as I can tell WellFormedConst adds a ConstEvaluable predicate for B::Value, which calls the query const_eval_resolve ->ty::Instance::resolve ->s resolve_instance -> resolve_associated_item.

B::Value has a VtableImpl and then returns none as param_env.reveal is Reveal::UserFacing, which causes the function to return None here. At least in my understanding everything up to the branch on param_env.reveal == traits::Reveal::All seems correct.

Looking at the comment for Reveal: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/traits/enum.Reveal.html

However, we could allow projections in fully-monomorphic cases. We choose not to, because we prefer for default type to force the type definition to be treated abstractly by any consumers of the impl.

This seems like to culprit, as the test case is fully monomorphic afaict.

Does my analysis seem correct and if so, how could I proceed?

eddyb (Mar 25 2020 at 15:02, on Zulip):

wait why is it using UserFacing?

eddyb (Mar 25 2020 at 15:05, on Zulip):

hmm, this uses with_reveal_all https://github.com/rust-lang/rust/blob/bfc32dd106f0fe191b8bcd9b76348a8875c30a60/src/librustc/mir/interpret/queries.rs#L21

eddyb (Mar 25 2020 at 15:06, on Zulip):

I wonder if users of const_eval_resolve have to do it themselves or something

eddyb (Mar 25 2020 at 15:08, on Zulip):

this uses with_reveal_all https://github.com/rust-lang/rust/blob/342c5f33d097b2dc07a2dbc0ca45a37379d2ff60/src/librustc/ty/sty.rs#L2401

eddyb (Mar 25 2020 at 15:11, on Zulip):

this is funny given that the ParamEnv matters little apparently https://github.com/rust-lang/rust/blob/e0f5df017368dc3f7cb458fc6d5a5e0420e1d2e5/src/librustc_mir_build/hair/pattern/mod.rs#L768

eddyb (Mar 25 2020 at 15:14, on Zulip):

@Bastian Kauschke okay so you can either add .with_reveal_all() to these files:

or just here: https://github.com/rust-lang/rust/blob/bfc32dd106f0fe191b8bcd9b76348a8875c30a60/src/librustc/mir/interpret/queries.rs#L41

cc @oli @Wesley Wiser @varkor I really don't know if anything at the value levels respects Reveal::UserFacing at all, since we seem to leave that for types (associated type defaults and impl Trait)

oli (Mar 25 2020 at 15:14, on Zulip):

probably not

oli (Mar 25 2020 at 15:15, on Zulip):

I never took any care to handle Reveal::UserFacing, because I didn't understand it when it was relevant

eddyb (Mar 25 2020 at 15:15, on Zulip):

then we can change const_eval_resolve I suppose

eddyb (Mar 25 2020 at 15:20, on Zulip):

(and remove .with_reveal_all() from callsites)

Bastian Kauschke (Mar 25 2020 at 15:23, on Zulip):

Currently testing with updated const_eval_resolve.

When is Reveal::UserFacing needed then, only for default assoc types?

eddyb (Mar 25 2020 at 15:23, on Zulip):

I think so? we might want to clue in @nikomatsakis just in case this is a mistake :P

eddyb (Mar 25 2020 at 15:27, on Zulip):

oli said:

I never took any care to handle Reveal::UserFacing, because I didn't understand it when it was relevant

FWIW, the old consteval ignored it completely, so you're not exactly in the wrong here I don't think, and the associated type default reasoning doesn't make much sense to me

eddyb (Mar 25 2020 at 15:35, on Zulip):

so the only jobs of Reveal::UserFacing, if we ignore associated defaults, would be:

  1. keeping "global predicates" (i.e. that don't depend on any type parameters and may look redundant. wait but why would Reveal::All hide those? that sounds like a recipe for ICEs in miri/layout! cc @nikomatsakis)
  2. hide ty::Opaque aka -> impl Trait
eddyb (Mar 25 2020 at 17:17, on Zulip):

that reminded me I should try to do this :D #70398

nikomatsakis (Mar 25 2020 at 20:00, on Zulip):

OK so I've gotten a bunch of pings rom this topic but i've not been able to keep up with it :)

nikomatsakis (Mar 25 2020 at 20:00, on Zulip):

is there a TL;DR

nikomatsakis (Mar 25 2020 at 20:01, on Zulip):

eddyb said:

so the only jobs of Reveal::UserFacing, if we ignore associated defaults, would be:

  1. keeping "global predicates" (i.e. that don't depend on any type parameters and may look redundant. wait but why would Reveal::All hide those? that sounds like a recipe for ICEs in miri/layout! cc nikomatsakis)
  2. hide ty::Opaque aka -> impl Trait

@eddyb is this the TL;DR?

eddyb (Mar 25 2020 at 20:01, on Zulip):

yeah I guess

eddyb (Mar 25 2020 at 20:02, on Zulip):

at lease regarding ParamEnv

eddyb (Mar 25 2020 at 20:04, on Zulip):

@nikomatsakis oh well also the fact that we likely don't want associated const defaults to be treated like associated types in terms of not normalizing during typeck

eddyb (Mar 25 2020 at 20:04, on Zulip):

but also I guess I pinged you on this :P https://github.com/rust-lang/rust/commit/8d98877112d4deb3786de521457418757e010c69#diff-1d1b0d29a2e8da97c6bfb6e364d920c7R1672-R1673

nikomatsakis (Mar 25 2020 at 20:42, on Zulip):

I'm feeling a bit confused :)

nikomatsakis (Mar 25 2020 at 20:42, on Zulip):

I guess I need to read a bit more of the backscroll

nikomatsakis (Mar 25 2020 at 20:43, on Zulip):

In particular, I think I never imagined that reveal-all would have any use besides specialization (associated ty defaults) and impl trait, but maybe I've forgotten something?

nikomatsakis (Mar 25 2020 at 20:43, on Zulip):

I'm not quite sure what you mean about global predicates, in particular

eddyb (Mar 25 2020 at 21:38, on Zulip):

@nikomatsakis UserFacing hides associated type defaults even when we know the entire type so we could just expose the type if we wanted to

eddyb (Mar 25 2020 at 21:38, on Zulip):

we don't anything like this for values, only types

eddyb (Mar 25 2020 at 21:39, on Zulip):

@nikomatsakis the global thing is https://github.com/rust-lang/rust/blob/cdb50c6f2507319f29104a25765bfb79ad53395c/src/librustc/ty/mod.rs#L1840-L1844

centril (Mar 26 2020 at 04:51, on Zulip):

eddyb said:

nikomatsakis oh well also the fact that we likely don't want associated const defaults to be treated like associated types in terms of not normalizing during typeck

Huh? Associated const defaults shouldn't be revealed either in trait defs, e.g. this should result in a unification failure:

trait Foo {
    const SIZE: usize = 0;
    fn foo(x: [u8; Self::SIZE]) {
        let _y: [u8; 0] = x; //~ ERROR mismatched types
    }
}
centril (Mar 26 2020 at 04:51, on Zulip):

This is specced in the associated type defaults RFC

eddyb (Mar 26 2020 at 06:09, on Zulip):

@centril what I'm talking about is not about trait defs, which have Self in there so clearly it's not known

centril (Mar 26 2020 at 06:09, on Zulip):

@eddyb ah; do you have an example in mind wrt. the diff?

eddyb (Mar 26 2020 at 06:10, on Zulip):

@centril if you have <ConcreteType as Trait>::AssociatedType, type-check won't let you see that if it finds an impl that doesn't specify it and relies on the trait default, IIRC. or maybe it's just with specialization

eddyb (Mar 26 2020 at 06:10, on Zulip):

I'll just go make an example

eddyb (Mar 26 2020 at 06:11, on Zulip):

@centril https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8f677687fc5cb72cd2e622047ceb99a6

eddyb (Mar 26 2020 at 06:11, on Zulip):

see the second error

eddyb (Mar 26 2020 at 06:12, on Zulip):

wait what it's gone?! https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=268803fb5872cd3803025cd27940afab

eddyb (Mar 26 2020 at 06:12, on Zulip):

but I could've sworn I've seen that code recently and it looked the same

eddyb (Mar 26 2020 at 06:13, on Zulip):

ah, this changed https://github.com/rust-lang/rust/blob/master/src/librustc_trait_selection/traits/project.rs#L1035

centril (Mar 26 2020 at 06:14, on Zulip):

@eddyb https://github.com/rust-lang/rust/pull/64564 or https://github.com/rust-lang/rust/pull/61812 probably changed this, per RFC

centril (Mar 26 2020 at 06:14, on Zulip):

likely the latter

eddyb (Mar 26 2020 at 06:14, on Zulip):

it only changed traits specifically, specialization is still affected

centril (Mar 26 2020 at 06:14, on Zulip):

while you're at it... you can always finish that RFCs impl :D

eddyb (Mar 26 2020 at 06:15, on Zulip):

@centril @nikomatsakis https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=d7ad891485b6404da0520f9ddcd50554

eddyb (Mar 26 2020 at 06:15, on Zulip):

we don't do this for associated consts

centril (Mar 26 2020 at 06:16, on Zulip):

that seems like a bug; <() as Trait>::Assoc has a ground term in Self, so Assoc must be fully known here

eddyb (Mar 26 2020 at 06:17, on Zulip):

no this has been intentional all along

eddyb (Mar 26 2020 at 06:17, on Zulip):

we've relaxed it for defaults coming from the trait itself more recently

eddyb (Mar 26 2020 at 06:17, on Zulip):

but there's long comments in that file about this :P

centril (Mar 26 2020 at 06:18, on Zulip):

wait what; why is this intentional?

eddyb (Mar 26 2020 at 06:18, on Zulip):

ask @nikomatsakis

eddyb (Mar 26 2020 at 06:19, on Zulip):

uh oh https://github.com/rust-lang/rust/commit/a323ff2c864801fdc8e044e88f11efb49a565ed1#diff-b50d9bf9e9d008c91a87602713a20333L1057-R1057

eddyb (Mar 26 2020 at 06:19, on Zulip):

@Jonas Schievink ^^ there's a comment above your change there, and probably elsewhere in the file too

eddyb (Mar 26 2020 at 06:20, on Zulip):

I'm not sure they're 100% correct anymore

eddyb (Mar 26 2020 at 06:22, on Zulip):

@centril https://github.com/rust-lang/rust/blob/master/src/librustc/traits/mod.rs#L37-L56

centril (Mar 26 2020 at 06:24, on Zulip):

@eddyb yea but your playground has a crucial difference: impl Trait for () {}, which locks in the associated type, making it not further specializable

eddyb (Mar 26 2020 at 06:24, on Zulip):

read what I just linked?

centril (Mar 26 2020 at 06:24, on Zulip):

I did yes

eddyb (Mar 26 2020 at 06:25, on Zulip):

"However, we could allow projections in fully-monomorphic cases. We choose not to,"

eddyb (Mar 26 2020 at 06:25, on Zulip):

my example is a "fully monomorphic case"

centril (Mar 26 2020 at 06:25, on Zulip):

Yes, I'm skeptical that comment is the right thing in the case of impl Trait for () {}

centril (Mar 26 2020 at 06:25, on Zulip):

at least

eddyb (Mar 26 2020 at 06:25, on Zulip):

oh their example is missing that haha

eddyb (Mar 26 2020 at 06:26, on Zulip):

that impl doesn't actually matter for the specialization example, it's a left-over from the trait example

eddyb (Mar 26 2020 at 06:26, on Zulip):

specializing empty impls do nothing

eddyb (Mar 26 2020 at 06:26, on Zulip):

it's only relevant if it actually overrides the default itself https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=c9cef05e1013f9a04142578724cbf53d

Bastian Kauschke (May 02 2020 at 13:45, on Zulip):

@eddyb I heard that you have a lot of free time right now. :heart:

As I feared that you might not know what to do in the following weeks I rebased and improved
https://github.com/rust-lang/rust/pull/70107, meaning that it should be ready for review again :sparkles:

Bastian Kauschke (May 02 2020 at 14:30, on Zulip):

Nevermind, it's not... Forgot to implement a previously mentioned change :sweat_smile:

Bastian Kauschke (May 02 2020 at 16:19, on Zulip):

Ok, I now generalized compute and removed compute_const. Now its actually ready

lcnr (May 27 2020 at 08:06, on Zulip):

Can someone start another perf run for https://github.com/rust-lang/rust/pull/70107?

nikomatsakis (May 27 2020 at 14:09, on Zulip):

(looks like it was done)

Last update: May 29 2020 at 18:00UTC