Get rid of most of the RefCells in librustc_typeck/check

[pythonesque]

There are two that I didn't touch that are kind of irksome to get rid of. Though I do want to do it, I also don't want to make this commit too much larger.

I may also try to refactor this to see if I can get rid of some of the extra structures / function parameters; it was helpful as an intermediate step, but I think most of the time we can get away with just passing one context around.

[rust-highfive]

r? @alexcrichton

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

[alexcrichton]

Wow! Nice!

I don't personally work much in these crates though, so I'll defer to @nikomatsakis or @pnkfelix

r? @pnkfelix

[arielb1]

couldn't you just take FnCtxt by mutable reference?

[pythonesque]

@arielb1 Not in general, because it has to be shared in a number of places (e.g. the Typing for the euv walker in upvar.rs). With more refactoring it could be, though; I'm working on another patch to fix trans in a similar way to open up this possibility.

[arielb1]

I couldn't find any Typing in upvar.rs

[pythonesque]

@arielb1https://github.com/rust-lang/rust/pull/25247/files#diff-193820a23727493def60049c0b6af47eR443 (might be called Typer). The problem is that this needs to be able to access some shared variables (notably, the remaining RefCell'd ones) mutably, but the Typer implementation requires access as well.

[pythonesque]

You could still pass it as a mutable reference in most places, though, and just split it up into two internal structures when necessary (as it is there), which is the potential refactoring I alluded to above. This is the approach I'm taking from the getgo in the trans patch.

[arielb1]

@pythonesque

I am afraid your splitting creates lots of ugly duplication.
Well its called TYPER. I would add a method to get the &mut TYPER from the euv (store a TYPER rather than &mut TYPER and lift mc::Typer over &) - I don't think euv relies on its typer staying the same.

[pythonesque]

@arielb1 Yes, like I said above, it can be done, but I didn't want to make the patch too much larger. In particular, adding such a method would also require us to make the Typer in trans mutable, which is what my other patch is doing (I think, anyway--but maybe making it store a Typer instead of a &mut Typer allows us to avoid this?).

I agree that the duplication is unfortunate; a lot of it could probably be resolved with a macro like the unpack_datum! one in trans, but I didn't think of it at the time.

[arielb1]

@pythonesque

The current patch seems rather large and ugly to me (passing FnCtxt and CheckEnv through all of typeck's methods - half of them already have rather more parameters than optimal).

[nikomatsakis]

@pythonesque

I didn't have time to read this today, but I have been wanting to refactor a lot of these refcells away for some time, so this at least sounds great!

[pythonesque]

@arielb1#25332 is the trans patch and passes a mutable reference like you were asking about.

Unfortunately, a lot of the time in order to actually use it I had to end up splitting the parameters immediately on function entry and then join them together for subsequent calls, which I feel like kinds of defeats the purpose of having multiple parameters.

Then again, trans is probably a little different, since there we are dealing with multiple blocks, each of which references a single context, while here we can probably just treat it as one context for all intents and purposes. So maybe I actually should have taken the double-parameter approach in trans, and the single-parameter approach here, rather than the other way around.

[bors]

☔ The latest upstream changes (presumably #25320) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #24619) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

I started a discuss thread to discuss this approach, since it seems to have some pros and cons:

https://internals.rust-lang.org/t/prs-removing-refcells-longer-term-plans/2099

@pythonesque I'm sorry for the slow feedback...it's been a bit crazy last few weeks.

[nikomatsakis]

Why make this more generic?

[nikomatsakis]

Oh, I see, Typer is no longer object-safe. I think I'd prefer that Typer stay object-safe; pity we don't have a good solution for passing FnOnce() closurs as objects yet.

[pythonesque]

Ah, yes, I remember the issue I had now. Yeah, there was literally no way to add a closure (that I could see) without breaking object safety.

[jroesch]

Doing a re-review of these changes, this doesn't matter anymore I axed the Typer interface last week.

[pnkfelix]

@nikomatsakis@jroesch how would this change interact with the recently discussed revisions to the compiler infrastructure? I am basically wondering if I should push through a review of this PR (and then presumably request it be rebased, assuming it passes review), or if we should put this aside for now until after other refactorings take place?

From what I remember, it seems like this PR should probably help more than it hurts future refactorings.

[nikomatsakis]

@pnkfelix While on vacation, I was thinking about these PRs and asking myself the same question. I'm not really clear on how these PR fit into the plans we were discussing. I think I have to get the various refactorings reloaded a bit into my head before I can evaluate the impact.

[jroesch]

I've been meaning to review this work as well, will sit down after lunch and read it over.

[nikomatsakis]

Currently my feeling though is that we are pursuing a lot of refactorings -- flattening contexts, breaking up tables to be more local, incremental compilation, MIR -- and that it's we should leave things as they are under the assumption that they are going to be dismembered and put back together, and when we do this we will try to make best use of our type system.

[Gankra]

Based on the inactivity and @nikomatsakis' comment, I'm going to close this PR.

Feel free to disagree!