oakmac / parinfer

Parinfer implementation in JavaScript
MIT License
11 stars 0 forks source link

bug in clampParenTrailToCursor #15

Open ggandor opened 3 years ago

ggandor commented 3 years ago

Shouldn't openers be at least a shallow copy here? (The same goes for the Python and Lua ports that implement v3.) I just looked into the Rust version as a sanity check, there they are indeed cloning the stack.

function clampParenTrailToCursor (result) {
      // ...     
      var openers = result.parenTrail.openers

      const openersLen = arraySize(openers)
      result.parenTrail.openers = arraySlice(openers, removeCount, openersLen)
      result.parenTrail.startX = newStartX
      result.parenTrail.endX = newEndX

      result.parenTrail.clamped.openers = arraySlice(openers, 0, removeCount)  // slicing from the mutated `openers`
      result.parenTrail.clamped.startX = startX
      result.parenTrail.clamped.endX = endX
      // ...
oakmac commented 3 years ago

Very possible!

When I did the Lua port recently I noticed that the tests were not checking the parenTrail.

I also noticed that this code related to parenTrail has no effect on the test suite.

I didn't do a deep dive on parenTrail; just made a mental note that there is probably a bug here that needs further evaluation.

shaunlebron commented 2 years ago

Javascript’s slice does a shallow copy, and every use of openers here is using that, so I think this is fine right?

shaunlebron commented 2 years ago

Were you thinking of splice which mutates the array?