Do two passes of handle_opaque_type_uses_next

[jackh726]

Fixes rust-lang/trait-system-refactor-initiative#240

Also did a little bit of cleanup, can squash the commits if decided.

r? lcnr

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[lcnr]

I feel conflicted about this change and slightly dislike it.

I like having apply_definition_site_hidden_types in a separate function. It more closely matches the setup in MIR borrowck, so it feels more consistent.

I guess you don't want to equate things again if you've already had a defining use in a previous iteration. We actually kind of need to do so in case we end up with new non-defining uses since a previous call to try_handle_opaque_type_uses_next.

I wouldn't worry about the perf of equating a few opaque types too frequently.

Idk if you've got strong opinions here yourself, but I would prefer to keep it as two functions 🤔

[jackh726]

I want to push back on this comment a bit. I definitely understand wanting to mirror MIR borrowck a bit, but the work done here is much simpler than the work done there - so it's not entirely a 1:1 comparison.

Second, there is no change here in when we equate opaque type. It's just a change in the flow of thought:

before:

• for each opaque ty, search through possible defining uses, find the first valid one
• store it
• if none, error
• separately, for each opaque ty use site, lookup the defining use, and equate the current hidden ty with that

here:

• for each opaque ty, search through possible defining uses, find the first valid one
• equate all other hidden tys with that defining use
• store it
• if none, error

Basically, rather than going through this "store the hidden ty then later look it up for every use", we equate immediately after identifying it - for me this is much more understandable (which is why I did this refactor)

There is a slight perf benefit, in that we don't do n hashmap looks (for each opaque type use site), but like you said, that isn't something too much to worry about and was not the motivation for this change.

Actually, MIR borrowck could be written in the same way (actually, briefly skimming the code there, it seems like it could be cleaner too, since there we're even emitting a NonDefiningUseInDefiningScope error in apply_definition_site_hidden_types, even though I would expect an invariant of compute_definition_site_hidden_types to be a hidden type (or error). I'm happy to make that change separately, but I am not convinced that we need to match that structure here.

[jackh726]

Actually, MIR borrowck could be written in the same way (actually, briefly skimming the code there, it seems like it could be cleaner too, since there we're even emitting a NonDefiningUseInDefiningScope error in apply_definition_site_hidden_types, even though I would expect an invariant of compute_definition_site_hidden_types to be a hidden type (or error). I'm happy to make that change separately, but I am not convinced that we need to match that structure here.

Taking a deeper look at this now, I didn't quite grok the MIR borrowck code on first glance. I'm not fully convinced that there isn't a cleanup that could be done there (but it's certainly not as simple as I thought), but I'm even more convinced that mirroring simple function names is not all that helpful. MIR borrowck handling of opaque types is much more complex than HIR typeck, and trying to mirror them just makes HIR typeck harder to understand.

[jackh726]

@lcnr I fixed your first three comments. The rename from first_pass to error_on_missing_defining_use kind of just made me realize that assert!(prev.is_none) is just kind of not needed, so I added a comment explaining as such. Your last comment, I do have strongish feelings about, replied on that. Curious on your thoughts.

[lcnr]

given that we only ever use the opaques in compute_definition_site_hidden_types, just move this into that function as well, or even have a single handle_opaque_type_uses_next(HandleOpaqueTypeUses::ErrorOnMissingDefiningUse9

[lcnr]

we can also just go from no uses to a defining use due to fallback, e.g. if you have soem ?will_fallback: Trait<opaque> goal

[lcnr]

please also/instead add the more minimized tests from rust-lang/trait-system-refactor-initiative#240

View changes since this review