implement bundle.warn and module.warn, to replace direct calls to bundle.onwarn (#1194)

package.json

@@ -53,6 +53,7 @@
     "eslint-plugin-import": "^2.2.0",
     "is-reference": "^1.0.0",
     "istanbul": "^0.4.3",
+    "locate-character": "^2.0.0",
     "magic-string": "^0.15.2",
     "minimist": "^1.2.0",
     "mocha": "^3.0.0",

src/Bundle.js

@@ -566,4 +566,9 @@ export default class Bundle {
 
 		return ordered;
 	}
+
+	warn ( warning ) {
+		warning.toString = () => warning.message || warning;
+		this.onwarn( warning );
+	}
 }

src/Module.js

@@ -1,10 +1,11 @@
-import { timeStart, timeEnd } from './utils/flushTime.js';
 import { parse } from 'acorn/src/index.js';
 import MagicString from 'magic-string';
+import { locate } from 'locate-character';
+import { timeStart, timeEnd } from './utils/flushTime.js';
 import { assign, blank, deepClone, keys } from './utils/object.js';
 import { basename, extname } from './utils/path.js';
-import getLocation from './utils/getLocation.js';
 import makeLegalIdentifier from './utils/makeLegalIdentifier.js';
+import getCodeFrame from './utils/getCodeFrame.js';
 import { SOURCEMAPPING_URL_RE } from './utils/sourceMappingURL.js';
 import error from './utils/error.js';
 import relativeId from './utils/relativeId.js';
@@ -195,7 +196,7 @@ export default class Module {
 			if ( this.imports[ localName ] ) {
 				const err = new Error( `Duplicated import '${localName}'` );
 				err.file = this.id;
-				err.loc = getLocation( this.code, specifier.start );
+				err.loc = locate( this.code, specifier.start, { offsetLine: 1 });
 				throw err;
 			}
 
@@ -361,7 +362,7 @@ export default class Module {
 				error({
 					message: `'${importDeclaration.name}' is not exported by ${relativeId( otherModule.id )} (imported by ${relativeId( this.id )}). For help fixing this error see https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module`,
 					file: this.id,
-					loc: getLocation( this.code, importDeclaration.specifier.start )
+					loc: locate( this.code, importDeclaration.specifier.start, { offsetLine: 1 })
 				});
 			}
 
@@ -381,7 +382,7 @@ export default class Module {
 				error({
 					message: `'${reexportDeclaration.localName}' is not exported by '${reexportDeclaration.module.id}' (imported by '${this.id}')`,
 					file: this.id,
-					loc: getLocation( this.code, reexportDeclaration.start )
+					loc: locate( this.code, reexportDeclaration.start, { offsetLine: 1 })
 				});
 			}
 
@@ -405,4 +406,22 @@ export default class Module {
 			if ( declaration ) return declaration;
 		}
 	}
+
+	warn ( warning, pos ) {
+		if ( pos ) {
+			warning.pos = pos;
+
+			const { line, column } = locate( this.code, pos, { offsetLine: 1 }); // TODO trace sourcemaps
+
+			warning.loc = {
+				file: this.id,
+				line: line + 1,
+				column
+			};
+
+			warning.frame = getCodeFrame( this.code, line, column );
+		}
+
+		this.bundle.warn( warning );
+	}
 }

src/ast/Node.js

@@ -1,5 +1,5 @@
+import { locate } from 'locate-character';
 import { UNKNOWN } from './values.js';
-import getLocation from '../utils/getLocation.js';
 
 export default class Node {
 	bind ( scope ) {
@@ -74,7 +74,7 @@ export default class Node {
 
 	locate () {
 		// useful for debugging
-		const location = getLocation( this.module.code, this.start );
+		const location = locate( this.module.code, this.start, { offsetLine: 1 });
 		location.file = this.module.id;
 		location.toString = () => JSON.stringify( location );
 

src/ast/nodes/CallExpression.js

@@ -1,4 +1,4 @@
-import getLocation from '../../utils/getLocation.js';
+import { locate } from 'locate-character';
 import error from '../../utils/error.js';
 import Node from '../Node.js';
 import isProgramLevel from '../utils/isProgramLevel.js';
@@ -14,12 +14,16 @@ export default class CallExpression extends Node {
 					message: `Cannot call a namespace ('${this.callee.name}')`,
 					file: this.module.id,
 					pos: this.start,
-					loc: getLocation( this.module.code, this.start )
+					loc: locate( this.module.code, this.start, { offsetLine: 1 })
 				});
 			}
 
 			if ( this.callee.name === 'eval' && declaration.isGlobal ) {
-				this.module.bundle.onwarn( `Use of \`eval\` (in ${this.module.id}) is strongly discouraged, as it poses security risks and may cause issues with minification. See https://github.com/rollup/rollup/wiki/Troubleshooting#avoiding-eval for more details` );
+				this.module.warn({
+					code: 'EVAL',
+					message: `Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification`,
+					url: 'https://github.com/rollup/rollup/wiki/Troubleshooting#avoiding-eval'
+				}, this.start );
 			}
 		}
 

src/ast/nodes/ExportDefaultDeclaration.js

@@ -1,5 +1,5 @@
 import Node from '../Node.js';
-import getLocation from '../../utils/getLocation.js';
+import { locate } from 'locate-character';
 import relativeId from '../../utils/relativeId.js';
 
 const functionOrClassDeclaration = /^(?:Function|Class)Declaration/;
@@ -74,7 +74,7 @@ export default class ExportDefaultDeclaration extends Node {
 					const newlineSeparated = /\n/.test( code.original.slice( start, end ) );
 
 					if ( newlineSeparated ) {
-						const { line, column } = getLocation( this.module.code, this.declaration.start );
+						const { line, column } = locate( this.module.code, this.declaration.start, { offsetLine: 1 });
 						this.module.bundle.onwarn( `${relativeId( this.module.id )} (${line}:${column}) Ambiguous default export (is a call expression, but looks like a function declaration). See https://github.com/rollup/rollup/wiki/Troubleshooting#ambiguous-default-export` );
 					}
 				}

src/ast/nodes/MemberExpression.js

@@ -1,5 +1,5 @@
 import isReference from 'is-reference';
-import getLocation from '../../utils/getLocation.js';
+import { locate } from 'locate-character';
 import relativeId from '../../utils/relativeId.js';
 import Node from '../Node.js';
 import { UNKNOWN } from '../values.js';
@@ -34,7 +34,7 @@ export default class MemberExpression extends Node {
 				declaration = declaration.module.traceExport( part.name );
 
 				if ( !declaration ) {
-					const { line, column } = getLocation( this.module.code, this.start );
+					const { line, column } = locate( this.module.code, this.start, { offsetLine: 1 });
 					this.module.bundle.onwarn( `${relativeId( this.module.id )} (${line}:${column}) '${part.name}' is not exported by '${relativeId( exporterId )}'. See https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` );
 					this.replacement = 'undefined';
 					return;

src/ast/nodes/ThisExpression.js

@@ -1,5 +1,5 @@
 import Node from '../Node.js';
-import getLocation from '../../utils/getLocation.js';
+import { locate } from 'locate-character';
 import relativeId from '../../utils/relativeId.js';
 
 const warning = `The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten. See https://github.com/rollup/rollup/wiki/Troubleshooting#this-is-undefined for more information`;
@@ -11,7 +11,7 @@ export default class ThisExpression extends Node {
 		if ( lexicalBoundary.isModuleScope ) {
 			this.alias = this.module.context;
 			if ( this.alias === 'undefined' ) {
-				const { line, column } = getLocation( this.module.code, this.start );
+				const { line, column } = locate( this.module.code, this.start, { offsetLine: 1 });
 				const detail = `${relativeId( this.module.id )} (${line}:${column + 1})`; // use one-based column number convention
 				this.module.bundle.onwarn( `${detail} ${warning}` );
 			}

src/ast/nodes/shared/disallowIllegalReassignment.js

@@ -1,4 +1,4 @@
-import getLocation from '../../../utils/getLocation.js';
+import { locate } from 'locate-character';
 import error from '../../../utils/error.js';
 
 // TODO tidy this up a bit (e.g. they can both use node.module.imports)
@@ -10,7 +10,7 @@ export default function disallowIllegalReassignment ( scope, node ) {
 				message: `Illegal reassignment to import '${node.object.name}'`,
 				file: node.module.id,
 				pos: node.start,
-				loc: getLocation( node.module.code, node.start )
+				loc: locate( node.module.code, node.start, { offsetLine: 1 })
 			});
 		}
 	}
@@ -21,7 +21,7 @@ export default function disallowIllegalReassignment ( scope, node ) {
 				message: `Illegal reassignment to import '${node.name}'`,
 				file: node.module.id,
 				pos: node.start,
-				loc: getLocation( node.module.code, node.start )
+				loc: locate( node.module.code, node.start, { offsetLine: 1 })
 			});
 		}
 	}

