Construct Vector without an invalid internal state

[Jin-H]

when i debug vector's add behavior, i found out the reason why scala throw NPE is because the value of variable newFocus is negative. So accordingly, the code judges its depth is 0(the actual depth should be 2), which lead to a incorrect start position.

    //Vector.scala method appended() line 501
    val s = new Vector(startIndex - shift, endIndex + 1 - shift, newBlockIndex)
    s.initFrom(this)
    s.dirty = dirty
    s.shiftTopLevel(shiftBlocks, 0) // shift left by n blocks
    //which result this problem
    s.gotoFreshPosWritable(newFocus, newBlockIndex, newFocus ^ newBlockIndex)
    s.display0(lo) = value.asInstanceOf[AnyRef]

Thus i think the value of newFocus should be positive all the time, we only need to pay attention to its absolute value

Fixes scala/bug#11636

[Ichoran]

I'm quite worried that this would mask an underlying logic error rather than just fix the NPE. Why should the absolute value accurately describe what to do?

[Jin-H]

I am sorry to reply so late. The following is my opinion.

newBlockIndex 
= blockIndex - shift
= (endIndex & ~31) - (startIndex & ~((1 << (5 * (depth - 1))) - 1)) 
>= (endIndex & ~31) - (startIndex & ~31) > 0
//when depth == 2 
= (endIndex & ~31) - (startIndex & ~31)
//when depth > 2
> (endIndex & ~31) - (startIndex & ~31)

newFocus
= focus - shift
= focus - (startIndex & ~((1 << (5 * (depth - 1))) - 1)) 

then

s.gotoFreshPosWritable(newFocus, newBlockIndex, newFocus ^ newBlockIndex)

the value of newFocus ^ newBlockIndex just standard for the level, so I guess it should be positive.

def gotoFreshPosWritable0(oldIndex: Int, newIndex: Int, xor: Int): Unit = { // goto block start pos
      if        (xor < (1 <<  5)) { // level = 0
        // we're already at the block start
      } else if (xor < (1 << 10)) { // level = 1
       // ...
      } else if (xor < (1 << 15)) { // level = 2
       
      }
      //....
}

the following test case works well, and the ++ operator go through the same branch, and the value of variable newFocus is positive. I also know it will be just a coincidence.

@Test
  def constructProperStartIndex(): Unit = {
    val a = "A1" +: "A" +: "O" +: Iterator.continually("E").take(2078).foldLeft(Vector.empty[String])((v, e) => v :+ e) :+ "C" :+"C"
    val d = "O" +: Iterator.continually("E").take(0)   .foldLeft(Vector.empty[String])((v, e) => v :+ e) :+ "C"
    val right = a ++ d

    assertEquals(2085, right.map(_ => 0).size)
  }

ahh, could you give me some suggestions? @Ichoran

[szeiger]

@Ichoran can you take a look?

[Ichoran]

@szeiger - Unfortunately I'm incredibly busy through the end of the month. Was it @joshlemer who had worked on Vector recently? (Is this already fixed by some other PR? I thought i saw something come through that addressed Vector soundness issues.)

[joshlemer]

@Ichoran yes it was me who last touched Vector code and that probably caused this bug.

The error is almost certainly somewhere in here:
#7743

I am trying to figure out what the error is but it is quite tricky...

[joshlemer]

It may be best to just revert that PR for now, since that one was also responsible for this bug: scala/bug#11600 though that one has existing fixes up for consideration.

[joshlemer]

Strike that, update: the bug is not in that commit, the bug has something to do with these changes: #7588

(When I revert that change, the bug is gone)

Update: Yeah, the VectorBuilder changes in that commit are not quite right somehow, that's definitely the issue.

[szeiger]

I opened #8410 to revert the VectorBuilder optimization for now. Rescheduling this PR to 2.13.2 to give us some more time to get it right.

[SethTisue]

@szeiger this seems like a fix that really ought to make 2.13.2 if possible

that is, if we don't end up replacing Vector entirely: #8534

leaving it on the 2.13.2 milestone for now

[lrytz]

@szeiger should this PR be re-targeted at 2.12.x?

[joshlemer]

@lrytz so far as I understand, this is only a bug fix meant to address bugs that appeared in 2.13.0

[SethTisue]

obviated by #8534