Stream: t-compiler

Topic: #58310 substsref


csmoe (Feb 19 2019 at 15:53, on Zulip):

@oli would you mind have a bors retrying?
it's strange that Substs already imported.

csmoe (Feb 19 2019 at 15:54, on Zulip):

https://github.com/rust-lang/rust/pull/58321#issuecomment-465138251

csmoe (Feb 23 2019 at 13:13, on Zulip):

@oli rebased again. may I request a "early" merging as the comment said?

csmoe (Feb 24 2019 at 08:47, on Zulip):

@oli

csmoe (Feb 24 2019 at 16:28, on Zulip):

@oli just to confirm, so you mean we will have type Substs = &'tcx List[Kind] finally?

oli (Feb 24 2019 at 16:29, on Zulip):

@csmoe yes

csmoe (Feb 26 2019 at 16:42, on Zulip):

@oli step 1 done.

varkor (Feb 26 2019 at 20:01, on Zulip):

if this is going to interact with https://github.com/rust-lang/rust/pull/58583 much, I'd appreciate it if the next steps could be delayed until after that PR is merged, because it's going to be troublesome enough to rebase it as it is :)

csmoe (Feb 27 2019 at 00:37, on Zulip):

@varkor okay I'll wait for you

csmoe (Feb 27 2019 at 17:39, on Zulip):

@oli this PR broke the RLS as high-five said, and she requested a PR to rls. but what's the work flow for such kind of fixes?

oli (Feb 27 2019 at 18:15, on Zulip):

Well, create a PR that fixes rls, then a PR that updates the rls submodulr in the rustc repo

oli (Feb 27 2019 at 18:15, on Zulip):

Probably needs a clippy submodule update

csmoe (Mar 13 2019 at 02:02, on Zulip):

@varkor 's PR was merged, so step next.

csmoe (Apr 25 2019 at 15:20, on Zulip):

@oli need help to impl ToStableHashKey for SubstsRef, what's the KeyType should be?

oli (Apr 25 2019 at 15:28, on Zulip):

isn't SubstsRef &'tcx List<...>?

oli (Apr 25 2019 at 15:28, on Zulip):

oh, that would require a HashStable impl for the ...

csmoe (Apr 25 2019 at 15:32, on Zulip):

isn't SubstsRef &'tcx List<...>?

SubstsRef {
   inner: &'tcx List<Kind>,
}
oli (Apr 25 2019 at 15:35, on Zulip):

ah heheh, Yea, so I think you can just forward to the &'tcx List impl. so type KeyType = <&'tcx List<Kind> as ToStableHashKey>::KeyType;

csmoe (Apr 25 2019 at 15:37, on Zulip):

@oli and during the refactoring trip, I saw some usages of &[Kind], should them be replaced with SubstsRef too?

oli (Apr 25 2019 at 15:37, on Zulip):

hmm... not sure, do you have a link to an example?

csmoe (Apr 25 2019 at 15:38, on Zulip):

pasted image

oli (Apr 25 2019 at 15:40, on Zulip):

hmm... can you try modifying them and see which callers exist? It seems to me like SubstsRef is indeed right here, but the callers may disagree

csmoe (Apr 25 2019 at 15:41, on Zulip):

@oli yep, I tried to replace them with SubstsRef. but stuck by how to convert [Kind] to List<Kind>.

csmoe (Apr 25 2019 at 15:42, on Zulip):

https://github.com/rust-lang/rust/pull/59312/files#r267297677

oli (Apr 25 2019 at 15:42, on Zulip):

where's the [Kind] from? we need to find the root caller, all the forwarding calls should be the same and not convert

csmoe (Apr 25 2019 at 15:43, on Zulip):

wait a moment, I'd paste a case.

oli (Apr 25 2019 at 15:43, on Zulip):

Oh that is a caller? But we do have a SubstsRef here, right?

oli (Apr 25 2019 at 15:45, on Zulip):

ah, this is happening because of the Deref impl on SubstsRef

oli (Apr 25 2019 at 15:46, on Zulip):

yea I think you can just replace &'tcx [Kind] with SubstsRef<'tcx>

csmoe (Apr 25 2019 at 15:51, on Zulip):

yea I think you can just replace &'tcx [Kind] with SubstsRef<'tcx>

okay.
(hmmm, failed to find out that case)

csmoe (Apr 28 2019 at 12:45, on Zulip):

@oli I was trying to convert a slice substs[...]: [Kind] into SubstsRef with tcx.mk_substs(), that's why tcx came in.

csmoe (Apr 28 2019 at 12:45, on Zulip):

https://github.com/rust-lang/rust/pull/59312#discussion_r279187806

oli (Apr 28 2019 at 12:45, on Zulip):

right, but isn't SubstsRef just SubstsRef { sth: &[Kind] }?

oli (Apr 28 2019 at 12:46, on Zulip):

the only time you need a tcx is if you don't start with a 'tcx lifetime

csmoe (Apr 28 2019 at 12:46, on Zulip):

right, but isn't SubstsRef just SubstsRef { sth: &[Kind] }?

inner is List<Kind>

oli (Apr 28 2019 at 12:46, on Zulip):

