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

Ignore fix for from_over_into if the target type contains a Self reference #10853

Merged
merged 3 commits into from Jun 4, 2023

Conversation

MarcusGrass
Copy link
Contributor

Fixes #10838.

This is my first time contributing here, and the fix is kind of ugly.
I've worked a bit with quote and was trying to figure out a way to replace the type in a better way than just a raw string-replace but couldn't quite figure out how to.

The only thing really required to fix this, is to replace all Self references with the type stated in the from variable, this isn't entirely simple to do with raw strings without creating a mess though.

We need to find and replace all Self's in a variable with from but there could be an arbitrary amount, in a lot of different positions. As well as some type that contains the name self, like SelfVarSelf which shouldn't be replaced.

The strategy is essentially, if "Self" is surrounded on both sides by something that isn't alphanumeric, then we're golden, then trying to make that reasonably efficient.

I would not be offended if the solution is too messy to accept!

changelog: [from_over_into]: Replace Self with the indicated variable in suggestion and fix.

@rustbot
Copy link
Collaborator

rustbot commented May 31, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 31, 2023
@Alexendoo
Copy link
Member

Tricky one for a first contribution 😄

We might be able to do it without re-lexing by getting the spans of all the Selfs and calculating the BytePos offset from the start of target_ty's span, but that has a bunch of its own problems and is also quite complicated

Maybe we should just stick

if !target_ty.find_self_aliases().is_empty() {
    return None;
}

in suggest_to_from, Self in that position should be relatively rare so it wouldn't be a huge deal to lose the suggestion, the lint itself will still fire

@MarcusGrass
Copy link
Contributor Author

Tricky one for a first contribution smile

We might be able to do it without re-lexing by getting the spans of all the Selfs and calculating the BytePos offset from the start of target_ty's span, but that has a bunch of its own problems and is also quite complicated

Maybe we should just stick

if !target_ty.find_self_aliases().is_empty() {
    return None;
}

in suggest_to_from, Self in that position should be relatively rare so it wouldn't be a huge deal to lose the suggestion, the lint itself will still fire

Sounds reasonable, now the change doesn't look like a 3AM, 5th-attempt, third-redbull leetcode submission.

Hopefully this was correct, I did the exit as early as possible, the stderr file is unchanged since the lint is just ignored, and the .fixed file keeps the original code, I think that's how it's supposed to look

@MarcusGrass MarcusGrass changed the title Add from_over_into replace for type in Self reference Ignore fix for from_over_into if the target type contains a Self reference Jun 1, 2023
@Alexendoo
Copy link
Member

Thanks @MarcusGrass!

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 4, 2023

📌 Commit b2c85b3 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 4, 2023

⌛ Testing commit b2c85b3 with merge 4886937...

@bors
Copy link
Collaborator

bors commented Jun 4, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 4886937 to master...

@bors bors merged commit 4886937 into rust-lang:master Jun 4, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auto fix failed converting Into to From with Self
5 participants