Duplicate cursor while working with Java visitors

[BoykoAlex]

The change in TreeVisitor:

            setCursor(cursor.getParent());

            if (topLevel) {
                sample.stop(Timer.builder("rewrite.visitor.visit").tag("visitor.class", getClass().getName()).register(Metrics.globalRegistry));
                visitCountSummary.record(visitCount);

                if (t != null && afterVisit != null) {
                    for (TreeVisitor<?, P> v : afterVisit) {
                        if (v != null) {
                             v.setCursor(new Cursor(cursor, tree)); // <---- This change
                            //noinspection unchecked
                            t = (T) v.visit(t, p);
                        }
                    }
                }

                sample.stop(Timer.builder("rewrite.visitor.visit.cumulative").tag("visitor.class", getClass().getName()).register(Metrics.globalRegistry));
                afterVisit = null;
                visitCount = 0;
            }

seems problematic and may lead to the creation of two cursors (child and parent) pointing to the same AST node.
The following call t = (T) v.visit(t, p); would create another cursor with the same tree element it seems:

    @Nullable
    public T visit(@Nullable Tree tree, P p) {
        if (tree == null) {
            return defaultValue(null, p);
        }

        Timer.Sample sample = null;
        boolean topLevel = false;
        if (visitCount == 0) {
            topLevel = true;
            sample = Timer.start();
        }

        visitCount++;
        setCursor(new Cursor(cursor, tree));

Having a duplicate cursor leads to problems with indents in the auto formatting visitor.

[BoykoAlex]

I propose to switch the cursor creation call:

from

v.setCursor(new Cursor(cursor, tree));

to

v.setCursor(cursor == null ? null : cursor.fork());

[knutwannheden]

@BoykoAlex Thanks for reporting. This seems to be about commit 3086e52. It indeed looks like the after visitor's cursor will end up with a parent with the same value.

@sambsnyd If I understand your commit correctly, you made a change to fix TreeVisitorTest#scheduleAfterOnVisitWithCursor(). But I think that what that test is doing is somewhat dangerous: Its visitor overrides visit(Tree, P). This is of course possible, but that method (in TreeVisitor) is the one which contains all the "important" bits about setting up the cursor etc. So I think a visitor doing that can't expect the cursor to point to the element being visited. In that way I think this test is wrong. What are your thoughts on this?

[knutwannheden]

@BoykoAlex Rather than calling Cursor#fork() I would like to propose to switch back to using the original cursor, as we had it before. I will update your PR accordingly.

[knutwannheden]

Using Cursor#fork(), as you propose, could also make sense in a second step to avoid any "pollution" between visitors.

[BoykoAlex]

@knutwannheden reverting the commit or switching to fork() either should fix the issue for me. thanks very much for looking into this!-)