Stream: t-compiler

Topic: design meeting 2019.06.14


nikomatsakis (Jun 14 2019 at 14:01, on Zulip):

Hi @T-compiler/meeting =)

eddyb (Jun 14 2019 at 14:01, on Zulip):

just got here!

nikomatsakis (Jun 14 2019 at 14:01, on Zulip):

Today we had scheduled to discuss compiler-team#104

eddyb (Jun 14 2019 at 14:02, on Zulip):

I'll go fix https://github.com/rust-lang/rust/pull/61817#issuecomment-501948291 now

nikomatsakis (Jun 14 2019 at 14:02, on Zulip):
Zoxc (Jun 14 2019 at 14:02, on Zulip):

Step 1) Merge https://github.com/rust-lang/rust/pull/61817
Step 2) Done? =P

pnkfelix (Jun 14 2019 at 14:03, on Zulip):

well in principle I guess we wanted to check whether this is the final product

pnkfelix (Jun 14 2019 at 14:03, on Zulip):

there were musings about e.g. renaming the variables

pnkfelix (Jun 14 2019 at 14:03, on Zulip):

(to cx or db or what not)

nikomatsakis (Jun 14 2019 at 14:03, on Zulip):

Yeah so I think the technical questions of how to do the merge have largely been resolved but

eddyb (Jun 14 2019 at 14:03, on Zulip):

so, I was going to take a gradual approach

nikomatsakis (Jun 14 2019 at 14:03, on Zulip):

(a) do we just want to let PRs bitrot? (I think yes)

nikomatsakis (Jun 14 2019 at 14:04, on Zulip):

but it'd be good if we advice for authors

nikomatsakis (Jun 14 2019 at 14:04, on Zulip):

on how to update

eddyb (Jun 14 2019 at 14:04, on Zulip):

but since it's possible to do it uniformly, in-place, I think we should separate getting to tcx: TyCtxt<'tcx> from renaming it in the future

Zoxc (Jun 14 2019 at 14:04, on Zulip):

We already broke PRs by removing 'a from TyCtxt

nikomatsakis (Jun 14 2019 at 14:04, on Zulip):

that doesn't invalidate what I wrote :)

eddyb (Jun 14 2019 at 14:04, on Zulip):

the rustc guide should be updated ASAP

nikomatsakis (Jun 14 2019 at 14:04, on Zulip):

and (b) let's talk a bit about the ultimate naming and conventions we want, I think?

nikomatsakis (Jun 14 2019 at 14:04, on Zulip):

But let's start with (a) ?

Zoxc (Jun 14 2019 at 14:04, on Zulip):

I think renaming TyCtxt to QueryCtxt makes sense. And it make make sense to do it now, since we brake PRs anyway

nikomatsakis (Jun 14 2019 at 14:05, on Zulip):

I think what would be useful is

nikomatsakis (Jun 14 2019 at 14:05, on Zulip):

well hmm I wonder if we can do it without knowing our end state.. I think it'd be useful to have some tracking issue showing the steps for the transition

nikomatsakis (Jun 14 2019 at 14:05, on Zulip):

and giving some advice -- it seems like eddyb did the work by some brute force search and replace

nikomatsakis (Jun 14 2019 at 14:05, on Zulip):

so just copying and pasting those steps is what I have in mind

nikomatsakis (Jun 14 2019 at 14:05, on Zulip):

(so others can do them on their code)

eddyb (Jun 14 2019 at 14:06, on Zulip):

note that these changes were hard because they're not just alpha renames, the type parametrization changes :P

nikomatsakis (Jun 14 2019 at 14:06, on Zulip):

Say a bit more?

eddyb (Jun 14 2019 at 14:06, on Zulip):

ah I see, you mean fixing their PRs, not renaming the contexts in the future

nikomatsakis (Jun 14 2019 at 14:07, on Zulip):

right so we've already landed the 'a removal PR --

nikomatsakis (Jun 14 2019 at 14:07, on Zulip):

so the fix for that is basically .. rewrite TyCtxt<'_, ...> to TyCtxt<...>

eddyb (Jun 14 2019 at 14:07, on Zulip):

I suspect, however, that most conflicts would be fixed by simply reacting to compiler errors

nikomatsakis (Jun 14 2019 at 14:07, on Zulip):

once we land the next PR, there are some more rewrites?

eddyb (Jun 14 2019 at 14:07, on Zulip):

most PRs don't touch signatures that much

nikomatsakis (Jun 14 2019 at 14:07, on Zulip):

I suspect, however, that most conflicts would be fixed by simply reacting to compiler errors

well having context for what was done is really helpful

eddyb (Jun 14 2019 at 14:08, on Zulip):

right, I just mean, you don't need my mass-replacement tricks, you can just go TyCtxt<?, ?, 'tcx> -> TyCtxt<'tcx> and you're good to go

nikomatsakis (Jun 14 2019 at 14:08, on Zulip):

so at least for the historical record @eddyb can you highlight the steps that have happened thus far, up to and including https://github.com/rust-lang/rust/pull/61817 ?

