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

Implement code action to ignore unused Result value #3255

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leah-u
Copy link

@leah-u leah-u commented Jun 9, 2024

Closes #2960

My first contribution, please be gentle :3

I had to undo the changes from commit 64924b9 as my tests would otherwise panic with an integer underflow. I'm not really sure why it was necessary in the first place, maybe @giacomocavalieri can help?

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you! Fab work here!

We can avoid traversing the AST as the location of the discarded result is contained by the warning that would have been emitted during compilation. There's 2 ways we could get this information:

  1. Replace the warning emitter trait with just a vec of warnings that gets appended to. Then the language server can iterate over the vec once to see if there's any code actions it should offer. This is the best solution.
  2. Have the analyser also record the location of unused warnings in the analysed module data and have the language server use this data. This would be a temporary solution, but we already do something like this for unused imports.

Either of these approached would be fab.

let range = (start as usize)..(end as usize);
offset += end - start;
offset -= edit.new_text.len() as u32;
offset += end - start - edit.new_text.len() as i32;
Copy link
Member

Choose a reason for hiding this comment

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

Revert this please, this old version has it has a bug in it 🙏

Copy link
Author

Choose a reason for hiding this comment

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

The way it was before with offset being a u32, the subtraction on line 100 would panic with an integer underflow if the code action added more characters than it removed. I'm not sure how else to solve this. What exactly is the bug?

Copy link
Member

Choose a reason for hiding this comment

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

It would not apply the text edit correctly. The underflow likely means you have a bug and are trying to remove more content than exists.

@lpil lpil marked this pull request as draft June 20, 2024 10:40
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.

LSP code action: add let _ = to focused unused value
2 participants