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.

Sign up for GitHub

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

Jump to bottom

Make RuboCop runner composable and capture any stdout message from it #275

Closed
wants to merge 3 commits into from
Closed

Make RuboCop runner composable and capture any stdout message from it #275

wants to merge 3 commits into from

Conversation

paracycle
Copy link
Member

@paracycle paracycle commented Aug 31, 2022 •
edited

Motivation

RuboCop is still too chatty and breaks the protocol by printing to stdout despite our best attempts. Today I was having problems with the formatting on Tapioca and realized it was because of the:

An error occurred while Layout/BlockEndNewline cop was inspecting /Users/ufuk/src/github.com/Shopify/tapioca/lib/tapioca/static/rbs_converter.rb:166:8.
To see the complete backtrace run rubocop -d.

problem (which is in pre-1.34 versions of RuboCop).

I am not exactly sure why this was causing a problem, since that specific message should be going to stderr, but the LSP communication was still broken because of it.

Regardless, I realized that we should start capturing the stdout from any RuboCop runs.

Implementation

In order to do this in an encapsulated and uniform manner, I realized that we were missing a layer of abstraction between RuboCop::Runner and our request classes that depend on it. I then realized that the problem was stemming from the fact that we've been using inheritance rather than composition for that part of the codebase.

So, I did a refactor to extract a composable RuboCopRunner that both the RuboCopDiagnosticsRunner and RuboCopFormattingRunner are able to use. That resulted in a much better and easier to read implementation for both of those classes.

Afterwards, I implemented a capture_output method in the RubocopRunner which I made all invocations to RuboCop::Runner#run wrapped in. This method does the following:

  1. Captures (and throws away) $stdout
  2. Captures (and throws away) $stderr
  3. Turns off Ruby $VERBOSE flag so that Kernel#warn messages are never printed.

This should make sure that we never print anything that we don't control to stdout/err anymore.

Finally, I think showing any errors that were encountered (and handled) during RuboCop processing is helpful, so I made RuboCopRunner explicitly print uniq errors to stderr.

Automated Tests

I haven't added any but it might be nice to maybe add one.

Manual Tests

Not sure how, but if you can recreate the conditions of rubocop/rubocop#10878 on a repo that is using RuboCop 1.33 or earlier, then it is enough to trigger the broken behaviour and to see that this PR fixes it.

@paracycle paracycle requested a review from a team as a code owner August 31, 2022 14:27
@st0012
Copy link
Member

st0012 commented Aug 31, 2022

If this is mainly for rubocop, maybe we can add a comment to mention that?
And if it's not and we want to capture all stderr, we need to take care of usages of STDERR as well. Because reassigning $stderr won't affect the libs that use STDERR.

irb(main):001:0> require "stringio"
=> true
irb(main):002:0> $stderr = StringIO.new
=> #<StringIO:0x00000001091f7a70>
irb(main):003:0> $stderr.puts("123")
=> nil
irb(main):004:0> STDERR.puts("123")
123
=> nil

@vinistock
Copy link
Member

TIL about the STDERR difference. To add a unit test for this, you can register a handler in the queue_test that prints something and verify that it didn't actually print anything.

@paracycle
Make RuboCop runner composable instead of inheritance
248db71 
The current implementation of running RuboCop tasks is not composable.
Instead, we use inheritance to obtain and use the functionality of the
`RuboCop::Runner` class, which ends up coupling our interfaces too
tightly to that class.

Instead, this commit creates a `RuboCopRunner` class that is meant to
encapsulate the functionality of the `RuboCop::Runner` class. Both the
formatter and the diagnostic request classes are refactored to use this
new class in a composable way, and the implementations are much nicer as
a result.

This new architecture also allows us to add common functionality to how
RuboCop runs are made.
@paracycle paracycle force-pushed the uk-make-sure-requests-dont-leak-to-stdout branch from b7bffbe to 0886be8 Compare August 31, 2022 19:36
@paracycle paracycle changed the title Make sure stdout is clean during all request processing Make RuboCop runner composable and capture any stdout message from it Aug 31, 2022
@paracycle
Copy link
Member Author

@st0012 Yeah, I know about the difference between STDERR/STDOUT and $stderr/$stdout. However, my goal is to catch legitimate uses of puts in general, which end up going to $stdout.

In any case, I changed my approach and implementation to limit the capture to only RuboCop and to capture only $stdout, which seems to working well for me. FYI @vinistock.

@paracycle paracycle force-pushed the uk-make-sure-requests-dont-leak-to-stdout branch from 0886be8 to d9edf5a Compare August 31, 2022 19:45
@paracycle
Start capturing all output during all RuboCop runs
26b787f 
Moreover, set `$VERBOSE` to `nil` so that we don't end up seeing messages
from `Kernel#warn` in the VSCode code Output pane as well.
@paracycle paracycle force-pushed the uk-make-sure-requests-dont-leak-to-stdout branch from d9edf5a to 0bc4b6f Compare August 31, 2022 21:04
@paracycle
Copy link
Member Author

