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

[feedback requested] Detect void value expressions #12671

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

Conversation

pablobm
Copy link

@pablobm pablobm commented Feb 2, 2024

This is a WIP 🚧 hoping to get some feedback before I commit more effort. I know it doesn't have documentation, changelog, etc. I hope this is fine for the moment 🙏

Some interesting questions here would be:

  • Is there a reason why Rubocop doesn't have a cop for this?
  • Is there a desire to have a cop for this?
  • Any tips on how to handle this?

Detail

Recently I came across a new (to me) error in Ruby code similar to the following:

def void_assignment
  a = return 1
end

This code would fail with the following syntax error:

run.rb: run.rb:2: void value expression (SyntaxError)

So that's ok, I can understand it. Also it is an interesting one: despite being a syntax error, it's not flagged by Rubocop or ruby-parse. I'm assuming the interpreter catches it after building the AST, as opposed to during the parsing process itself? Whichever way it is, the tools I use (including Rubocop) don't flag it so I don't get to know it's happening until I run the code.

I suspect this is well known scenario and there's a good reason why Rubocop is not catching it. Is this correct?

Just in case, it piqued my curiosity and I had a go at it :-) This PR is a first version of what a cop for this might look like, if there's desire for one. I'm sure I'm missing something. For one, I know that next and break can also cause a void value expression, so that'd be something to add. And of course documentation.

I have also noticed that case statements appear immune to it, so I'm not raising offences on them. However I wonder if that's something that should be an option to the rule.

Additionally, I have noticed that different MRI and JRuby treat this differently, which makes sense. There are edge cases (noted in the spec) where JRuby is better at catching these cases. Also there are questions of what to do in cases that look wrong, but the interpreters I have tried don't complain (see also marked in the spec).

Checklist

Not fully checked as this is a WIP asking for feedback:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@pablobm pablobm changed the title [WIP] Detect void value expressions [feedback requested] Detect void value expressions Mar 8, 2024
@pablobm
Copy link
Author

pablobm commented Mar 8, 2024

@koic 👋 Not sure what the correct way to ask for feedback is 😅 I hope a ping is ok! Thank you in advance.

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.

None yet

1 participant