Skip to content
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

Fix resolution for inherent array methods #10180

Merged
merged 3 commits into from
Sep 8, 2021
Merged

Fix resolution for inherent array methods #10180

merged 3 commits into from
Sep 8, 2021

Conversation

yotamofek
Copy link
Contributor

My second attempt at fixing #9992 , previous attempt was here: #10017 , but the logic was broken.

I know that this is not an ideal solution.... that would require, IIUC, a pretty big overhaul of the const generics handling in rust-analyzer. But, given that some of the array methods were/are being stabilized (e.g. rust-lang/rust#87174 ), I think it'll be very beneficial to rust-analyzer users to have some preliminary support for them. (I know it's something I've been running into quite a lot lately :) )

As far as my limited understanding of this project's architecture goes, I think this isn't the worst hack in the world, and shouldn't be too much of a hassle to undo if/when const generics become better supported. If the maintainers deem this approach viable, I'll want to add some comments, emphasizing the purpose of this code, and that it should be removed at some point in the future.

Copy link
Member

@flodiebold flodiebold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that seems acceptable to me. Can you extract the logic into two functions and add some comments explaining it?

bors d+

@bors
Copy link
Contributor

bors bot commented Sep 8, 2021

✌️ yotamofek can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@yotamofek
Copy link
Contributor Author

@flodiebold Done. Would love your opinion about the function names and comments, I can hardly understand what the code I wrote does, so I'm not sure the explanation is very clear 😛

@flodiebold
Copy link
Member

Seems ok, I can maybe try to improve them later a bit, but let's merge it for now 👍 Thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 8, 2021

@bors bors bot merged commit 108b080 into rust-lang:master Sep 8, 2021
@yotamofek yotamofek deleted the fix-array-method-resolution branch September 8, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants