Add per query cycle handlers and use them for representability#153028
Add per query cycle handlers and use them for representability#153028Zoxc wants to merge 1 commit intorust-lang:mainfrom
Conversation
rustbot
added
A-query-system
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rustbot reroll |
There was a problem hiding this comment.
Ah yea, that's actually better than dispatching on types. I think we can nuke most if not all fallbacks with such a scheme.
Still not sure we can make all query cycles fatal, but worst case we make the cycle handlers return the query return type, too
| mod ty; | ||
|
|
||
| pub fn provide(providers: &mut Providers) { | ||
| abi::provide(providers); |
There was a problem hiding this comment.
Please do changes like this in a separate commit
| for variant in tcx.adt_def(def_id).variants() { | ||
| for field in variant.fields.iter() { | ||
| rtry!(tcx.representability(field.did.expect_local())); | ||
| let _ = tcx.representability(field.did.expect_local()); |
There was a problem hiding this comment.
The let _ is not necessary for unit returning queries
| /// Checks whether a type is representable or infinitely sized | ||
| query representability(key: LocalDefId) -> rustc_middle::ty::Representability { | ||
| /// Checks whether a type is representable or infinitely sized, emits a cycle error if it's not. | ||
| query representability(key: LocalDefId) { |
There was a problem hiding this comment.
Should probably be check_representable or sth now
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
☔ The latest upstream changes (presumably #153047) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Zoxc could you please explain how did you come up with this idea? |
|
Or just clarify what you did or did not come up with on your own? |
I think this is a good goal, could make a lot of things simpler. I don't understand a lot of what's going on in this PR though. There is some new stuff added (why?) and lots of code moved around, all in a single commit. @zetanumbers: Do you have thoughts on the implementation approach? If you wanted to remove query cycle recovery, how would you go about it? |
I am not sure if "removing query cycle recovery" also means removing break_query_cycles. If it does then I would like to hear what plan does @Zoxc have for it. I have major concerns about it. And "making all cycle errors fatal" would require untangling all of diagnostics code relying on query cycle detection. It might also require drawbacks to diagnostics or added complexity to the query system as it is written the way it is for some reason, that's the "new stuff added". So I don't see this goal to be inherently positive and required changes should be judged case by case. But maybe Zoxc has some insight about other queries too. These are my general concerns. I should give it a closer look before giving a proper guidance for further changes. We should check if we lost any information by removing those diagnostic messages. |
|
I don't like the fact that I wouldn't have personally bought this partial change for myself, as I doubt that reworking these diagnostics would be possible without some substantial drawback. On the other hand if we had all of required code to reach this goal, that could be a pretty sweet deal. Also I think I've figured what Zoxc really wants to do there. I would like @Zoxc to give explanation for himself, so please elaborate a bit on this part: Although I am struggling to understand why this loop stops on the first cycle handler to return |
6 participants
This adds a per query cycle handler where queries which are part of a cycle can choose to emit a suitable cycle error instead of the default cycle error format.
The
representabilityquery is changed to make use of it instead of doing it usingValuewhich is not intended for that purpose.The
representabilityquery is also changed so that the error is fatal, so we no longer resume after recursive type errors. This improves error output for our tests since we no longer get multiple related cycle errors.This is a first step towards removing query cycle recovery and making all cycle errors fatal.