Resolver: introduce a conditionally mutable Resolver for (non-)speculative resolution.

[LorrensP-2158466]

This pr introduces a CmResolver, a wrapper around the main resolver which gives out mutable access given a condition.

CmResolver only allows mutation when we’re not in speculative import resolution. This ensures we can’t accidentally mutate the resolver during this process, which is important as we move towards a batched resolution algorithm.

This also changes functions that are used during speculative import resolution to take a CmResolver instead of a &mut Resolver.

Also introduces a new kind of "smart pointer" which has the behaviour described above:

/// A wrapper around a mutable reference that conditionally allows mutable access. pub(crate) struct RefOrMut<'a, T> { p: &'a mut T, mutable: bool, } type CmResolver<'r, 'ra, 'tcx> = RefOrMut<'r, Resolver<'ra, 'tcx>>;

r? petrochenkov

[petrochenkov]

Looks good to me at high level.

Although I expected to use

impl Resolver { fn method(self: SmartResolver) { ... } }

instead of

impl SmartResolver { fn method(&mut self) { ... } }

(It would also allow to avoid moving code and make the diff readable.)

[LorrensP-2158466]

I though arbitrary self types were not allowed? Only std smartpointers and Pin.

[petrochenkov]

There's feature(arbitrary_self_types) for using arbitrary types for smart pointers, and the compiler can use unstable features.

[petrochenkov]

The current scheme of constructing SmartResolver::Finalize is very brittle, I think.
We can very easily construct it without providing any proofs that we are not actually called from resolve_import.

I think the speculative/finalize flag should live in Resolver after all, and be set/cleared only in fn resolve_imports for now.
And then SmartResolver would never be constructed manually, only through resolver.smart(), where smart is

fn smart(&mut self) -> SmartResolver { if self.speculative { SmartResolver::Speculative(self) } else { SmartResolver::Finalize(self) } }

And then every resolver method taking &mut self should take SmartResolver instead.

Of course there may be some temporary escape hatch until resolver_import is fully migrated from mutating resolver to producing outputs.

[LorrensP-2158466]

The current scheme of constructing SmartResolver::Finalize is very brittle, I think.

I very much agree.

Of course there may be some temporary escape hatch until resolver_import is fully migrated from mutating resolver to producing outputs.

Currently, mutating this flag shouldn't be a problem, since resolve_imports takes a &mut Resolver. This would need to be converted to a cell once we convert this to batched import resolution.

[LorrensP-2158466]

And then every resolver method taking &mut self should take SmartResolver instead.

Also, this seems a bit extreme, I'm sure not every &mut self should be a SmartResolver.

[petrochenkov]

And then every resolver method taking &mut self should take SmartResolver instead.

Also, this seems a bit extreme, I'm sure not every &mut self should be a SmartResolver.

How are you going to guaranteed that this &mut method is not called from resolve_import?
If it doesn't use SmartResolver, then it should run an assert checking that resolver.speculative is false.
Upd: we didn't get to it yet, but when import resolution is actually parallelized, non-SmartResolver methods probably just won't pass borrow-send-sync checking.

[LorrensP-2158466]

How are you going to guaranteed that this &mut method is not called from resolve_import?
If it doesn't use SmartResolver, then it should run an assert checking that resolver.speculative is false.
Upd: we didn't get to it yet, but when import resolution is actually parallelized, non-SmartResolver methods probably just won't pass borrow-send-sync checking.

Good point, didn't think of it like that. It is going to take some time.

[LorrensP-2158466]

So we'd only call res_mut() when we actually mutate the resolver, like:

self.res_mut().some_field = some_value;

[petrochenkov]

Hmm, but on the other hand if fn resolve_import takes a smart resolver, then the only way to obtain &mut self in it would be to call res_mut at some point, so you cannot get it in speculative mode.

I guess it's not necessary to convert everything then, just convert enough functions directly or called by resolve_import to avoid panics caused by calling res_mut unconditionally before them rather than conditionally inside them.

[LorrensP-2158466]

I converted some more to use a SmartResolver and I also checked every call for res_mut. They should be fine.

mutable resolver usages in resolve_import are now annotated with fixmes.

Still wrapped (single,multi)_segment_macro_resolutions in RefCells just to be sure, but also annotated them with Fixmes since they should be outputs of speculative resolution.

@rustbot ready

[petrochenkov]

Also need to update the PR title and description.

^^^

[petrochenkov]

r=me after squashing commits.
@rustbot author
@bors delegate+

[bors]

✌️ @LorrensP-2158466, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, please make that change, then do @bors r=@petrochenkov

[LorrensP-2158466]

@bors r=@petrochenkov

[bors]

📌 Commit 487e5ce has been approved by petrochenkov

It is now in the queue for this repository.