nikomatsakis (Jun 14 2019 at 14:08, on Zulip):

right, ok

nikomatsakis (Jun 14 2019 at 14:08, on Zulip):

makes sense

eddyb (Jun 14 2019 at 14:09, on Zulip):

we've banned unused lifetimes: #61735 (this means that when you replace uses of a lifetime you'll also be directed to remove its definition, if explicit)

nikomatsakis (Jun 14 2019 at 14:09, on Zulip):

at the moment, lift is still a thing -- I'm trying to remember -- I guess there are no other massive simplifications yet?

nikomatsakis (Jun 14 2019 at 14:09, on Zulip):

/me takes some notes in the hackmd

nikomatsakis (Jun 14 2019 at 14:10, on Zulip):

(what was the PR that removed the 'a from TyCtxt?)

eddyb (Jun 14 2019 at 14:10, on Zulip):

you're too fast for me!

Zoxc (Jun 14 2019 at 14:10, on Zulip):

https://github.com/rust-lang/rust/pull/61722

eddyb (Jun 14 2019 at 14:10, on Zulip):

we've removed the first lifetime of TyCtxt: #61722 - affected PRs need to do the same, and simply remove unused lifetimes after that

eddyb (Jun 14 2019 at 14:11, on Zulip):

and finally, #61817 requires replacing 'gcx lifetimes with 'tcx ones, and passing one less lifetime when 'gcx, 'tcx was previously taken

eddyb (Jun 14 2019 at 14:12, on Zulip):

@nikomatsakis one guiding principle is that when adapting your code past this PR you should never need to introduce a new lifetime, you're only removing them

nikomatsakis (Jun 14 2019 at 14:12, on Zulip):
eddyb (Jun 14 2019 at 14:12, on Zulip):

e.g. adding <'gcx> to an impl/fn is wrong

nikomatsakis (Jun 14 2019 at 14:12, on Zulip):

that's my summary thus far

nikomatsakis (Jun 14 2019 at 14:12, on Zulip):

but I'll add that note, that's a good point

eddyb (Jun 14 2019 at 14:13, on Zulip):

one weird edge case is TyCtxt<'_, 'tcx, '_>, that also needs to become TyCtxt<'tcx>

eddyb (Jun 14 2019 at 14:13, on Zulip):

(so it's not as simple as "leave only the last lifetime")

eddyb (Jun 14 2019 at 14:13, on Zulip):

@nikomatsakis also, "ask eddyb if you hit a weird issue and they're online" :P

nikomatsakis (Jun 14 2019 at 14:14, on Zulip):

ok I mean it basically comes down to

eddyb (Jun 14 2019 at 14:14, on Zulip):

the worst part about #61817 was a HashStable impl which used 'gcx instead of 'a for something and it ended up with a requirement that two unrelated lifetimes be the same

nikomatsakis (Jun 14 2019 at 14:14, on Zulip):

you probably just always want TyCtxt<'tcx> or TyCtxt<'_>?

Zoxc (Jun 14 2019 at 14:14, on Zulip):

Yeah, we're doing TyCtxt<'a, 'gcx, 'tcx> to TyCtxt<'gcx> but calling it TyCtxt<'tcx>

eddyb (Jun 14 2019 at 14:14, on Zulip):

and which caused lifetime errors in derived code

nikomatsakis (Jun 14 2019 at 14:14, on Zulip):

(of course that assumes people are following general conventions)

eddyb (Jun 14 2019 at 14:15, on Zulip):

@nikomatsakis yes. also, 'lcx -> 'tcx (but that's much rarer)

eddyb (Jun 14 2019 at 14:16, on Zulip):

@nikomatsakis something we could do is go over the affected PRs (as reported by @bors) and paste a link to a summary like this?

nikomatsakis (Jun 14 2019 at 14:16, on Zulip):

OK. this seems great. I'll try to write up some brief notes and open a tracking issue

eddyb (Jun 14 2019 at 14:16, on Zulip):

I could do it if I don't forget

nikomatsakis (Jun 14 2019 at 14:16, on Zulip):

and then yeah we can post the link to the affected PRs, I guess

eddyb (Jun 14 2019 at 14:16, on Zulip):

GitHub already shows a bunch in #61722

nikomatsakis (Jun 14 2019 at 14:18, on Zulip):

should we discuss the bikeshed-y questions? :)

eddyb (Jun 14 2019 at 14:18, on Zulip):

for "renaming away from TyCtxt", I propose to think about the guiding principles first, and then bikeshed later

eddyb (Jun 14 2019 at 14:19, on Zulip):

e.g. we really want this context to extend back to parsing, so it having "Ty(pe)" in the name (or "Mir", as I've offhandedly proposed) would be misleading/incorrect. this is why "query" or salsa's "database" ("db") come to mind. other options are some variations on "rustc" or "compile(r)"

eddyb (Jun 14 2019 at 14:21, on Zulip):

another thing is that we should have an uniform naming scheme for contexts - at some point I renamed a bunch of things in rustc_codegen_llvm using a new scheme, but haven't added it to anything else (that scheme is something like fx: FooCx)

Zoxc (Jun 14 2019 at 14:21, on Zulip):

@mw discussed having other context which only has access to types. So that could be a TyCtxt and QueryCtxtwould be the thing with access to executing queries.

nikomatsakis (Jun 14 2019 at 14:21, on Zulip):

should we discuss the bikeshed-y questions? :)

quick note: I created https://github.com/rust-lang/rust/issues/61838 (tracking issue)

eddyb (Jun 14 2019 at 14:21, on Zulip):

what do you mean "only"?!

eddyb (Jun 14 2019 at 14:21, on Zulip):

very few things work without queries

Zoxc (Jun 14 2019 at 14:22, on Zulip):

You can create types and do operations on types which do not require queries

eddyb (Jun 14 2019 at 14:22, on Zulip):

anyway, we should either undo that rustc_codegen_llvm change, use it everywhere, or come up with a third naming scheme

nikomatsakis (Jun 14 2019 at 14:22, on Zulip):

I agree with @eddyb that linking the name to something specific like types seems overly narrow

Vadim Petrochenkov (Jun 14 2019 at 14:22, on Zulip):

I propose to think about the guiding principles first

1) The shorter the better!

eddyb (Jun 14 2019 at 14:22, on Zulip):

@Zoxc I think that linking together allocation/interning and query execution is important

eddyb (Jun 14 2019 at 14:23, on Zulip):

@Vadim Petrochenkov heh, yeah. this is why I like qx: QueryCx<'q>

nikomatsakis (Jun 14 2019 at 14:23, on Zulip):

As an experience report, I've been very happy with salsa's db: Database convention -- it is very easy to explain and people seem to get it quite naturally.

Zoxc (Jun 14 2019 at 14:23, on Zulip):

See https://github.com/rust-lang/rust/pull/59505#issuecomment-478573274

eddyb (Jun 14 2019 at 14:23, on Zulip):

db is also unlikely to arise randomly in code

nikomatsakis (Jun 14 2019 at 14:23, on Zulip):

But I'm a bit unsure what @Zoxc is saying so I'd like to understand that better

eddyb (Jun 14 2019 at 14:23, on Zulip):

and db: RustcDb<'db> feels pretty nice

oli (Jun 14 2019 at 14:23, on Zulip):

db is also unlikely to arise randomly in code

db: DiagnosticBuilder ;)

