Skip to content

Commit

Permalink
Add integrity checking to tests and fix various bugs
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
alangpierce committed Jul 7, 2017
1 parent 46ea184 commit 8a85c8c
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 18 deletions.
1 change: 1 addition & 0 deletions .eslintrc
Expand Up @@ -6,6 +6,7 @@
"keyword-spacing": [ 2, { "before": true, "after": true } ],
"space-before-blocks": [ 2, "always" ],
"space-before-function-paren": [ 2, "always" ],
"space-in-parens": [ 2, "always" ],
"no-mixed-spaces-and-tabs": [ 2, "smart-tabs" ],
"object-shorthand": [2, "always" ],
"no-const-assign": 2,
Expand Down
34 changes: 17 additions & 17 deletions src/MagicString.js
Expand Up @@ -235,13 +235,9 @@ MagicString.prototype = {
if ( charIndex === chunk.start ) {
chunk.prependRight( indentStr );
} else {
const rhs = chunk.split( charIndex );
rhs.prependRight( indentStr );

this.byStart[ charIndex ] = rhs;
this.byEnd[ charIndex ] = chunk;

chunk = rhs;
this._splitChunk( chunk, charIndex );
chunk = chunk.next;
chunk.prependRight( indentStr );
}
}
}
Expand Down Expand Up @@ -313,7 +309,7 @@ MagicString.prototype = {
}

first.previous = newLeft;
last.next = newRight;
last.next = newRight || null;

if ( !newLeft ) this.firstChunk = first;
if ( !newRight ) this.lastChunk = last;
Expand Down Expand Up @@ -362,15 +358,15 @@ MagicString.prototype = {

first.edit( content, storeName, contentOnly );

if ( last ) {
first.next = last.next;
} else {
first.next = null;
this.lastChunk = first;
}
if ( first !== last ) {
let chunk = first.next;
while ( chunk !== last ) {
chunk.edit( '', false );
chunk = chunk.next;
}

first.original = this.original.slice( start, end );
first.end = end;
chunk.edit( '', false );
}
}

else {
Expand Down Expand Up @@ -586,10 +582,13 @@ MagicString.prototype = {

// if chunk was trimmed, we have a new lastChunk
if ( chunk.end !== end ) {
this.lastChunk = chunk.next;
if ( this.lastChunk === chunk ) {
this.lastChunk = chunk.next;
}

this.byEnd[ chunk.end ] = chunk;
this.byStart[ chunk.next.start ] = chunk.next;
this.byEnd[ chunk.next.end ] = chunk.next;
}

if ( aborted ) return this;
Expand Down Expand Up @@ -617,6 +616,7 @@ MagicString.prototype = {

this.byEnd[ chunk.end ] = chunk;
this.byStart[ chunk.next.start ] = chunk.next;
this.byEnd[ chunk.next.end ] = chunk.next;
}

if ( aborted ) return this;
Expand Down
11 changes: 10 additions & 1 deletion test/MagicString.js
@@ -1,6 +1,6 @@
const assert = require( 'assert' );
const SourceMapConsumer = require( 'source-map' ).SourceMapConsumer;
const MagicString = require( '../' );
const MagicString = require( './utils/IntegrityCheckingMagicString' );

require( 'source-map-support' ).install();
require( 'console-group' ).install();
Expand Down Expand Up @@ -808,6 +808,15 @@ describe( 'MagicString', () => {
s.move( 6, 9, 3 );
assert.throws( () => s.overwrite( 5, 7, 'XX' ), /Cannot overwrite across a split point/ );
});

it ( 'allows later insertions at the end', () => {
const s = new MagicString( 'abcdefg' );

s.appendLeft(4, '(');
s.overwrite( 2, 7, '' );
s.appendLeft(7, 'h');
assert.equal( s.toString(), 'abh' );
});
});

describe( 'prepend', () => {
Expand Down
46 changes: 46 additions & 0 deletions test/utils/IntegrityCheckingMagicString.js
@@ -0,0 +1,46 @@
const MagicString = require( '../../' );
const assert = require( 'assert' );

class IntegrityCheckingMagicString extends MagicString {
checkIntegrity () {
let prevChunk = null;
let chunk = this.firstChunk;
let numNodes = 0;
while ( chunk ) {
assert.strictEqual( this.byStart[chunk.start], chunk );
assert.strictEqual( this.byEnd[chunk.end], chunk );
assert.strictEqual( chunk.previous, prevChunk );
if ( prevChunk ) {
assert.strictEqual( prevChunk.next, chunk );
}
prevChunk = chunk;
chunk = chunk.next;
numNodes++;
}
assert.strictEqual( prevChunk, this.lastChunk );
assert.strictEqual( this.lastChunk.next, null );
assert.strictEqual( Object.keys(this.byStart).length, numNodes );
assert.strictEqual( Object.keys(this.byEnd).length, numNodes );
}
}

for (const key in MagicString.prototype) {
if ( !MagicString.prototype.hasOwnProperty( key ) ) {
continue;
}
const func = MagicString.prototype[key];
if ( typeof func === 'function' ) {
IntegrityCheckingMagicString.prototype[key] = function () {
const result = func.apply( this, arguments );
try {
this.checkIntegrity();
} catch ( e ) {
e.message = `Integrity error after invoking ${key}:\n${e.message}`;
throw e;
}
return result;
};
}
}

module.exports = IntegrityCheckingMagicString;

0 comments on commit 8a85c8c

Please sign in to comment.