Stream: t-compiler

Topic: lazy normalization example


Bastian Kauschke (Apr 15 2020 at 09:29, on Zulip):
// check-pass
#![feature(const_generics)]
//~^ WARN the feature `const_generics` is incomplete and may cause the compiler to crash

struct A<const N: usize>(usize);

trait Foo<T> {
    fn foo<const N: usize>(&self) -> [T; N];
}

impl<T> Foo<T> for T {
    fn foo<const N: usize>(&self) -> [T; N] {
        todo!()
    }
}


fn main() {
    ().foo::<{ 2 + 3 }>();
}

Does that need lazy nomalization?

Bastian Kauschke (Apr 15 2020 at 09:30, on Zulip):

or rather, should it? It doesn't work at all rn as type dependent const args aren't supported yet

eddyb (Apr 15 2020 at 12:44, on Zulip):

no

eddyb (Apr 15 2020 at 12:44, on Zulip):

lazy normalization is mostly about ParamEnvs

eddyb (Apr 15 2020 at 12:44, on Zulip):

and the constant being in its own tcx.param_env(def_id)

eddyb (Apr 15 2020 at 12:45, on Zulip):

so that would require {2+3} to be written in main's where clauses, I think?

eddyb (Apr 15 2020 at 12:46, on Zulip):