ah

oli (Apr 28 2019 at 12:47, on Zulip):

slightly uncool. Why do we even start with a &'tcx [Kind], can't that also be SubstsRef<'tcx>?

csmoe (Apr 28 2019 at 12:49, on Zulip):

slightly uncool. Why do we even start with a &'tcx [Kind], can't that also be SubstsRef<'tcx>?

yes, but I'm not sure can &[Kind] be optimized as the original issue planned?

oli (Apr 28 2019 at 12:53, on Zulip):

wait I'm confused, we're trying to reduce the number of &[Kind] and have more SubstsRef (which can have optimizations)

oli (Apr 28 2019 at 12:56, on Zulip):

ok, so https://github.com/rust-lang/rust/pull/59312/files#diff-239d31f6f959d7140a4da7f011fd08afR134 starts out with a SubstsRef, but we then want a "subslice" of that SubstsRef

oli (Apr 28 2019 at 12:57, on Zulip):

the old code just subsliced it and it worked

oli (Apr 28 2019 at 12:57, on Zulip):

now we are creating a duplicate of the subslice

oli (Apr 28 2019 at 12:57, on Zulip):

same in https://github.com/rust-lang/rust/pull/59312/files#diff-239d31f6f959d7140a4da7f011fd08afR202

oli (Apr 28 2019 at 12:59, on Zulip):

oh, nevermind, we have some invariants to keep up: https://github.com/rust-lang/rust/blob/517fb1b06f8b481d559285d1c3c665e143ad8156/src/librustc/ty/mod.rs#L603

oli (Apr 28 2019 at 12:59, on Zulip):

:confused:

oli (Apr 28 2019 at 13:02, on Zulip):

cc @eddyb we're running into a space vs perf optimization issue. Taking subslices of List<Subst> should be a O(1) operation imo and not take up any additional interning space, but at the same time we have the invariant that any List<Subst> is unique, so comparing two List<T>s is just a pointer equality check

eddyb (Apr 30 2019 at 05:19, on Zulip):

if you see &[Kind<'tcx>] it's intentional

eddyb (Apr 30 2019 at 05:19, on Zulip):

at least most of the time

eddyb (Apr 30 2019 at 05:20, on Zulip):

maybe I should've added a comment on all of the places

eddyb (Apr 30 2019 at 05:20, on Zulip):

specifically, substituting and printing necessarily need to work without a full Substs

eddyb (Apr 30 2019 at 05:21, on Zulip):

substituting, because creating a Substs needs to be able to substitute in defaults, and printing because of all the slicing going on

eddyb (Apr 30 2019 at 05:22, on Zulip):

I don't know why you might want to replace [Kind] with Substs, but you should avoid it unless necessary for something

csmoe (Apr 30 2019 at 05:25, on Zulip):

I don't know why you might want to replace [Kind] with Substs, but you should avoid it unless necessary for something

Substs derefs to [Kind], so I need to write &substs in some cases, but @oli felt uncool about those additional &.

csmoe (Apr 30 2019 at 05:25, on Zulip):

@eddyb https://github.com/rust-lang/rust/pull/59312#discussion_r267297677

eddyb (Apr 30 2019 at 06:50, on Zulip):

are we making Subst a struct?

eddyb (Apr 30 2019 at 06:50, on Zulip):

is that why?

csmoe (Apr 30 2019 at 06:51, on Zulip):

[Quoting…]
yes SubstsRref { inner: &List<Kind> }

eddyb (Apr 30 2019 at 06:51, on Zulip):

if so, is there any chance you could keep the & on the outside of an unsized struct? I guess not without another type alias....

eddyb (Apr 30 2019 at 06:51, on Zulip):

any reason why?

eddyb (Apr 30 2019 at 06:52, on Zulip):

if this isn't about the optimization @Ariel Ben-Yehuda proposed at the allhands, then I don't see why we're doing it

eddyb (Apr 30 2019 at 07:01, on Zulip):

@csmoe @oli if you want to make it more explicit, you can use &substs[..]

eddyb (Apr 30 2019 at 07:02, on Zulip):

and make the subst method take a Substs

eddyb (Apr 30 2019 at 07:02, on Zulip):

(and have it call another method that does the actual substitution)

eddyb (Apr 30 2019 at 07:02, on Zulip):

I assume that's where most of the & would've come from

Vadim Petrochenkov (Apr 30 2019 at 07:18, on Zulip):

Question (perhaps off-topic): Why SubstsRef is named SubstsRef and not Substs?
For interned / arena-allocated data the reference to it is the primarily used type and naming the data type itself is more rare.
So it's natural to use a short name for the reference and a long name for the data type (like Span and SpanData).
For substs that would be Substs and SubstsData or something.

eddyb (Apr 30 2019 at 07:18, on Zulip):

the Ref thing is transitionary, I believe

eddyb (Apr 30 2019 at 07:18, on Zulip):

@Ariel Ben-Yehuda wanted to optimize single-element substs so I think he did a name split to make refactoring easier

eddyb (Apr 30 2019 at 07:19, on Zulip):

it's relatively recent and the PR might have more information

Last update: Nov 20 2019 at 01:10UTC