From d2f58273675ea6636daa5934d4e4011eeb4aa07e Mon Sep 17 00:00:00 2001 From: Alan Pierce Date: Thu, 6 Jul 2017 14:12:41 -0700 Subject: [PATCH] Add integrity checking to tests and fix various bugs 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 ( #115 ). I tried changing the code to remove the nodes properly, but the details seemed to get really subtle, especially accounting for moved fragments, and it didn't seem like the complexity was worth it, so I effectively just reverted #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 operations, since it meant that some chunks just get dropped. We now only run that line on the last chunk. --- .eslintrc | 1 + src/MagicString.js | 34 ++++++++-------- test/MagicString.js | 11 +++++- test/utils/IntegrityCheckingMagicString.js | 46 ++++++++++++++++++++++ 4 files changed, 74 insertions(+), 18 deletions(-) create mode 100644 test/utils/IntegrityCheckingMagicString.js diff --git a/.eslintrc b/.eslintrc index 7ed2294..704eeee 100644 --- a/.eslintrc +++ b/.eslintrc @@ -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, diff --git a/src/MagicString.js b/src/MagicString.js index d20c946..ea3b36e 100644 --- a/src/MagicString.js +++ b/src/MagicString.js @@ -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 ); } } } @@ -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; @@ -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 { @@ -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; @@ -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; diff --git a/test/MagicString.js b/test/MagicString.js index c9388e9..c4f31f0 100644 --- a/test/MagicString.js +++ b/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(); @@ -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', () => { diff --git a/test/utils/IntegrityCheckingMagicString.js b/test/utils/IntegrityCheckingMagicString.js new file mode 100644 index 0000000..9290a26 --- /dev/null +++ b/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;