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 #8642] Fix a false negative for Style/SpaceInsideHashLiteralBraces when a correct empty hash precedes the incorrect hash #8649

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Sep 4, 2020

Fixes #8642.

I'm not 100% sure I understand what ConfigurableEnforcedStyle is doing (it'd be great if it could have more documentation added!), but the problem here is that the detected style was flipping between space and nospace depending on which hash was being evaluated. The fix is to just pass the defined style to ambiguous_or_unexpected_style_detected rather than trying to calculate it.


Consider the following code (which is also added as a test):

{}
{key: 1}
  1. When the first empty hash is evaluated by the cop, the memoized detected style (in ConfigurableEnforcedStyle) is nil. Because the spacing for the empty has is correct, correct_style_detected is called, which sets detected_as_strings and updated_list to ['space'] (since that is the EnforcedStyle value).
  2. When the first brace of the second hash is evaluated by the check method, it detects an offense and runs incorrect_style_detected. At this point, a space is required no_space is passed to ambiguous_or_unexpected_style_detected. Because of this, updated_list is set to the intersection of [space] and [no_space], which is [], and no_acceptable_style! is called. The offense gets registered (and corrected).
  3. When further hash braces are encountered, no_acceptable_style? returns true, and style_detected short-circuits. This causes incorrect_style_detected to return before adding an offense, and all further hash spacing issues are ignored in the file.

My change is to not override style but pass in the style from config on both correct_style_detected and incorrect_style_detected, which prevents no_acceptable_style? from short-circuiting further offense detection. None of the other code or existing tests had to be changed.


Before submitting the PR make sure the following are checked:

  • 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.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@dvandersluis dvandersluis force-pushed the fix-space-inside-hash-literal-braces-false-negative branch from 1d50670 to d17744b Compare September 8, 2020 15:29
@dvandersluis
Copy link
Member Author

This was a regression in 0.90 btw, it worked fine in 0.89.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 9, 2020

Did you manage to identify which commit introduced the regression?

@dvandersluis
Copy link
Member Author

@bbatsov I ran git bisect on it, and 6c05f69 is the first bad commit.

@dvandersluis dvandersluis force-pushed the fix-space-inside-hash-literal-braces-false-negative branch from d17744b to 41d2162 Compare September 9, 2020 14:32
@dvandersluis
Copy link
Member Author

@bbatsov I think it's because of 6c05f69#diff-434a2611de47b29849cbf58aeb8094aeL141-R134

Before ambiguous_or_unexpected_style_detected was only called inside the add_offense block, but now it's called every time incorrect_style_detected is called.

@dvandersluis dvandersluis force-pushed the fix-space-inside-hash-literal-braces-false-negative branch from 41d2162 to 40ae7e9 Compare September 10, 2020 04:51
Copy link
Member

@koic koic left a comment

Choose a reason for hiding this comment

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

This regression is due to my mistake 💦 Thank you for the fixing. Can you rebase with the latest master branch?

…eralBraces` when a correct empty hash precedes the incorrect hash.
@dvandersluis dvandersluis force-pushed the fix-space-inside-hash-literal-braces-false-negative branch from 40ae7e9 to 6a3fab2 Compare September 10, 2020 17:32
@dvandersluis
Copy link
Member Author

@koic of course! done.

@koic koic merged commit d9204ec into rubocop:master Sep 10, 2020
@koic
Copy link
Member

koic commented Sep 10, 2020

@dvandersluis Thanks!

@getaaron
Copy link

getaaron commented Sep 13, 2020

@koic @dvandersluis @bbatsov thank you for fixing this so quickly! would it be possible to cut a new release 0.91 with this fix kinda soon? it's fairly frustrating

@dvandersluis dvandersluis deleted the fix-space-inside-hash-literal-braces-false-negative branch January 18, 2021 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants