Add failing test for cycle revalidation #257
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flodiebold
added a commit
to flodiebold/rust-analyzer
that referenced
this pull request
Mar 21, 2021
This allows us to handle more cases without a query cycle, which includes certain cases that rustc accepted. That in turn means we avoid triggering salsa-rs/salsa#257 on valid code (it will still happen if the user writes an actual cycle). We actually accept more definitions than rustc now; that's because rustc only ignores bindings when looking up super traits, whereas we now also ignore them when looking for predicates to disambiguate associated type shorthand. We could introduce a separate query for super traits if necessary, but for now I think this should be fine.
flodiebold
added a commit
to flodiebold/rust-analyzer
that referenced
this pull request
Mar 21, 2021
This allows us to handle more cases without a query cycle, which includes certain cases that rustc accepted. That in turn means we avoid triggering salsa-rs/salsa#257 on valid code (it will still happen if the user writes an actual cycle). We actually accept more definitions than rustc now; that's because rustc only ignores bindings when looking up super traits, whereas we now also ignore them when looking for predicates to disambiguate associated type shorthand. We could introduce a separate query for super traits if necessary, but for now I think this should be fine.
bors bot
added a commit
to rust-lang/rust-analyzer
that referenced
this pull request
Mar 21, 2021
8133: Ignore type bindings in generic_predicates_for_param (fix panic on ena and crates depending on it) r=flodiebold a=flodiebold This allows us to handle more cases without a query cycle, which includes certain cases that rustc accepted. That in turn means we avoid triggering salsa-rs/salsa#257 on valid code (it will still happen if the user writes an actual cycle). We actually accept more definitions than rustc now; that's because rustc only ignores bindings when looking up super traits, whereas we now also ignore them when looking for predicates to disambiguate associated type shorthand. We could introduce a separate query for super traits if necessary, but for now I think this should be fine. Co-authored-by: Florian Diebold <flodiebold@gmail.com>
Member
|
Hmm. I dont' have time to think hard about this right now (working through notifications) but I'll try to ping on salsa zulip |
Member
Member
|
We've fixed the bugs here and I think we have good test coverage. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following test hits the
unwrapin runtime.rs:415. I encountered this panic while working on rust-analyzer.As far as I understand,
report_unexpected_cycleassumes the query we hit the cycle on has to be executing right now, but that's not the case here: it's just being revalidated (throughvalidate_memoized_value). I'm not sure what the correct way to handle this is?