src/utils/getCodeFrame.js

@@ -0,0 +1,38 @@
+function spaces ( i ) {
+	let result = '';
+	while ( i-- ) result += ' ';
+	return result;
+}
+
+
+function tabsToSpaces ( str ) {
+	return str.replace( /^\t+/, match => match.split( '\t' ).join( '  ' ) );
+}
+
+export default function getCodeFrame ( source, line, column ) {
+	let lines = source.split( '\n' );
+
+	const frameStart = Math.max( 0, line - 3 );
+	const frameEnd = Math.min( line + 2, lines.length );
+
+	const digits = String( frameEnd + 1 ).length;
+
+	lines = lines.slice( frameStart, frameEnd );
+	while ( !/\S/.test( lines[ lines.length - 1 ] ) ) lines.pop();
+
+	return lines
+		.map( ( str, i ) => {
+			const isErrorLine = frameStart + i + 1 === line;
+
+			let lineNum = String( i + frameStart + 1 );
+			while ( lineNum.length < digits ) lineNum = ` ${lineNum}`;
+
+			if ( isErrorLine ) {
+				const indicator = spaces( digits + 2 + tabsToSpaces( str.slice( 0, column ) ).length ) + '^';
+				return `${lineNum}: ${tabsToSpaces( str )}\n${indicator}`;
+			}
+
+			return `${lineNum}: ${tabsToSpaces( str )}`;
+		})
+		.join( '\n' );
+}

src/utils/getLocation.js

@@ -1,20 +0,0 @@
-export default function getLocation ( source, charIndex ) {
-	const lines = source.split( '\n' );
-	const len = lines.length;
-
-	let lineStart = 0;
-	let i;
-
-	for ( i = 0; i < len; i += 1 ) {
-		const line = lines[i];
-		const lineEnd =  lineStart + line.length + 1; // +1 for newline
-
-		if ( lineEnd > charIndex ) {
-			return { line: i + 1, column: charIndex - lineStart };
-		}
-
-		lineStart = lineEnd;
-	}
-
-	throw new Error( 'Could not determine location of character' );
-}

test/function/cannot-call-external-namespace/_config.js

@@ -7,6 +7,6 @@ module.exports = {
 		assert.equal( err.message, 'Cannot call a namespace (\'foo\')' );
 		assert.equal( err.file.replace( /\//g, path.sep ), path.resolve( __dirname, 'main.js' ) );
 		assert.equal( err.pos, 28 );
-		assert.deepEqual( err.loc, { line: 2, column: 0 });
+		assert.deepEqual( err.loc, { character: 28, line: 2, column: 0 });
 	}
 };

test/function/cannot-call-internal-namespace/_config.js

@@ -7,6 +7,6 @@ module.exports = {
 		assert.equal( err.message, 'Cannot call a namespace (\'foo\')' );
 		assert.equal( err.file.replace( /\//g, path.sep ), path.resolve( __dirname, 'main.js' ) );
 		assert.equal( err.pos, 33 );
-		assert.deepEqual( err.loc, { line: 2, column: 0 });
+		assert.deepEqual( err.loc, { character: 33, line: 2, column: 0 });
 	}
 };

test/function/duplicate-import-fails/_config.js

@@ -4,8 +4,8 @@ var assert = require( 'assert' );
 module.exports = {
 	description: 'disallows duplicate imports',
 	error: function ( err ) {
-		assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) );
+		assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) );
-		assert.deepEqual( err.loc, { line: 2, column: 9 });
+		assert.deepEqual( err.loc, { character: 36, line: 2, column: 9 });
 		assert.ok( /Duplicated import/.test( err.message ) );
 	}
 };

test/function/duplicate-import-specifier-fails/_config.js

@@ -4,8 +4,8 @@ var assert = require( 'assert' );
 module.exports = {
 	description: 'disallows duplicate import specifiers',
 	error: function ( err ) {
-		assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) );
+		assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) );
-		assert.deepEqual( err.loc, { line: 1, column: 12 });
+		assert.deepEqual( err.loc, { character: 12, line: 1, column: 12 });
 		assert.ok( /Duplicated import/.test( err.message ) );
 	}
 };

