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

The clear star of yesterday #2920

Merged
merged 16 commits into from
Mar 9, 2016

Conversation

alexdowad
Copy link
Contributor

Before submitting a PR make sure the following are checked:

  • Wrote good commit messages.
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

@alexdowad alexdowad force-pushed the the_clear_star_of_yesterday branch from 2376025 to 8186277 Compare March 5, 2016 20:52
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 6, 2016

Apart from the build failure, the changes look good.

@alexdowad alexdowad force-pushed the the_clear_star_of_yesterday branch from 8186277 to 11646f2 Compare March 6, 2016 20:15
@alexdowad
Copy link
Contributor Author

Still have to fix up a style violation.

Also profiled autocorrection of some very slow files from Rails. It turns out that AutocorrectUnlessChangingAST completely dominated runtime. It is clear that the whole approach of reparsing files to see if a correction is syntactically valid absolutely kills performance on large source files.

For autocorrection to be fast, we need to use specific checks to detect cases where autocorrection should not be done. AutcorrectUnlessChangingAST is not an option.

I have removed almost all uses of it. The only one left is Style/Not.

@alexdowad
Copy link
Contributor Author

Forgot to say: on one large test file from Action Pack which I was using for benchmarking, autocorrection is now more than 10x faster.

@@ -160,11 +160,11 @@
end

context 'in a method call without parentheses' do
it 'does not correct a hash parameter with trailing comma' do
it 'corrects a hash parameter with trailing comma' do
# Because `get :i, x: 1,` is invalid syntax.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems to suggest that your change produces invalid code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll delete the comment.

@alexdowad alexdowad force-pushed the the_clear_star_of_yesterday branch from 843f45f to 70da440 Compare March 7, 2016 06:17
@alexdowad
Copy link
Contributor Author

With this change, autocorrection of large and style-error-ridden projects like Rails is quite fast! My stress tests can be run comfortably.

The very first run of the autocorrection stress test has picked up a bunch of bugs. Just looking at a problem where TrivialAccessors creates a syntax error.

@alexdowad alexdowad force-pushed the the_clear_star_of_yesterday branch from 70da440 to c1706c8 Compare March 7, 2016 20:14
@alexdowad
Copy link
Contributor Author

Fixed up the broken build.

@alexdowad alexdowad force-pushed the the_clear_star_of_yesterday branch from c1706c8 to 6ddff47 Compare March 7, 2016 20:24
@alexdowad
Copy link
Contributor Author

Rebased on master.

@alexdowad
Copy link
Contributor Author

Turns out the TrivialAccessor problem which I discovered was caused by a bug in parser... caused by yours truly. Just sent a patch to parser.

@alexdowad alexdowad force-pushed the the_clear_star_of_yesterday branch from 6ddff47 to f0a705a Compare March 8, 2016 11:50
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 8, 2016

One commit was removed, right? Please rebase on top of master

When the enforced style is `semantic`, `check_for(:raise, ...)` meant that we
*don't* want to see `raise`. But with other styles, `check_for(:raise, ...)`
meant that we *do* want to see `raise`.

Make the usage consistent.
This helper had two different functions depending on the enforced style.
Split it so each helper method does just one thing.
…stom `fail` methods

If a custom method called `fail` is defined anywhere in a source file, calls to
`fail` within that file are not flagged, even if the enforced style is
`only_raise`. This prevents unwanted offenses in situations like:

    class Foo
      def self.fail(msg)
        # something
      end

      def self.foo
        fail 'message'
      end
    end
…ue in braceless hash

Previously, the use of `{}` would be flagged in:

    cr.stubs client: mock {
      expects(:email_disabled=).with(true)
      expects :save
    }

However, changing this to use `do...end` would change the meaning of the code,
since it would bind to the outer method call rather than the inner one.

The changes pushed this class over the line count limit, so a couple of tweaks
have been made to save on excess code size.
There is a good reason for this; using AutocorrectUnlessChangingAST imposes
an extremely high performance cost on autocorrection. Surprisingly, after
removing it from AndOr, all the tests still pass.

Likely, AndOr was not very smart before about which expressions to flag, so
AutocorrectUnlessChangingAST was used to stop it from breaking code. Now, it may
be smarter than before.

It is also possible that there are some cases where it will perform invalid
autocorrections. If that is the case, we can fix them when they are found
(and add corresponding test cases).
Again, this provides a significant performance boost for autocorrection.
Another performance boost for autocorrection.
Finally, with the main users of AutocorrectUnlessChangingAST removed,
autocorrection can really fly.

One very slow file (actionpack/test/controller/routing_test.rb) from Rails,
which I was using as a test case, is more than 10x faster now.
… autocorrecting

A Ruby method works exactly the same when an unused block argument is removed.
It also makes each call to the method faster.
@alexdowad alexdowad force-pushed the the_clear_star_of_yesterday branch from 3bd12c5 to e60ee2f Compare March 8, 2016 19:29
@alexdowad alexdowad force-pushed the the_clear_star_of_yesterday branch 2 times, most recently from 23bc2f4 to 3d87cbf Compare March 9, 2016 03:55
It uses parentheses as necessary to avoid changing the meaning of an expression,
and better yet:

    not a == b

...is corrected to:

    a != b

It is also much faster when autocorrecting, since AutocorrectUnlessChangingAST
is not used any more.
It was convenient, but it killed performance.
@alexdowad alexdowad force-pushed the the_clear_star_of_yesterday branch from 3d87cbf to 7bd4ccd Compare March 9, 2016 04:37
@alexdowad
Copy link
Contributor Author

And we're back to green.

bbatsov added a commit that referenced this pull request Mar 9, 2016
@bbatsov bbatsov merged commit 53b2b54 into rubocop:master Mar 9, 2016
@alexdowad alexdowad deleted the the_clear_star_of_yesterday branch March 9, 2016 05:54
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.

3 participants