pnkfelix (Jun 14 2019 at 14:24, on Zulip):

If I see "db" in a random place in the Rust repo

pnkfelix (Jun 14 2019 at 14:24, on Zulip):

its definitely going to be ambiguous

pnkfelix (Jun 14 2019 at 14:24, on Zulip):

tcx isn't. I suspect qx wouldn't be, either.

eddyb (Jun 14 2019 at 14:25, on Zulip):

@oli dammit

nikomatsakis (Jun 14 2019 at 14:25, on Zulip):

See https://github.com/rust-lang/rust/pull/59505#issuecomment-478573274

OK, this helps. So the idea is that sometimes we have "pure functions" and we'd like to give them access to only limited parts of the state, such as the interning caches. The salsa answer here would be to use traits for this -- give that code access to the db, but with a restricted trait (e.g., an db: &impl InterningDatabase)

pnkfelix (Jun 14 2019 at 14:25, on Zulip):

(I'm not just talking about source code; I'm including comments or snippets from chat sessions)

nikomatsakis (Jun 14 2019 at 14:25, on Zulip):

its definitely going to be ambiguous

we can of course rewrite those places, but that's more work.

eddyb (Jun 14 2019 at 14:26, on Zulip):

I don't like the "database" terminology too much in general but it seemed neat in the context of salsa

nikomatsakis (Jun 14 2019 at 14:26, on Zulip):

I'm thinking about the x convention --

nikomatsakis (Jun 14 2019 at 14:26, on Zulip):

My experience has been that tcx is very confusing for newcomers

nikomatsakis (Jun 14 2019 at 14:26, on Zulip):

or at least most of the times that I explain what it is, people go "ah, so that's what it is" :)

nikomatsakis (Jun 14 2019 at 14:27, on Zulip):

I think it's not a huge deal --

nikomatsakis (Jun 14 2019 at 14:27, on Zulip):

you have to learn that pretty quickly :)

nikomatsakis (Jun 14 2019 at 14:27, on Zulip):

but if we can find something that's more immediately accessible, seems good

pnkfelix (Jun 14 2019 at 14:27, on Zulip):

See https://github.com/rust-lang/rust/pull/59505#issuecomment-478573274

OK, this helps. So the idea is that sometimes we have "pure functions" and we'd like to give them access to only limited parts of the state, such as the interning caches. The salsa answer here would be to use traits for this -- give that code access to the db, but with a restricted trait (e.g., an db: &impl InterningDatabase)

