Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add integrity checking to tests and fix various bugs #126

Merged

Conversation

alangpierce
Copy link
Collaborator

Fixes #115

This commit adds a new IntegrityCheckingMagicString class that subclasses
MagicString and does an integrity check operation, which is used in test code.

This revealed 5 bugs, all of which are also fixed in this commit:

  • Within indent, the code was calling chunk.split, then making an attempt at
    properly updating byStart and byEnd. But it wasn't properly updating
    lastChunk. Rather than calling chunk.split, we can just call
    this._splitChunk, which updates byStart, byEnd, and lastChunk
    correctly.
  • move could sometimes assign undefined as a next pointer, which is
    inconsistent since all other cases use null. Not a really big deal, but nice
    to be consistent.
  • overwrite was attempting to do a linked list removal in a way that wasn't
    properly updating all state ( overwrite method can cause state corruption #115 ). I tried changing the code to remove the
    nodes properly, but the details seemed to get really subtle, especially
    accounting for moved chunks, and it didn't seem like the complexity was worth
    it, so I effectively just reverted optimisation - remove empty chunks when overwriting #113 to fix the issue. But now with the
    new integrity checking, it should be a lot easier to get the details right if
    that's desirable.
  • trimStart and trimEnd were attempting to update byStart and byEnd, but
    they didn't properly set byEnd of the newly-created chunk created by
    chunk.trimStart/chunk.trimEnd. Setting the byEnd for that new chunk
    seems to fix things.
  • trimEnd was setting this.lastChunk = chunk.next, which makes sense for the
    first iteration (when working with the very last chunk in the linked list),
    but doesn't make sense for later iterations, since it meant that some chunks
    just get dropped. We now only run that line on the last chunk.

Fixes Rich-Harris#115

This commit adds a new `IntegrityCheckingMagicString` class that subclasses
`MagicString` and does an integrity check operation, which is used in test code.

This revealed 5 bugs, all of which are also fixed in this commit:
* Within `indent`, the code was calling `chunk.split`, then making an attempt at
  properly updating `byStart` and `byEnd`. But it wasn't properly updating
  `lastChunk`. Rather than calling `chunk.split`, we can just call
  `this._splitChunk`, which updates `byStart`, `byEnd`, and `lastChunk`
  correctly.
* `move` could sometimes assign `undefined` as a `next` pointer, which is
  inconsistent since all other cases use `null`. Not a really big deal, but nice
  to be consistent.
* `overwrite` was attempting to do a linked list removal in a way that wasn't
  properly updating all state ( Rich-Harris#115 ). I tried changing the code to remove the
  nodes properly, but the details seemed to get really subtle, especially
  accounting for moved chunks, and it didn't seem like the complexity was worth
  it, so I effectively just reverted Rich-Harris#113 to fix the issue. But now with the
  new integrity checking, it should be a lot easier to get the details right if
  that's desirable.
* `trimStart` and `trimEnd` were attempting to update `byStart` and `byEnd`, but
  they didn't properly set `byEnd` of the newly-created chunk created by
  `chunk.trimStart`/`chunk.trimEnd`. Setting the `byEnd` for that new chunk
  seems to fix things.
* `trimEnd` was setting `this.lastChunk = chunk.next`, which makes sense for the
  first iteration (when working with the very last chunk in the linked list),
  but doesn't make sense for later iterations, since it meant that some chunks
  just get dropped. We now only run that line on the last chunk.
@Rich-Harris Rich-Harris merged commit 5766177 into Rich-Harris:master Jul 7, 2017
@Rich-Harris
Copy link
Owner

This is awesome, thanks so much! Released this as 0.22.

How would you feel about having admin access to this repo? I think it's reasonably stable at this point, but increasing bus factor is never a bad idea. No pressure though, totally your call 😀

@alangpierce
Copy link
Collaborator Author

Sure, that would be great. 😃

@Rich-Harris
Copy link
Owner

done! welcome aboard 🎉 Added you as an owner of the npm package as well, in case you need to cut a release and I'm on vacation/whatever

@alangpierce alangpierce deleted the add-integrity-checking-fix-bugs branch July 7, 2017 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

overwrite method can cause state corruption
2 participants