Conversation
There was a problem hiding this comment.
Thanks for splitting in into two commits, that made the review easier.
This looks good! Simpler than what I proposed.
specialize_query_vtablesmodifies the necessary vtable entries. This puts some per-query initialization information in a different place to thequerydefinitions. I don't love this, but it avoids having to change the proc macro at all.- It doesn't change the signature of the
value_from_cycle_errorfunction, so it doesn't affectCycleErrorHandlingor thecycle_*query modifiers.
The macro and signature changes could still be done as follow-ups, so I think it's fine that they weren't done in this PR.
Good things:
- It gets rid of the specialization and the associated
unsafeblocks and the leaks. - It makes the connection between specific queries and the from_cycle_error behaviour clearer -- you no longer have to look at the type and then see which queries have that as their return type.
Some minor suggestions below, but overall I'm happy with this.
| vtables.variances_of.value_from_cycle_error = | ||
| |tcx, cycle, guar| erase_val(variances_of(tcx, cycle, guar)); | ||
| vtables.layout_of.value_from_cycle_error = | ||
| |tcx, cycle, guar| erase_val(layout_of(tcx, cycle, guar)); |
There was a problem hiding this comment.
Can you put a blank line between each entry? It's currently quite dense and hard to read.
|
|
||
| vtables.fn_sig.value_from_cycle_error = |tcx, cycle, guar| erase_val(fn_sig(tcx, cycle, guar)); | ||
| vtables.check_representability.value_from_cycle_error = | ||
| |tcx, cycle, guar| erase_val(check_representability(tcx, cycle, guar)); |
There was a problem hiding this comment.
check_representability aborts, so there is no need to erase_val here. Also, check_representability's return type can be changed to ! and Representability can be removed.
|
Also, can you expand the commit message on the first commit? Just a couple of sentences to explain the current situation, and how the commit changes things, and why that's an improvement. Thanks. |
|
And I'm wondering whether this overlaps with #149319 at all. That PR fixes two bugs, are those bugs relevant here? |
It does overlap, however those bugs were fixed by passing a query key as an argument to the
I have restrained myself from this change to minimize this PR. |
Fair enough. Just a few comments to address and then we can merge. |
Currently `QueryVTable`'s `value_from_cycle_error` function pointer uses the `FromCycleError` trait to call query cycle handling code and to construct an erroneous query output value. This trick however relies too heavily on trait impl specialization and makes those impls inconsistent (which is bad AFAIK). Instead this commit directly modifies `value_from_cycle_error` function pointer upon vtable initialization and gets rid of `FromCycleError`.
4d776a9 to
9418745
Compare
Ok, I've only squashed commits with suggested changes into the first commit and added some info to its commit message. Should be good now. @rustbot ready |
Currently
QueryVTable'svalue_from_cycle_errorfunction pointer uses theFromCycleErrortrait to call query cycle handling code and to construct an erroneous query output value. This trick however relies too heavily on trait impl specialization and makes those impls inconsistent (which is bad AFAIK). Instead this PR directly modifiesvalue_from_cycle_errorfunction pointer upon vtable initialization and gets rid ofFromCycleError.Removal of
FromCycleErrormight also be a desired change for some developers involved in the query system development due to some other reasons.This PR is split up into two commits. Code formatting changes are isolated into the second commit for better git diffs in the first one. Nice and simple refactor PR.
I got this idea from #153470 (comment) discussion and thought it would be a pretty quick change to implement.
r? @nnethercote
I think this PR would benefit from your reviewer polish.