in theory I want to support ideas like this. But in practice, what it often leads to, is that when I'm in the midst of debugging, I end up having to thread state down from a caller in order to get access to the context object (qx or whatever) that I would have otherwise already had on hand.

Zoxc (Jun 14 2019 at 14:27, on Zulip):

I think having just 1 lifetime instead of 3 is very helpful already

pnkfelix (Jun 14 2019 at 14:27, on Zulip):

However, if we continue to offer our thread-local accessors for the qx

nikomatsakis (Jun 14 2019 at 14:28, on Zulip):

I agree that reducing the lifetimes is very helpful :)

pnkfelix (Jun 14 2019 at 14:28, on Zulip):

with thread-local accessors, the need to thread the state down goes away (and thus my objection dissolves)

nikomatsakis (Jun 14 2019 at 14:28, on Zulip):

It seems like "query context" and "database" are the two main contenders right now?

nikomatsakis (Jun 14 2019 at 14:28, on Zulip):

One other thing I wanted to say:

eddyb (Jun 14 2019 at 14:29, on Zulip):

@pnkfelix oh, regarding that, I wanted to say that lift is still useful for "going between unrelated lifetimes" (i.e. using it with TLS) even though the 'tcx -> 'gcx use is gone

eddyb (Jun 14 2019 at 14:29, on Zulip):

@nikomatsakis something involving "rustc" or "compiler" too

nikomatsakis (Jun 14 2019 at 14:29, on Zulip):

It's definitely a common thing to have "contexts" all over the code, as @eddyb pointed out, and having a convention there seems good -- but does it make sense for the "main compiler context" to also follow that convenion, or is worth distinguishing it? It's kind of the "ur-context" from which all others derive, and it's special in that it's the only one that hosts queries, so I can see a case for distinct conventions. But not sure.

eddyb (Jun 14 2019 at 14:30, on Zulip):

@nikomatsakis that's where I see "db" come in

eddyb (Jun 14 2019 at 14:30, on Zulip):

I wouldn't need to tack "context" onto "db"

nikomatsakis (Jun 14 2019 at 14:30, on Zulip):

that's kind of my point, right

pnkfelix (Jun 14 2019 at 14:30, on Zulip):

with thread-local accessors, the need to thread the state down goes away (and thus my objection dissolves)

(and also, when the motivation is not just an abstract notion of "code purity", but rather a concrete thing like "this allows us to use a cache and eases the proof that its correct", then my objection also goes away)

nikomatsakis (Jun 14 2019 at 14:30, on Zulip):

(I was going to say @pnkfelix that I think the correctness of the incremental caches outweighs that)

nikomatsakis (Jun 14 2019 at 14:31, on Zulip):

(but I agree that people should generally just take the tcx and be done with it otherwise)

oli (Jun 14 2019 at 14:31, on Zulip):

note that the DiagnosticBuilder case can be ignored, it's easily changed and @WG-diagnostics is working on stuff in this direction anyway. I just wanted to bring it up

eddyb (Jun 14 2019 at 14:31, on Zulip):

I don't know how to shorten rustc otherwise rustc: Rustc<'r> might be easier on everyone?

nikomatsakis (Jun 14 2019 at 14:32, on Zulip):

I also agree it doesn't seem like the killer objection

eddyb (Jun 14 2019 at 14:32, on Zulip):

like, you're passing the compiler around :P

nikomatsakis (Jun 14 2019 at 14:32, on Zulip):

I don't know how to shorten rustc otherwise rustc: Rustc<'r> might be easier on everyone?

that's an interesting idea :)

eddyb (Jun 14 2019 at 14:32, on Zulip):

(and nowadays we can move the lifetime outside it btw)

eddyb (Jun 14 2019 at 14:32, on Zulip):

we had to add a lint against &TyCtxt because people intuitively did that

nikomatsakis (Jun 14 2019 at 14:33, on Zulip):

the other thing that salsa does which I continu to think we may want eventually -- though I know @eddyb has concerns -- is that it breaks up the "database" into traits, one per crate, so that you don't have to define all the queries in one place. This makes it meaningful to talk about things like db: &impl TypeCheckDatabase (just those queries that the type checker needs)

eddyb (Jun 14 2019 at 14:33, on Zulip):

and TyCtxt being a struct of references is an artifact of the 'gcx/'tcx split (I wonder if we'll speed up the codegen time of rustc if we clean that stuff up :P?)

nikomatsakis (Jun 14 2019 at 14:33, on Zulip):

I feel like names like rustc don't scale to that as easily

pnkfelix (Jun 14 2019 at 14:33, on Zulip):

I don't know how to shorten rustc otherwise rustc: Rustc<'r> might be easier on everyone?

(what, you think rc might be confusing?) :lol:

eddyb (Jun 14 2019 at 14:33, on Zulip):

@pnkfelix I almost said it out loud!

eddyb (Jun 14 2019 at 14:33, on Zulip):

but it seemed too obvious to even bring it up

nikomatsakis (Jun 14 2019 at 14:34, on Zulip):

"lrc" :P

eddyb (Jun 14 2019 at 14:34, on Zulip):

rs: Rust<'rs>

eddyb (Jun 14 2019 at 14:34, on Zulip):

let's rename rustc to rsc

nikomatsakis (Jun 14 2019 at 14:34, on Zulip):

So I guess we have:

nikomatsakis (Jun 14 2019 at 14:35, on Zulip):

I'm not sure how serious you were @eddyb but it's...kinda cute?

pnkfelix (Jun 14 2019 at 14:35, on Zulip):

or doctor rust: rx: Rust<'rx>

eddyb (Jun 14 2019 at 14:35, on Zulip):

do we want to piss off https://swtch.com/~rsc/, hmmm

Zoxc (Jun 14 2019 at 14:35, on Zulip):

Also Session<'s>?

eddyb (Jun 14 2019 at 14:35, on Zulip):

"if you wanted to work on Go why did you put RS in your name"

eddyb (Jun 14 2019 at 14:36, on Zulip):

@Zoxc ah, nice!

nikomatsakis (Jun 14 2019 at 14:36, on Zulip):

Session?

nikomatsakis (Jun 14 2019 at 14:36, on Zulip):

/me contemplates

eddyb (Jun 14 2019 at 14:36, on Zulip):

ss: Sess<'ss>. wait, SS has negative connotations :(

nikomatsakis (Jun 14 2019 at 14:36, on Zulip):

It implies to me that there is something "longer lived" that we are connected to, I guess maybe the incremental state?

Zoxc (Jun 14 2019 at 14:36, on Zulip):

And merge TyCtxt with our existing Session struct =P

nikomatsakis (Jun 14 2019 at 14:36, on Zulip):

Ah, yeah, I forgot about the existing sess

eddyb (Jun 14 2019 at 14:37, on Zulip):

@nikomatsakis whereas this context we want is a generation within the session :P?

eddyb (Jun 14 2019 at 14:38, on Zulip):

@nikomatsakis regarding salsa, and taking inspiration from the FooCx scheme, what about db: &impl TypeckDb?

eddyb (Jun 14 2019 at 14:38, on Zulip):

I wouldn't mind that

nikomatsakis (Jun 14 2019 at 14:39, on Zulip):

yes, seems fine. I've been trending away from abbreviations where I can but "db" seems ok

Zoxc (Jun 14 2019 at 14:39, on Zulip):

I also don't like the database term. That's for storing and loading stuff, not for computation =P

eddyb (Jun 14 2019 at 14:39, on Zulip):

you could use that to hide all the typesystem-related stuff from libsyntax

eddyb (Jun 14 2019 at 14:39, on Zulip):

/me pours some Prolog clauses onto @Zoxc

eddyb (Jun 14 2019 at 14:39, on Zulip):

(but I used to agree. salsa is the only place where it feels okay)

eddyb (Jun 14 2019 at 14:40, on Zulip):

hmmmm regarding the libsyntax comment, we might want to switch to salsa's model? instead of making libsyntax depend on librustc?

eddyb (Jun 14 2019 at 14:40, on Zulip):

I wonder what the performance penalty is like

nikomatsakis (Jun 14 2019 at 14:41, on Zulip):

we can discuss; I don't know that there is one

nikomatsakis (Jun 14 2019 at 14:42, on Zulip):

at least in salsa there is just one point of virtual dispatch (or none, if you use &impl ...)

nikomatsakis (Jun 14 2019 at 14:42, on Zulip):

(i.e., you can use &dyn)

eddyb (Jun 14 2019 at 14:42, on Zulip):

it might make sense to have librustc_hir andlibrustc_ty and have code that just doesn't see "more advanced" queries

nikomatsakis (Jun 14 2019 at 14:42, on Zulip):

yes, I think it makes sense to switch. But it's probably a good topic to dive into later.

eddyb (Jun 14 2019 at 14:42, on Zulip):

@nikomatsakis OOOOOOOOH

nikomatsakis (Jun 14 2019 at 14:42, on Zulip):

I feel like we reached the end of our "bikeshedding" section but I don't know if we have a clear consensus

nikomatsakis (Jun 14 2019 at 14:43, on Zulip):

I guess we could do a little poll :)

nikomatsakis (Jun 14 2019 at 14:43, on Zulip):

(we should also ask if we think renaming is worthwhile at all)

eddyb (Jun 14 2019 at 14:43, on Zulip):

I think we can take longer on it

nikomatsakis (Jun 14 2019 at 14:44, on Zulip):

