+ Source::TreeRewriter: Improved merging and representations

[marcandre]

This PR provides TreeRewriter with:

• improved merging (allowing different ProcessedSource or offsetting)
• two representations of the rewriter (a basic one and a complete one)

Background:
I was looking at erb-lint and they do some processing on some excerpts of erb files. Later, they want to combine the rewriting actions on the original erb (with an offset).
With the upcoming RuboCop 1.0 this would be tricky, and this made me realize that the two avenues I was thinking about were not available (transferring a TreeRewriter with an offset, or at least having a representation of the TreeRewriter so we could recreate the actions on a different source and with an offset). This PR allows both avenues.

I added a minor doc change too. Let me know if I should split this into different PRs

[iliabylich]

Thanks a lot, looks great overall! A few nits/questions.

One more question: currently if you merge rewriters that belong to different source buffers you get a RuntimeError (that is more like a sanity check). Your PR seems to turn rewriters into some kind of a giant anonymous action object that can be applied on arbitrary source buffer. Is that correct?

I've never contributed to any rewriters-related classes, so this part of the gem is still a mystery to me

[iliabylich]

I'd probably inherit TestSourceTreeRewriterMerge from TestSourceTreeRewriter (or there could be a single base class). Dependencies between test classes are unusual.

[marcandre]

I don't know Minitest well (and don't intend to, I'm too much a fan of rspec), but that's what I did first and the tests of TestSourceTreeRewriter get executed twice...

[marcandre]

If you prefer I could simply add the extra setup to the first part. I miss RSpec!

[iliabylich]

Right, minitest simply invokes all /test_.*/ methods that are defined twice because of the inheritance (I'm sure it could to instance_methods(false) to drop inherited methods, no idea why it's designed this way). However, creating a base class with a setup method (and inheriting 2 test classes from it) could work. Let's keep it as is

[marcandre]

currently if you merge rewriters that belong to different source buffers you get a RuntimeError (that is more like a sanity check). Your PR seems to turn rewriters into some kind of a giant anonymous action object that can be applied on arbitrary source buffer. Is that correct?

Yes, that's exactly it. It usually doesn't make any sense to apply a rewriter to different sources, but erb_lint gave me a very valid use case.

I've seen cases of mixed sources in RuboCop but these were sloppy test setup where two different source buffers were prepared from the same code (so not a real issue), or cache not being invalidated (more of an issue).

You make me realize that it's probably best to have either a different method (moved ?) or an argument (allow_mixed_sources: false). Let me refactor this a bit.

[marcandre]

So I'm proposing to add #import! and keep #merge! as it was. I don't think that there's a need for #import (which would just be dup.import!).

My first draft had a method a bit like moved that add source_buffer and offset as arguments, but also needed policy and had to call self.class.new which is never great as it complicates subclassing (which we do in RuboCop now), and there's the diagnostic that's not part of the constructor, etc. Easier to have something like import! which mutates an already created object.

What do you think?

[marcandre]

Tests crash with TruffleRuby, with a nice cryptic error...

[iliabylich]

It says

Fatal error: java.lang.OutOfMemoryError: Could not allocate an aligned heap chunk

@eregon Could you take a look please? Are there are additional JVM options that we could set to fix it?

Also, does JVM (just like MRI) run GC when malloc (or some wrapper around it) fails to get memory? If so, does it mean that we have a test that consumes too much memory so it causes oom error on truffleruby? Not sure how's it possible.

Here's the link to the failed build - https://travis-ci.org/github/whitequark/parser/jobs/696562301, I'll rerun it to see if it's consistent.

EDIT: oof, I should've restarted the whole build. Sorry, the backtrace has gone.

[marcandre]

@eregon There was also talk about an "implicit exception raised when an explicit one would be fatal" or something. That made my head spin 😅

[iliabylich]

And it's green now. I guess I'll move truffleruby to allow_failures group once it happens again.

So I'm proposing to add #import! and keep #merge! as it was. I don't think that there's a need for #import (which would just be dup.import!).

I agree.

Is there anything you'd like to change? If not I'll merge it

[marcandre]

LGTM, thanks 😄

Thanks for the review. I need to tell you that your reviews have been the most in-depth, precise and on-point I remember having 🙇

[eregon]

Seeing this now. Sorry about the failure.
Don't hesitate to report an issue on https://github.com/oracle/truffleruby/issues if you see a failure like "implicit exception raised when an explicit one would be fatal" or OutOfMemoryError.

OutOfMemoryError could be caused by excessive memory usage by the program, but also by internal issues, or by maybe representing some objects inefficiently in memory, in any case worth reporting if it doesn't NoMemoryError on MRI.