-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: Add recursion limit when hovering on const values #21504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
92c60bd to
d8073f8
Compare
This comment has been minimized.
This comment has been minimized.
d8073f8 to
7738031
Compare
This comment has been minimized.
This comment has been minimized.
|
I don't think it can be possible to create an infinite recursion. When evaluating yes, but not when rendering. |
|
@ChayimFriedman2 It shouldn't be possible, but it definitely is today unfortunately -- see the test added. Perhaps the type parameters aren't being substituted correctly? It looks like rendering is infinitely recursing on |
|
Didn't we have a similar issue with |
|
#16877 |
|
Ah, right, we didn't solve it I worked around it 😆 |
7738031 to
35dd559
Compare
This comment has been minimized.
This comment has been minimized.
|
I see there isn't any substitution in the environment here, that feels like it might be the problem? rust-analyzer/crates/hir-ty/src/display.rs Lines 940 to 944 in 2e35358
|
35dd559 to
a1efdaf
Compare
|
This fixes #21503
AI disclosure: I minimised the crash manually and wrote the unit test, then asked Claude to write a fix. I've reviewed all the changes and they seem reasonable.
I'm in two minds about this solution: the sample code shouldn't produce an infinite recursion as far as I can see, so maybe there's a better fix. However, I also suspect it'd be possible to write a genuine infinite recursion const function, in which case you wouldn't want r-a to stack overflow even though the code wouldn't compile.
Let me know what you think.