(I tend to think it is -- I think these little bits of naming add up and form a mental tax that I'm only lately becoming aware of.)

nikomatsakis (Jun 14 2019 at 14:44, on Zulip):

but I also agree I feel very little rush

eddyb (Jun 14 2019 at 14:44, on Zulip):

given that tcx: TyCtxt<'tcx> "just works"

nikomatsakis (Jun 14 2019 at 14:44, on Zulip):

maybe we record the options and let this settle --

nikomatsakis (Jun 14 2019 at 14:44, on Zulip):

and plan a later session to talk out the salsa composition model and how that might work?

nikomatsakis (Jun 14 2019 at 14:44, on Zulip):

not sure if that influences the decision but I think maybe it does a little

eddyb (Jun 14 2019 at 14:45, on Zulip):

so, with salsa: you pay the cost of monomorphization, which I don't think we want (it would cause rustc_interface or rustc_driver to compile the entirety of rustc that uses tcx today. and it doesn't work with the TLS stuff). we'd have to use &dyn everywhere. would &dyn at least not do a virtual call in the case when it hits the cache?

eddyb (Jun 14 2019 at 14:45, on Zulip):

or is salsa so much simpler than I thought it was =)))

(given the fact that TyCtxt contains one big struct of fn pointers per crate, I thought Salsa also had to store them somewhere)

nikomatsakis (Jun 14 2019 at 14:45, on Zulip):

There were the options I saw:

nikomatsakis (Jun 14 2019 at 14:46, on Zulip):

would &dyn at least not do a virtual call in the case when it hits the cache?

no you do pay a virtual call even when cached

eddyb (Jun 14 2019 at 14:46, on Zulip):

db: RustcDb<'db> is one I liked I think

eddyb (Jun 14 2019 at 14:47, on Zulip):

no you do pay a virtual call even when cached

okay yeah I assumed Salsa was magical when it simply didn't have the same optimizations =))

nikomatsakis (Jun 14 2019 at 14:48, on Zulip):

I was always hoping we can get incremental to the point where monomorphizing everything at the end is fine

nikomatsakis (Jun 14 2019 at 14:49, on Zulip):

it's...probably not there yet :)

nikomatsakis (Jun 14 2019 at 14:49, on Zulip):

All right, thanks everybody! :tada:

pnkfelix (Jun 14 2019 at 14:49, on Zulip):

I really hope we don’t go with db. For some reason I’d even prefer s, which seems absurd ...

nikomatsakis (Jun 14 2019 at 14:50, on Zulip):

Interesting. I really like it, but I'm open to the alternatives too

nikomatsakis (Jun 14 2019 at 14:50, on Zulip):