the process here, with the new plan, is that ().foo(...) is resolved to Foo::foo (the impl doesn't matter)

eddyb (Apr 15 2020 at 12:47, on Zulip):

then the DefId of the const N in the trait will be placed in ConstKind::Unevaluated

eddyb (Apr 15 2020 at 12:48, on Zulip):

and from there it propagates through const_eval -> miri -> MIR building -> typeck_tables_of

Bastian Kauschke (Apr 15 2020 at 17:16, on Zulip):

then the DefId of the const N in the trait will be placed in ConstKind::Unevaluated

This doesn't quite work, as it breaks diagnostics. and causes errors when trying to generate mir.

error[E0080]: evaluation of constant value failed
  --> /home/programming/rust/src/test/ui/const-generics/slice-const-param.rs:10:34
   |
LL | pub fn function_with_bytes<const BYTES: &'static [u8]>() -> &'static [u8] {
   |                                  ^^^^^ could not load MIR for DefId(0:6 ~ slice_const_param[317d]::function_with_bytes[0]::BYTES[0])

I am currently working on a slightly different approach which looks at least somewhat promising :laughing:

eddyb (Apr 15 2020 at 17:18, on Zulip):

what do you mean?

eddyb (Apr 15 2020 at 17:19, on Zulip):

there is no other way AFAIK

Bastian Kauschke (Apr 15 2020 at 17:29, on Zulip):

there is no other way AFAIK

I avoid calling type_of for consts. Not sure if it will work but it's a great way to familiarize myself with the compiler :thinking:
tbh it's a bodge on top of a bodge rn, so I still have a lot of cleanup to do

eddyb (Apr 15 2020 at 17:31, on Zulip):

you have to pass that data down

eddyb (Apr 15 2020 at 17:31, on Zulip):

through miri, through all the MIR stuff, through MIR building, through typeck

Bastian Kauschke (Apr 15 2020 at 17:32, on Zulip):

through miri, through all the MIR stuff, through MIR building, through typeck

aye

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

@varkor I currently emit errors twice. When const evaluating const args,

I can either call typeck_tables_of here, causing cycle errors,
or typeck_tables_of_const_arg(a varsion of typeck_tables_of which correctly handles const args), which emits the same errors as typeck_tables_of without "caching" its result for typeck_tables_of. Causing a later call to typeck_tables_of to emit the same errors once more.
Any idea on how I might solve this?

https://github.com/rust-lang/rust/blob/835428c35d785733e72bfbf32fc2f8fff3e50e63/src/librustc_mir/const_eval/eval_queries.rs#L297

eddyb (Apr 15 2020 at 20:34, on Zulip):

my suggestion was to have typeck_tables_offor const args look up the right DefId and then use the right typeck_tables_of_const_arg invocation

eddyb (Apr 15 2020 at 20:34, on Zulip):

so that it's only ever type-checked once

Bastian Kauschke (Apr 16 2020 at 16:48, on Zulip):

This doesn't quite work, as it breaks diagnostics. and causes errors when trying to generate mir.

We can add an Option<LocalDefId>to ConstKind::Unevaluated without changing its size.
Considering that we only need to typeck local methods, we can store the DefId of the arg as a LocalDefId which we now use for diagnostics and in places where using the DefId of the param fails.

@eddyb Is there a danger of breaking incremental/cross crate queries with this?

eddyb (Apr 16 2020 at 16:49, on Zulip):

don't be worried about increasing the size

eddyb (Apr 16 2020 at 16:49, on Zulip):

and the called method could be in another crate, so LocalDefId is not appropriate

Bastian Kauschke (Apr 16 2020 at 16:51, on Zulip):

and the called method could be in another crate, so LocalDefId is not appropriate

I store the used argument as a localdefid, shouldn't that one be local?

eddyb (Apr 16 2020 at 17:01, on Zulip):

@Bastian Kauschke but that's already there

eddyb (Apr 16 2020 at 17:01, on Zulip):

it's the DefId

eddyb (Apr 16 2020 at 17:01, on Zulip):

or do you mean you're doing something super sneaky?

eddyb (Apr 16 2020 at 17:02, on Zulip):

where you're storing the param def_id in the DefId

eddyb (Apr 16 2020 at 17:02, on Zulip):

and the true DefId in the LocalDefId

Bastian Kauschke (Apr 16 2020 at 17:02, on Zulip):

where you're storing the param def_id in the DefId

yes

eddyb (Apr 16 2020 at 17:02, on Zulip):

but even then that's not correct because we need to serialize unevaluated constants cross-crate anyway

Bastian Kauschke (Apr 16 2020 at 17:03, on Zulip):

........ why? :shock:

eddyb (Apr 16 2020 at 17:03, on Zulip):

because they can be in a type

eddyb (Apr 16 2020 at 17:03, on Zulip):

there's no reason to think anything will force their normalization

eddyb (Apr 16 2020 at 17:04, on Zulip):

just make Unevaluated larger :P

Bastian Kauschke (Apr 16 2020 at 17:04, on Zulip):

hmm, I could resubstitute the DefId using the LocalDefId during serialization :laughing: I doubt this would ever surprise someone

eddyb (Apr 16 2020 at 17:05, on Zulip):

you'll just waste your own time if you don't do this the easy way

Bastian Kauschke (Apr 16 2020 at 17:05, on Zulip):

just make Unevaluated larger :P

PREMATURE OPTIMIZATION

Bastian Kauschke (Apr 16 2020 at 18:40, on Zulip):

just make Unevaluated larger :P

We can actually use DefId and still not change the size :laughing:

eddyb (Apr 16 2020 at 18:40, on Zulip):

please I beg you

eddyb (Apr 16 2020 at 18:41, on Zulip):

first get it working, then worry about size penalties (if they even matter)

Bastian Kauschke (Apr 16 2020 at 18:54, on Zulip):

I do :laughing: Was just suprised that usng a DefId didn't actually break the static assert

eddyb (Apr 16 2020 at 18:54, on Zulip):

oh you mean adding a DefId doesn't change the size?

Bastian Kauschke (Apr 16 2020 at 18:54, on Zulip):

Yes

eddyb (Apr 16 2020 at 18:54, on Zulip):

it's (u32, u32)

eddyb (Apr 16 2020 at 18:55, on Zulip):

so the same size as a pointer

Bastian Kauschke (Apr 16 2020 at 18:55, on Zulip):

jup, I thought it Unevaluated was the biggest variant and 8 byte aligned

eddyb (Apr 16 2020 at 18:55, on Zulip):

one of the ConstValue cases has an u128, I wonder if ConstValue is the largest variant of ConstKind :P

Bastian Kauschke (Apr 16 2020 at 18:57, on Zulip):

yes, we may try to split data into 2 u64

Bastian Kauschke (Apr 16 2020 at 18:57, on Zulip):

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_mir/interpret/enum.Scalar.html

Bastian Kauschke (Apr 16 2020 at 18:57, on Zulip):

Alignment costs so much there

eddyb (Apr 16 2020 at 18:57, on Zulip):

OH I forgot about the u8 haha

eddyb (Apr 16 2020 at 18:58, on Zulip):

although, no, just the fact that Scalar is an enum by itself makes it 24 bytes

eddyb (Apr 16 2020 at 18:59, on Zulip):

that means ConstValue is 32 bytes

eddyb (Apr 16 2020 at 18:59, on Zulip):

which is enough for 2 DefId's, a SubstsRef and an Option<Promoted>

Bastian Kauschke (Apr 16 2020 at 19:01, on Zulip):

afaict it should be possible to get scalar down to 24 bytes with alignment 8. which should allow us to keep ConstKind smaller as well.
Don't know if this is that useful though :shrug:

eddyb (Apr 16 2020 at 19:02, on Zulip):

Scalar is 24 bytes

eddyb (Apr 16 2020 at 19:02, on Zulip):

at least that's what it looks like to me

eddyb (Apr 16 2020 at 19:02, on Zulip):

u128 can't have alignment of 16 I don't think

Bastian Kauschke (Apr 16 2020 at 19:03, on Zulip):

it actually isn't. It's 8

Bastian Kauschke (Apr 16 2020 at 19:03, on Zulip):

mb, remembered it wrong

eddyb (Apr 16 2020 at 19:03, on Zulip):

ConstValue isn't and maybe could be 24 bytes

eddyb (Apr 16 2020 at 19:03, on Zulip):

ah I see

Bastian Kauschke (Apr 16 2020 at 19:04, on Zulip):

ConstValue with size 24 is possible

Bastian Kauschke (Apr 16 2020 at 19:11, on Zulip):

How relevant is the size of Consts?

eddyb (Apr 16 2020 at 19:12, on Zulip):

idk, ask @Nicholas Nethercote

Nicholas Nethercote (Apr 16 2020 at 22:32, on Zulip):

For all the types I've encountered where the size matters, I have added a static_assert_size! and a comment about it, like this: https://github.com/rust-lang/rust/blob/3712e11a828af2eea273a3e7300115e65833fbc5/src/librustc_infer/traits/mod.rs#L59-L61

Nicholas Nethercote (Apr 16 2020 at 22:32, on Zulip):

Both Scalar and ConstValue have static_assert_size!, but they lack that comment, which indicates that I didn't add the assertions.

Nicholas Nethercote (Apr 16 2020 at 22:34, on Zulip):

git blame will be useful here to work out who added the assertions and why

bjorn3 (Apr 16 2020 at 22:38, on Zulip):

https://github.com/rust-lang/rust/commit/2a1748834e80b2461be4e18d420503d60e687312

Bastian Kauschke (Apr 17 2020 at 11:43, on Zulip):

Big progress :sparkles: Now fails with a 460 stackframes deep backtrace

Bastian Kauschke (Apr 17 2020 at 13:16, on Zulip):

f thread 'rustc' panicked at 'attempt to read from stolen value', /home/programming/rust/src/librustc_middle/ty/steal.rs:42:9

Bastian Kauschke (Apr 17 2020 at 13:58, on Zulip):

What is the intended use of https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast/ast/enum.TyKind.html#variant.Typeof

eddyb (Apr 17 2020 at 14:12, on Zulip):

heh

eddyb (Apr 17 2020 at 14:12, on Zulip):

typeof

eddyb (Apr 17 2020 at 14:12, on Zulip):

the funny thing is it would also be very easy to finish the implementation thereof

eddyb (Apr 17 2020 at 14:12, on Zulip):

but nobody made an RFC AFAIK

eddyb (Apr 17 2020 at 14:12, on Zulip):

maybe we should make it error, printing the type

Bastian Kauschke (Apr 17 2020 at 14:16, on Zulip):

This? https://github.com/rust-lang/rust/issues/3228

eddyb (Apr 17 2020 at 14:16, on Zulip):

lmao I don't remember seeing that

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

wouldn't that always be kind of a mess due to variance?

eddyb (Apr 17 2020 at 14:22, on Zulip):

funny you should say that

eddyb (Apr 17 2020 at 14:22, on Zulip):

@nikomatsakis raised a similar point recently :P

eddyb (Apr 17 2020 at 14:23, on Zulip):

although no, not variance, outlives

Bastian Kauschke (Apr 17 2020 at 16:43, on Zulip):

@varkor How can I reliably get the relevant Generics for a const argument. (don't care about potential cycle errors there)

I currently try tcx.generics_of(tcx.hir().local_def_id(tcx.hir().get_parent_node(tcx.hir().as_local_hir_id(def_id).unwrap())))
But some parent nodes don't have a associated DefId, which causes local_def_id to panic

eddyb (Apr 17 2020 at 16:45, on Zulip):

@Bastian Kauschke literally just tcx.generics_of(def_id)

eddyb (Apr 17 2020 at 16:46, on Zulip):

assuming the def_id is of an AnonConst

Bastian Kauschke (Apr 17 2020 at 16:46, on Zulip):

hhhhhhhhhhhhhh

Bastian Kauschke (Apr 17 2020 at 16:46, on Zulip):

ok

varkor (Apr 17 2020 at 16:50, on Zulip):

sorry for always being slow to respond: I don't keep Zulip open and so I only spot messages every so often when I check my phone :P

Bastian Kauschke (Apr 17 2020 at 16:52, on Zulip):

no worries @eddyb Is just too fast :laughing:

Bastian Kauschke (Apr 17 2020 at 17:33, on Zulip):

apparently tcx.generics_of(def_id) doesn't work in all cases. We previously used Res to get a correct DefId.
https://github.com/rust-lang/rust/blob/8d67f576b56e8fc98a31123e5963f8d00e40611c/src/librustc_typeck/collect/type_of.rs#L263-L277

What exactly is the use ofRes here. I worked on this code for a few times and still have no idea tbh :sweat_smile:

eddyb (Apr 17 2020 at 17:34, on Zulip):

@Bastian Kauschke oh that's a completely different thing

Bastian Kauschke (Apr 17 2020 at 17:34, on Zulip):

Why do we have to use the parent_did for DefKind::Ctor

eddyb (Apr 17 2020 at 17:34, on Zulip):

res is the resolution

eddyb (Apr 17 2020 at 17:34, on Zulip):

of the path

eddyb (Apr 17 2020 at 17:34, on Zulip):

so the generics_of is for the thing that takes const generics

eddyb (Apr 17 2020 at 17:35, on Zulip):

the parent for the ctor is because the params are on e.g. on the struct

eddyb (Apr 17 2020 at 17:35, on Zulip):

@Bastian Kauschke earlier I thought you were asking about getting the generics in scope of the anon const expression

eddyb (Apr 17 2020 at 17:35, on Zulip):

not what it's passed to

eddyb (Apr 17 2020 at 17:35, on Zulip):

the latter depends on what you're passing it to

Bastian Kauschke (Apr 17 2020 at 17:36, on Zulip):
let a = foo::<7>();
              ^ I want to get `N` here

fn foo<const N: usize>()
             ^ this `DefId`
eddyb (Apr 17 2020 at 17:36, on Zulip):

every place calling to_const or whatever has to supply its own depending on where the const expression is

eddyb (Apr 17 2020 at 17:37, on Zulip):

right so that requires you to figure out where the const args <-> parameters matching is checked

eddyb (Apr 17 2020 at 17:37, on Zulip):

basically the same place that errors when the number of generic args doesn't match the number of parameters

eddyb (Apr 17 2020 at 17:37, on Zulip):

and likely calls to_const

Bastian Kauschke (Apr 17 2020 at 17:42, on Zulip):

I previously used https://github.com/rust-lang/rust/blob/99c18ab8a98e1b171489cb2e0a06ea5471b9c633/src/librustc_typeck/collect/type_of.rs#L246-L272

The get the type of the param, can this be adjusted to get its DefId?

eddyb (Apr 17 2020 at 17:42, on Zulip):

heh

varkor (Apr 17 2020 at 17:43, on Zulip):

eddyb said:

basically the same place that errors when the number of generic args doesn't match the number of parameters

that's check_generic_arg_count, though I don't think it uses to_const

eddyb (Apr 17 2020 at 17:43, on Zulip):

@Bastian Kauschke sort of, but you need to get the DefId of the thing the segment refers to. type_relative_defs is I think the right table

eddyb (Apr 17 2020 at 17:44, on Zulip):

there's something like qpath_res maybe?

Bastian Kauschke (Apr 17 2020 at 17:45, on Zulip):

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.TypeckTables.html#structfield.type_dependent_defs

Bastian Kauschke (Apr 17 2020 at 17:45, on Zulip):

?

eddyb (Apr 17 2020 at 17:45, on Zulip):

yeah

Bastian Kauschke (Apr 17 2020 at 20:50, on Zulip):

Thanks :heart: I believe I now have a working concept!

I fear the error messages though

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

error[E0391]: cycle detected when type-checking `main`
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:41:1
   |
LL | fn main() {
   | ^^^^^^^^^
   |
note: ...which requires const-evaluating + checking `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires const-evaluating + checking `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires const-evaluating `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires optimizing MIR...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires borrow-checking `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires processing `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires const checking `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires processing `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires unsafety-checking `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires building MIR for...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires type-checking `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
note: ...which requires processing `main::{{constant}}#0`...
  --> /home/programming/rust/src/test/ui/__check/issue-61936.rs:44:47
   |
LL |     for array in v.as_slice().array_windows::<FOUR>() {
   |                                               ^^^^
   = note: ...which again requires type-checking `main`, completing the cycle
   = note: cycle used when type-checking all item bodies
Bastian Kauschke (Apr 18 2020 at 13:43, on Zulip):

A UI test requires the line in the error message and I don't know why https://github.com/rust-lang/rust/blob/86f765faac81d71192fa3df5466beec556b7bb58/src/test/ui/recursion/issue-26548-recursion-via-normalize.rs#L11

varkor (Apr 18 2020 at 13:44, on Zulip):

if we already have cycle errors, another one doesn't seem particularly harmful, as long as this isn't incorrect

Bastian Kauschke (Apr 18 2020 at 13:45, on Zulip):

My problem is that //~ NOTE 11:1: 11:10: cycle used when main[None] is required

Bastian Kauschke (Apr 18 2020 at 13:45, on Zulip):

//~ NOTE cycle used when main[None] does not detect this error

Bastian Kauschke (Apr 18 2020 at 13:46, on Zulip):

stderr seems normal

error[E0391]: cycle detected when computing layout of `std::option::Option<S>`
   |
   = note: ...which requires computing layout of `S`...
   = note: ...which again requires computing layout of `std::option::Option<S>`, completing the cycle
note: cycle used when `main`[None]
  --> $DIR/issue-26548-recursion-via-normalize.rs:11:1
   |
LL | fn main() {
   | ^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0391`.
varkor (Apr 18 2020 at 13:48, on Zulip):

oh, weird

Bastian Kauschke (Apr 18 2020 at 13:51, on Zulip):

nm, it suddently works... did I mess up a whitespace or something? :surrender:

Bastian Kauschke (Apr 18 2020 at 14:12, on Zulip):

https://github.com/rust-lang/rust/blob/dc0823a9a766ee6f72f5a7b013d7cb8288bf7b12/src/librustc_mir/borrow_check/mod.rs#L92-L100

@varkor I currently check in all queries if the DefId is a const arg and then call the same query with the corresponding param DefId.
This causes longer cycles (on potentially valid cycle errors) than necessary though. I could try and move this check to the callsite instead, but I fear that this is even more complex

Bastian Kauschke (Apr 18 2020 at 14:13, on Zulip):

It's worse enough to review this PR as is :unamused:

Bastian Kauschke (Apr 18 2020 at 14:15, on Zulip):

See https://github.com/rust-lang/rust/pull/71154/commits/dc0823a9a766ee6f72f5a7b013d7cb8288bf7b12

varkor (Apr 18 2020 at 14:22, on Zulip):

@Bastian Kauschke: I shall try to take a look soon :)

eddyb (Apr 18 2020 at 14:33, on Zulip):

@varkor btw if the PR isn't marked as Draft, please do

varkor (Apr 18 2020 at 14:33, on Zulip):

it is :+1:

eddyb (Apr 18 2020 at 14:33, on Zulip):

I bet when I finally get to it I will regret taking the time to get to it lol

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

It is still marked as draft, will take a few more hours for me to get through this Screenshot-from-2020-04-18-16-33-33.png

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

I bet when I finally get to it I will regret taking the time to get to it lol

It is horrible

varkor (Apr 18 2020 at 14:39, on Zulip):

lol

varkor (Apr 18 2020 at 14:39, on Zulip):

I love reviewing 1k-line PRs :)

Bastian Kauschke (Apr 18 2020 at 14:43, on Zulip):

crate fn mir_built(tcx: TyCtxt<'_>, (def_id, param_def_id): (DefId, Option<DefId>)) -> &ty::steal::Steal<BodyAndCache<'_>> {
Why doesn't rustfmt split this line :O

varkor (Apr 18 2020 at 14:56, on Zulip):

rustfmt is imperfect

Bastian Kauschke (Apr 18 2020 at 15:10, on Zulip):

Some tips for cleaning up this commit history? Screenshot-from-2020-04-18-17-08-29.png

The PR should now be at least somewhat coherent :laughing:

Bastian Kauschke (Apr 18 2020 at 15:28, on Zulip):

https://github.com/rust-lang/rust/pull/71154 no draft no more

eddyb (Apr 18 2020 at 15:34, on Zulip):

lmao https://github.com/rust-lang/rust/pull/71154#issuecomment-613709634

eddyb (Apr 18 2020 at 15:34, on Zulip):

I will do that again

eddyb (Apr 18 2020 at 15:34, on Zulip):

please keep it "draft" until I can review

Bastian Kauschke (Apr 18 2020 at 15:35, on Zulip):

btw if the PR isn't marked as Draft, please do

you meant if it isn't marked as Draft, please mark it as such?

Bastian Kauschke (Apr 18 2020 at 15:35, on Zulip):

Not "if it isn't marked as Draft, please review it soon"

eddyb (Apr 18 2020 at 15:35, on Zulip):

argh, the former

Bastian Kauschke (Apr 18 2020 at 15:35, on Zulip):

:sweat_smile:

eddyb (Apr 18 2020 at 15:36, on Zulip):

/me shouldn't use terse English constructs

eddyb (Apr 18 2020 at 15:36, on Zulip):

they sound fancy but they're ambiguous for everyone other than the person who wrote them

mark-i-m (Apr 18 2020 at 16:24, on Zulip):

Bastian Kauschke said:

Some tips for cleaning up this commit history? Screenshot-from-2020-04-18-17-08-29.png

The PR should now be at least somewhat coherent :laughing:

You could squash commits together, especially commits that are just random cleanup or fixes (see git rebase -i)

Bastian Kauschke (Apr 18 2020 at 16:38, on Zulip):

:thumbs_up:

Bastian Kauschke (Apr 18 2020 at 16:41, on Zulip):

My main problem is that I probably will end up with on commit containing 500 lines, as I don't know how to split it into selfcontained segments.

I only changes a few lines per file. Is it better or worse to split it for each crate (i.e. change librustc_mir and then in another commit librustc_typeck) even if these individual commits don't compile.

mark-i-m (Apr 18 2020 at 16:49, on Zulip):

@Bastian Kauschke without looking at the PR, it's hard to say. If all the changes are logically the same sort, it might be reasonable to just squash into one big commit. If there are smaller self-contained commits that could be separated out, that could also be good. Also, if eddyb (as the reviewer) has a preference, you should do that.

mark-i-m (Apr 18 2020 at 16:50, on Zulip):

Another trick is to git reset back to the head so all of the changes are unstaged. You can then do git add -p to stage just the parts you want and rebuild your commit history however you want.

Bastian Kauschke (Apr 19 2020 at 10:21, on Zulip):

https://github.com/rust-lang/rust/pull/71154 currently still fails when using multiple crates.

As typeck_tables now doesn't call type_of(const_arg) anymore, other crates ,ay call type_of[more accurately const_param_of) which must not happen, as this query requires the local_hir_id. https://github.com/rust-lang/rust/blob/c94d6b5e1807b6487c676a75e4485f9becda8e2e/src/librustc_typeck/collect/type_of.rs#L24

Is there a way to get the corresponding generic parameter for a const argument of an external crate?

Last update: May 29 2020 at 16:05UTC