Ok, it ended up being much more tricky and I had to change the $VERBOSE global as well. That flag controls if Kernel#warn is going to have an output or not and I propose that we set $VERBOSE = nil for the ruby-lsp executable overall in any case. I don't think we want random warning messages being sent to the Output tab.

WDYT?

@paracycle paracycle force-pushed the uk-make-sure-requests-dont-leak-to-stdout branch from 0bc4b6f to d604ef0 Compare August 31, 2022 21:15
@st0012
Copy link
Member

st0012 commented Sep 1, 2022

I propose that we set $VERBOSE = nil for the ruby-lsp executable overall in any case

I think that makes sense 👍

Regarding the implementation, I was a bit worried about swapping $stderr/out will affect the other thread's output as we can run 2 requests at a time now. But right now it's not going to happen because language_server-protocol-ruby uses STDOUT instead of $stdout.

However, it's still quite dangerous because if one day the STDOUT thing is changed, we can start experiencing weird race condition issues.

Morriar
Morriar reviewed Sep 1, 2022
View reviewed changes
lib/ruby_lsp/requests/support/rubocop_runner.rb
Comment on lines +83 to +85
$stdout = orig_stdout
$stderr = orig_stderr
$VERBOSE = original_verbosity
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, VERBOSE is first in the two previous related blocks.

Suggested change
$stdout = orig_stdout
$stderr = orig_stderr
$VERBOSE = original_verbosity
$VERBOSE = original_verbosity
$stdout = orig_stdout
$stderr = orig_stderr

Copy link
Member Author

Choose a reason for hiding this comment

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

I was explicitly reverting changes in the reverse order :)

lib/ruby_lsp/requests/support/rubocop_runner.rb
orig_stderr = $stderr

$VERBOSE = nil
$stderr = StringIO.new
Copy link
Contributor

Choose a reason for hiding this comment

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

How hard/bad would it be to monkey patch the classes from Rubocop that still print so we noop puts rather than changing the global variable?

vinistock
vinistock reviewed Sep 1, 2022
View reviewed changes
lib/ruby_lsp/requests/support/rubocop_runner.rb
attr_reader :offenses

DEFAULT_ARGS = T.let([
"--force-exclusion",
Copy link
Member

Choose a reason for hiding this comment

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

I assume we dropped the stderr option because we're now capturing stdout, but should we keep the formatter to reduce the amount of prints from RuboCop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we can still keep things less chatty overall. I'll reinstate.

lib/ruby_lsp/requests/support/rubocop_runner.rb
private

sig { void }
def display_handled_errors
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? What type of error is this trying to display? If we don't pop up a dialogue, I doubt anyone will actually see it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is so that people can see and/or report problems that any formatting/diagnostic operation has encountered. I feel like this is needed more for troubleshooting than making people aware. After all, there is no action to be taken on any of these, since they are already handled.

This is what the output looks like:

[RuboCop] Encountered and handled errors:
[RuboCop]   - An error occurred while Layout/BlockEndNewline cop was inspecting /Users/ufuk/src/github.com/Shopify/tapioca/lib/tapioca/static/rbs/parameter_converter.rb:4:17.

lib/ruby_lsp/requests/support/rubocop_runner.rb
def run(path, contents)
@errors = []
@warnings = []
@offenses = []
Copy link
Member

Choose a reason for hiding this comment

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

Can we use @offenses.clear instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was what I tried first, but it didn't work since it is a frozen array.

lib/ruby_lsp/requests/support/rubocop_runner.rb
Comment on lines +41 to +42
@errors = []
@warnings = []
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need these?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we don't recycle runners, but only use a single instance, errors and warnings need to cleared at the top of every run so that they don't keep building up run over run.

lib/ruby_lsp/requests/support/rubocop_diagnostics_runner.rb
def file_finished(_file, offenses)
@diagnostics = offenses.map { |offense| Support::RuboCopDiagnostic.new(offense, T.must(@uri)) }
sig { params(uri: String).returns(T::Array[Support::RuboCopDiagnostic]) }
def diagnostics(uri)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure there's a lot of value in keeping this method around after the refactor. What do you think about moving this to the run body?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we can do that!

@paracycle
Copy link
Member Author

Btw, I decided to close this PR and open another one that limits scope to only the RuboCop composition refactor and leave output capture to another date. I am no longer sure if that's the source of my problems.

@paracycle paracycle closed this Sep 1, 2022
@vinistock vinistock deleted the uk-make-sure-requests-dont-leak-to-stdout branch April 26, 2023 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Morriar Morriar Morriar left review comments

@vinistock vinistock vinistock left review comments

@KaanOzkan KaanOzkan Awaiting requested review from KaanOzkan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@paracycle @st0012 @vinistock @Morriar @KaanOzkan