repl.rb: guard against negative overhang

[DamienRobert]

When copy pasting into pry a code like this

foo do
	bar #note the tab here
end

then @indent.indent(val) replace the tab by two spaces, and so
overhang = original_val.length - indented_val.length is negative,
which yields an Exception in whitespace = ' ' * overhang.

Guard against this by taking the max with 0.

[kyrylo]

Hey @DamienRobert, thanks for the patch.

I noticed your code snippet is not valid Ruby. Is this on purpose? The description makes sense, however when I tried copy-pasting it in my pry but I don't see any difference between master and this patch. It still looks broken:

[1] pry(main)> def aaa
[1] pry(main)*   foo do
        bar #note the tab here
[1] pry(main)*     bar #note the tab herehere
[1] pry(main)*     donedone
[1] pry(main)*   end

That said, I don't see any exceptions. I'm running it on OSX. Is there any extra information to be shared?

[kyrylo]

I forgot to mention that if you use iTerm2, there's a cool feature, which lets you convert tabs to spaces:

[DamienRobert]

@kyrylo sorry, the code should be correct ruby, replace the 'done' by 'end'. I guess I wrote the commit message after writing some shell code...

Its strange that you don't get any exception, when I copy paste the snippet (even though it is not valid code) into pry, I get:
/data/dams/opt/pkgmgr/gems/gems/pry-0.11.3/lib/pry/indent.rb:392:in `*': negative argument (ArgumentError)

[kyrylo]

What's the Ruby version you're using?

[kyrylo]

Even when I paste the corrected version of the snippet, I still see broken output. If this is a bug on older Rubies, we probably need a test to confirm that.

[DamienRobert]

@kyrylo I use ruby 2.5.1p57, but this is definitively not a bug from ruby, but from pry; when overhang is negative, ' ' * overhang will always give "negative argument (ArgumentError)"

This pr is not about correcting the output, but preventing an exception in pry (which is very annoying since I need to relaunch pry and reload everything if it happens). The exception only occurs when Pry.config.correct_indent is true (and also Pry.config.auto_indent). Maybe this is not enabled in your config?

To correct the output, one would need to replace original_val.length by the space it takes into the screen, ie count every character as one except tabs which count up to the nearest multiple of 8. (Of course there are complications: wide unicode characters should count for 2, anssi sequences for 0, but we only would need a upper bound, it does not matter if correct_indent insert more white space than necessary.)

But I think it is more urgent to prevent the exception than to correct the displayed output.

[kyrylo]

Would it be possible to add a test for this? It's not obvious from the code how this can occur. If it's unachievable, I'd be inclined to have a comment instead.

[kyrylo]

Not trying to be nitpicky here but this code is executed frequently, therefore it's better to make it more performant. I believe if we used a temp variable with an if check it would accomplish that goal. What do you think?

[DamienRobert]

Sorry, but I am not sure I understand your review. This line is executed only once for every read, so I don't know how I could use a temp variable?

[kyrylo]

Sorry for not being clear enough. I think we could put the result of evaluation of original_val.length - indented_val.length into a temp variable, then check if it’s larger than 0. If it is, then we return it. If not, then we return nil. With this approach there’s no need to allocate an array. Does it make sense?

[DamienRobert]

Oh I see, indeed a benchmark shows that an if is slightly faster, but allocating a temporary variable is actually slower:

require 'benchmark'
array = (1..1000000).map { rand(10)-5 }
Benchmark.bmbm do |x|
  x.report("array") { array.each {|x| [x,0].max }}
  x.report("if")  { array.each {|x| x >= 0 ? x : 0  }}
  x.report("if temp var")  { array.each {|x| y=x; y=0 if y <= 0  }}             
end

                 user          system       total         real

array 0.048741 0.000000 0.048741 (0.048804)
if 0.047802 0.000000 0.047802 (0.047877)
if temp var 0.052751 0.000000 0.052751 (0.052818)

Anyway I don't really think microoptimising this code is really important (especially since it follows an io operation), so I changed my version to your suggestion.

[kyrylo]

Seems like you are right. Thanks for running this benchmark and sorry for the nitpicky comment.

[kyrylo]

I am sorry, I got confused and mixed issues together. Unfortunately, I still cannot replicate this error even when I explicitly enable correct_indent and auto_indent. Either way, I can see how this could be an issue, so it's better to patch that.

[DamienRobert]

Sure no problem! So your review is about another pr right?
I would have liked to understand why you cannot replicate this error. But in any case this patch is pretty safe and won't do any harm (and in my case fix an exception!)

[kyrylo]

So your review is about another pr right?

My review is still applicable to this PR, this stands the same :)

I would have liked to understand why you cannot replicate this error.

Heh, same here. I'll try to upgrade my Ruby to see if it happens but either way the review is still applicable.

[DamienRobert]

So your review is about another pr right?
My review is still applicable to this PR, this stands the same :)

Ok, I replied to your review, but I am not sure what you meant.

Heh, same here. I'll try to upgrade my Ruby to see if it happens but either way the review is still applicable.

What does the following do?

i=Pry::Indent.new
i.indent("def foo") #so that i.@indent_level=1
orig="\t'bar'"
indented=i.indent(orig) # => "  'bar'"
overhang=orig.length - indented.length # overhang = -1

This should replicate a case where overhang will be equal to -1.

By the way when you paste the snippet, do you paste it line by line or at once?
If you paste it line by line, due to pry auto indent feature the negative overhang won't happen.

To simulate a full paste, you can use this snippet

require 'pry'
class Repl < Pry::REPL
  def read_line(_prompt)
    <<-EOS
foo do
	bar #note the tab here
end
    EOS
  end
end
 Repl.new(Pry.new, {}).send(:read) #=> ArgumentError: negative argument

[kyrylo]

What does the following do? By the way when you paste the snippet, do you paste it line by line or at once?

I can now reproduce the problem this way. I used to paste at once but perhaps it depends on how your terminal handles this. I run iTerm 2 but even Terminal.app doesn't show any errors. Either way, thanks for the patch.

Have you considered this comment of mine?

Would it be possible to add a test for this? It's not obvious from the code how this can occur. If it's unachievable, I'd be inclined to have a comment instead.

You basically wrote that test in your last comment, we just need to put that to the test file 😄

P.S. Could you also rebase this on master? I see the "out-of-date branch" message by GitHub here, similarly to your other PR.

[DamienRobert]

I can now reproduce the problem this way. I used to paste at once but perhaps it depends on how your terminal handles this. I run iTerm 2 but even Terminal.app doesn't show any errors. Either way, thanks for the patch.

Ok, so it seems that these terminals split the paste on new lines, interesting...

You basically wrote that test in your last comment, we just need to put that to the test file smile

I used the ReplTester helper, but I had to do some convolutions (like mock tty?) to force the problematic code path to be taken. I tried and this test does fails without my fix.

P.S. Could you also rebase this on master? I see the "out-of-date branch" message by GitHub here, similarly to your other PR.

Done.

[kyrylo]

Can we mock using RSpec?
If I am not mistaken, this test modifies the outside environment, which we'd want to prevent.

[DamienRobert]

Can we mock using RSpec?

We could, but there is no reason to, since we don't want to test tty? here.

If I am not mistaken, this test modifies the outside environment, which we'd want to prevent.

No, ReplTester.start invokes redirect_pry_io input, output where output is (essentially) a StringIO.new. So this is a temp object that only lives during the duration of the ReplTester block, and I am only adding to its singleton class, so this won't change the outside environment.

[kyrylo]

I don't necessarily agree with this reasoning but I have to admit it's not a big deal.

[kyrylo]

This looks good to me. Could you rebase, please? Otherwise, I'll have to hit the "update branch" button, which will create a new junk commit. Happy to merge as soon as this is rebased.

[kyrylo]

Never mind, it turned out to be a setting in the project which requires PRs to be rebased on latest master. I disabled this check because it's very impractical (I have to bug contributors to rebase).

Thanks for your work 👍

[DamienRobert]

Yes I had rebased, but you pushed a new commit in the meantime :) Thanks for merging!