(To me, it just feels like the right metaphor, esp. with queries -- it's the place where all the data lives, and you are fetching things out of it)

nikomatsakis (Jun 14 2019 at 14:50, on Zulip):

but I am happy to let the options sit a bit

nikomatsakis (Jun 14 2019 at 14:51, on Zulip):

it probably makes sense to try and record some notes about each option

Vadim Petrochenkov (Jun 14 2019 at 14:52, on Zulip):

Is it going to be a top level context with nothing above it?

Vadim Petrochenkov (Jun 14 2019 at 14:53, on Zulip):

Perhaps not because there can be multiple "sessions" with entirely independent contexts?

eddyb (Jun 14 2019 at 14:54, on Zulip):

@Vadim Petrochenkov top-level but there is stuff in rustc_interface which creates one such context for every incremental "generation" (i.e. after every change to the inputs)

nikomatsakis (Jun 14 2019 at 14:54, on Zulip):
- We are not thrilled with `TyCtxt&lt;'tcx&gt;` and are considering some alternatives:
    - `qx: QueryCx&lt;'q&gt;`
        - couple with `x` being the suffix we use for "contexts" in general (this convention was adopted in trans, but not elsewhere)
    - `db: RustcDb&lt;'db&gt;` (or perhaps `db: Database&lt;'db&gt;`)
        - if we do more modules, then `RustcDb` might also be `ParsingDb` (for parsing-related queries), `TypeckDb` (for type-related queries), etc
        - though there are some impl concerns around that; salsa style modules do impose 
            - dyn dispatch for each call, even cache hits; or,
            - monomorphization costs
    - `rs: Rust&lt;'rs&gt;`
    - `rx: Rust&lt;'rx&gt;`
        - this is the "rust compiler", so maybe that should be the basis of the name?
    - `s: Session&lt;'s&gt;`
        - related to existing `tcx.sess`
        - session of "what"?
eddyb (Jun 14 2019 at 14:54, on Zulip):

so it's technically not a rustc session for RLS, because the "session" can survive for many generations

nikomatsakis (Jun 14 2019 at 14:55, on Zulip):

er, I edited a bit more,

eddyb (Jun 14 2019 at 14:55, on Zulip):

@nikomatsakis seß: Seß<'seß>

nikomatsakis (Jun 14 2019 at 14:55, on Zulip):

but take a look at the hackmd and add your own thoughts

nikomatsakis (Jun 14 2019 at 14:55, on Zulip):

I can try to consolidate if it gets messy

eddyb (Jun 14 2019 at 14:55, on Zulip):

I can type that with AltGr+W

Zoxc (Jun 14 2019 at 14:56, on Zulip):

I prefer just merging crates into librustc instead of having a modular query system, especially since Rust doesn't have a module system

eddyb (Jun 14 2019 at 14:56, on Zulip):

don't you like to hack around the lack of ML modules with traits :grinning_face_with_smiling_eyes:

eddyb (Jun 14 2019 at 14:58, on Zulip):

the crate separation exists for a reason. I wouldn't want to split the crates too much but I wouldn't e.g. merge libsyntax into librustc, just e.g. the AST (and make libsyntax depend on librustc)

Zoxc (Jun 14 2019 at 14:59, on Zulip):

It adds too much syntatic overhead, for very little benefit.

I also like having a central list of queries, as it's useful to keep the amount of queries low (for performance reasons) and it gives kind of an overview of the compiler

nikomatsakis (Jun 14 2019 at 14:59, on Zulip):

ok, I gotta run, but I'll leave the hackmd world editable for a bit longer

nikomatsakis (Jun 14 2019 at 14:59, on Zulip):

(I would prefer to have a system that supports crate splits, but I don't care to argue it now :)

eddyb (Jun 14 2019 at 15:00, on Zulip):

@nikomatsakis FWIW the problem is not with incremental - if we do MIR-only rlibs for rustc we'd have codegen at the end anyway - then making the code generic over the context would only result in noise (i.e. new type parameters on every context struct) but not a perf difference

nikomatsakis (Jun 14 2019 at 15:01, on Zulip):

It does introduce a type parameter, that's true. I've thought about trying to make it so that you have db: DB instead of db: &DB, so that you are just making everything parameteric over the type parameter only

eddyb (Jun 14 2019 at 15:01, on Zulip):

so it's 1. when codegen happens 2. noooooiseeeee

nikomatsakis (Jun 14 2019 at 15:01, on Zulip):

(db: impl Database)

eddyb (Jun 14 2019 at 15:01, on Zulip):

@nikomatsakis one cool thing would be these databases to have not just queries but also arenas/interners for various entities

eddyb (Jun 14 2019 at 15:02, on Zulip):

this is one of those things I want a DSL to automate away, and salsa's derives are kind of a high-level compiler DSL :P

eddyb (Jun 14 2019 at 15:05, on Zulip):

@nikomatsakis the generic problem would be alleviated by having entire crates generic over<'db, DB: RustcDb<'db>> :P

Vadim Petrochenkov (Jun 14 2019 at 15:07, on Zulip):

@eddyb

stuff in rustc_interface which creates one such context for every incremental "generation" (i.e. after every change to the inputs) ... so it's technically not a rustc session for RLS, because the "session" can survive for many generations

I don't understand what are "generation", "input" and "session" here.
Any specific examples?

eddyb (Jun 14 2019 at 15:08, on Zulip):

@pnkfelix @nikomatsakis @Zoxc @oli @centril hang on how are we merging #61817? who's r+'-ing it? are we treeclose-ing?

eddyb (Jun 14 2019 at 15:09, on Zulip):

@Vadim Petrochenkov right now you have to run rustc multiple times with -Cincremental=some-dir-name (which cargo does for you) to incrementally recompile with different "input" (i.e. the contents of the files and maybe CLI flags)

eddyb (Jun 14 2019 at 15:09, on Zulip):

you can call each of those different runs, "generations"

eddyb (Jun 14 2019 at 15:10, on Zulip):

@Vadim Petrochenkov so a "rustc session", ran by RLS, might contain many such generations, as many as one per keypress :P

Zoxc (Jun 14 2019 at 15:11, on Zulip):

How can you review such large PRs? =P

eddyb (Jun 14 2019 at 15:11, on Zulip):

@nikomatsakis oh dammit we didn't discuss whether to merge the rustfmt commit!

eddyb (Jun 14 2019 at 15:12, on Zulip):

how did I forget about it

eddyb (Jun 14 2019 at 15:12, on Zulip):

@Zoxc scrolling while squinting

Zoxc (Jun 14 2019 at 15:12, on Zulip):

I know github doesn't handle them =P

eddyb (Jun 14 2019 at 15:12, on Zulip):

ugh

eddyb (Jun 14 2019 at 15:13, on Zulip):

@Vadim Petrochenkov imagine a loop that handles both input changes (by throwing away the global context and starting a new generation, but reusing incremental state) and IDE requests (e.g. "go to def", by querying the appropriate information from the current generation's context) - that's kinda RLS. or will be, in the future (the current situation is a bit messy and the whole "save-analysis" is a disaster we still need to recover from)

Zoxc (Jun 14 2019 at 15:13, on Zulip):

Doesn't the rustfmt commit touch the same lines as the PR? So it shouldn't matter?

eddyb (Jun 14 2019 at 15:14, on Zulip):

it may touch the entire signature where the first commit might only have touched one argument type

eddyb (Jun 14 2019 at 15:14, on Zulip):

other than that, yes

Zoxc (Jun 14 2019 at 15:14, on Zulip):

The changes lines are the same for both commits and the whole PR, so including it shouldn't cause more breakage

Vadim Petrochenkov (Jun 14 2019 at 15:15, on Zulip):

@eddyb
I see, so it' s a hypothetical future design rather than something that already exists in rustc.
If those generations work with "same code" more or less, then it's indeed reasonable to keep all the global data (like symbol interner), which is apparently going to be a part of this BikeshedCtxt.

eddyb (Jun 14 2019 at 15:16, on Zulip):

@Vadim Petrochenkov maybe. I believe right now it might start from scratch every single time (and is forced to reload from disk). while RLS might not be using it yet (not sure), the code should already exist in rustc_interface (@Zoxc knows more)

Zoxc (Jun 14 2019 at 15:17, on Zulip):

There's only code to load from disk right now

oli (Jun 14 2019 at 15:18, on Zulip):

@eddyb damn we forgot to discuss that, but I got the feeling that it was unanimous that we'll do this, so I guess I can just scroll through it, sign off and you can treeclose and force it through whenever you have a few hours time

eddyb (Jun 14 2019 at 15:18, on Zulip):

haha

eddyb (Jun 14 2019 at 15:19, on Zulip):

let's do it!

Vadim Petrochenkov (Jun 14 2019 at 15:19, on Zulip):

oh dammit we didn't discuss whether to merge the rustfmt commit!

GitHub recently implemented a little UX "improvement" - +100 -500 line diffs are no longer reported for separate commits.
So e.g. I don't even known how much of that PR is rustfmt and how much is lifetime changes without cloning it locally.
:angry:

eddyb (Jun 14 2019 at 15:20, on Zulip):

@Vadim Petrochenkov oh right. I can check!

oli (Jun 14 2019 at 15:20, on Zulip):

@Vadim Petrochenkov if you go to the commit directly you still get that info: https://github.com/rust-lang/rust/commit/65257a9bd7d5fc31dec583923f7b4977400de6e6

eddyb (Jun 14 2019 at 15:21, on Zulip):

git log --shortstat:

commit 94cfb14651528288deaa84fd386648ca7520acdb (HEAD -> begone-gcx-attempt-2, origin/begone-gcx-attempt-2)
Author: Eduard-Mihai Burtescu <edy.burt@gmail.com>
Date:   Fri Jun 14 01:32:15 2019 +0300

    Run `rustfmt --file-lines ...` for changes from previous commits.

 107 files changed, 813 insertions(+), 1159 deletions(-)

commit 65257a9bd7d5fc31dec583923f7b4977400de6e6
Author: Eduard-Mihai Burtescu <edy.burt@gmail.com>
Date:   Fri Jun 14 00:48:52 2019 +0300

    Unify all uses of 'gcx and 'tcx.

 341 files changed, 3110 insertions(+), 3336 deletions(-)
eddyb (Jun 14 2019 at 15:21, on Zulip):

oh dammit

eddyb (Jun 14 2019 at 15:21, on Zulip):

that doesn't give me the delta of files touched

eddyb (Jun 14 2019 at 15:22, on Zulip):

I guess I can try to compute it? both commits combined are:

 341 files changed, 3628 insertions(+), 4200 deletions(-)
Vadim Petrochenkov (Jun 14 2019 at 15:25, on Zulip):

@oli
That's better than nothing, perhaps there's some way to go to the commit directly without doing manual URL editing?

oli (Jun 14 2019 at 15:25, on Zulip):

yes

oli (Jun 14 2019 at 15:25, on Zulip):

you can click the commit id in the PR commit view

Zoxc (Jun 14 2019 at 15:27, on Zulip):

Seems like it's not insignificant then. I'd prefer to leave rustfmt off then. I'd find it much easier to spot that only TyCtxt changed if there's no formatting changes too

eddyb (Jun 14 2019 at 15:27, on Zulip):

so the sum is +3923 -4495 but the actual combination is +3628 -4200

centril (Jun 14 2019 at 15:28, on Zulip):

I second @Vadim Petrochenkov's motion for brevity; qx: QCx<'q> would be neat

eddyb (Jun 14 2019 at 15:28, on Zulip):

so +295 -295 (i.e. 295 changed lines) are shared

eddyb (Jun 14 2019 at 15:36, on Zulip):

@Zoxc @Vadim Petrochenkov sorry, got distracted: so my guess is that the second commit has +518 -864 changes which touch lines not touched by the main commit

eddyb (Jun 14 2019 at 15:37, on Zulip):

the large negative diff is from multi-line signatures collapsing into a single line now, I think?

nikomatsakis (Jun 21 2019 at 13:19, on Zulip):

I added some notes from the gcx, tcx design meeting here:

https://github.com/rust-lang/compiler-team/pull/108

these are basically the hackmd proposal plus the notes I made live.

Do we think this transition is basically done? (i.e., all affected PRs are updated etc)

pnkfelix (Jun 21 2019 at 13:21, on Zulip):

From the T-compiler meeting yesterday it sounded like @eddyb was treating it as "gone done done."

eddyb (Jun 21 2019 at 17:23, on Zulip):

yeah pretty much. it's just the longer-term renaming that's yet to be started on

Last update: Nov 21 2019 at 13:05UTC