test/function/namespace-reassign-import-fails/_config.js

@@ -4,8 +4,8 @@ var assert = require( 'assert' );
 module.exports = {
 	description: 'disallows reassignments to namespace exports',
 	error: function ( err ) {
-		assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) );
+		assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) );
-		assert.deepEqual( err.loc, { line: 3, column: 0 });
+		assert.deepEqual( err.loc, { character: 31, line: 3, column: 0 });
 		assert.ok( /Illegal reassignment/.test( err.message ) );
 	}
 };

test/function/namespace-update-import-fails/_config.js

@@ -4,8 +4,8 @@ var assert = require( 'assert' );
 module.exports = {
 	description: 'disallows updates to namespace exports',
 	error: function ( err ) {
-		assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) );
+		assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) );
-		assert.deepEqual( err.loc, { line: 3, column: 0 });
+		assert.deepEqual( err.loc, { character: 31, line: 3, column: 0 });
 		assert.ok( /Illegal reassignment/.test( err.message ) );
 	}
 };

test/function/reassign-import-fails/_config.js

@@ -5,8 +5,8 @@ module.exports = {
 	description: 'disallows assignments to imported bindings',
 	error: function ( err ) {
 		assert.ok( /Illegal reassignment/.test( err.message ) );
-		assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) );
+		assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) );
-		assert.deepEqual( err.loc, { line: 8, column: 0 });
+		assert.deepEqual( err.loc, { character: 113, line: 8, column: 0 });
 	}
 };
 

test/function/reassign-import-not-at-top-level-fails/_config.js

@@ -4,8 +4,8 @@ var assert = require( 'assert' );
 module.exports = {
 	description: 'disallows assignments to imported bindings not at the top level',
 	error: function ( err ) {
-		assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) );
+		assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) );
-		assert.deepEqual( err.loc, { line: 7, column: 2 });
+		assert.deepEqual( err.loc, { character: 95, line: 7, column: 2 });
 		assert.ok( /Illegal reassignment/.test( err.message ) );
 	}
 };

test/function/update-expression-of-import-fails/_config.js

@@ -4,8 +4,8 @@ var assert = require( 'assert' );
 module.exports = {
 	description: 'disallows updates to imported bindings',
 	error: function ( err ) {
-		assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) );
+		assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) );
-		assert.deepEqual( err.loc, { line: 3, column: 0 });
+		assert.deepEqual( err.loc, { character: 28, line: 3, column: 0 });
 		assert.ok( /Illegal reassignment/.test( err.message ) );
 	}
 };

test/function/warn-on-eval/_config.js

@@ -1,16 +1,16 @@
-var assert = require( 'assert' );
-
-var warned = false;
-
 module.exports = {
 	description: 'warns about use of eval',
-	options: {
+	warnings: [
-		onwarn: function ( message ) {
+		{
-			warned = true;
+			code: 'EVAL',
-			assert.ok( /Use of `eval` \(in .+?main\.js\) is strongly discouraged, as it poses security risks and may cause issues with minification\. See https:\/\/github.com\/rollup\/rollup\/wiki\/Troubleshooting#avoiding-eval for more details/.test( message ) );
+			message: `Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification`,
+			pos: 13,
+			loc: {
+				column: 13,
+				file: require( 'path' ).resolve( __dirname, 'main.js' ),
+				line: 2
+			},
+			url: 'https://github.com/rollup/rollup/wiki/Troubleshooting#avoiding-eval'
 		}
-	},
+	]
-	exports: function () {
-		assert.ok( warned, 'did not warn' );
-	}
 };

test/test.js

@@ -270,7 +270,17 @@ describe( 'rollup', function () {
 						}
 
 						if ( config.warnings ) {
-							config.warnings( warnings );
+							if ( Array.isArray( config.warnings ) ) {
+								assert.deepEqual( warnings.map( warning => {
+									const clone = Object.assign( {}, warning );
+									delete clone.toString;
+									delete clone.frame;
+
+									return clone;
+								}), config.warnings );
+							} else {
+								config.warnings( warnings );
+							}
 						} else if ( warnings.length ) {
 							throw new Error( `Got unexpected warnings:\n${warnings.join('\n')}` );
 						}