From 18ba8f09accbf87c34a1c9ead30fcd0e771f872f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Segersv=C3=A4rd?= Date: Wed, 23 Sep 2015 12:05:56 +0200 Subject: [PATCH 1/4] Don't include 'default' export from 'export * from ...' --- src/Module.js | 2 +- test/form/export-all-from-internal/_expected/amd.js | 2 -- test/form/export-all-from-internal/_expected/cjs.js | 2 -- test/form/export-all-from-internal/_expected/es6.js | 2 -- test/form/export-all-from-internal/_expected/iife.js | 2 -- test/form/export-all-from-internal/_expected/umd.js | 2 -- 6 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/Module.js b/src/Module.js index a32c021..2123103 100644 --- a/src/Module.js +++ b/src/Module.js @@ -288,7 +288,7 @@ export default class Module { // bind the name to the other module's reference. this.allExportsFrom.forEach( module => { module.exports.getNames().forEach( name => { - if ( !this.exports.defines( name ) ) { + if ( name !== 'default' && !this.exports.defines( name ) ) { this.exports.bind( name, module.exports.reference( name ) ); } }); diff --git a/test/form/export-all-from-internal/_expected/amd.js b/test/form/export-all-from-internal/_expected/amd.js index 89f5652..6b0c47e 100644 --- a/test/form/export-all-from-internal/_expected/amd.js +++ b/test/form/export-all-from-internal/_expected/amd.js @@ -2,10 +2,8 @@ define(['exports'], function (exports) { 'use strict'; const a = 1; const b = 2; - var internal = 42; exports.a = a; exports.b = b; - exports['default'] = internal; }); diff --git a/test/form/export-all-from-internal/_expected/cjs.js b/test/form/export-all-from-internal/_expected/cjs.js index cadb160..869bba0 100644 --- a/test/form/export-all-from-internal/_expected/cjs.js +++ b/test/form/export-all-from-internal/_expected/cjs.js @@ -2,8 +2,6 @@ const a = 1; const b = 2; -var internal = 42; exports.a = a; exports.b = b; -exports['default'] = internal; diff --git a/test/form/export-all-from-internal/_expected/es6.js b/test/form/export-all-from-internal/_expected/es6.js index d68e7f9..aafcac6 100644 --- a/test/form/export-all-from-internal/_expected/es6.js +++ b/test/form/export-all-from-internal/_expected/es6.js @@ -1,6 +1,4 @@ const a = 1; const b = 2; -var internal = 42; export { a, b }; -export default internal; diff --git a/test/form/export-all-from-internal/_expected/iife.js b/test/form/export-all-from-internal/_expected/iife.js index c093623..238e208 100644 --- a/test/form/export-all-from-internal/_expected/iife.js +++ b/test/form/export-all-from-internal/_expected/iife.js @@ -2,10 +2,8 @@ const a = 1; const b = 2; - var internal = 42; exports.a = a; exports.b = b; - exports['default'] = internal; })((this.exposedInternals = {})); diff --git a/test/form/export-all-from-internal/_expected/umd.js b/test/form/export-all-from-internal/_expected/umd.js index 45a93fa..50c5cc4 100644 --- a/test/form/export-all-from-internal/_expected/umd.js +++ b/test/form/export-all-from-internal/_expected/umd.js @@ -6,10 +6,8 @@ const a = 1; const b = 2; - var internal = 42; exports.a = a; exports.b = b; - exports['default'] = internal; })); From b325f33a028198f891731f77752c341b4e1a7573 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Segersv=C3=A4rd?= Date: Thu, 24 Sep 2015 13:38:24 +0200 Subject: [PATCH 2/4] Changed the default deconfliction function to behave more like Babel. Should be much faster for many identical identifiers. --- src/Scope.js | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/Scope.js b/src/Scope.js index e5f5967..8185085 100644 --- a/src/Scope.js +++ b/src/Scope.js @@ -27,9 +27,23 @@ function isntReference ( id ) { return !( id instanceof Reference ); } -// Prefix the argument with '_'. -function underscorePrefix ( x ) { - return '_' + x; +// Returns a function that will prefix its argument with '_' +// and append a number if called with the same argument more than once. +function underscorePrefix () { + function number ( x ) { + if ( !( x in map ) ) { + map[ x ] = 0; + return ''; + } + + return map[ x ]++; + } + + var map = blank(); + + return function ( x ) { + return '_' + x + number( x ); + } } // ## Scope @@ -51,7 +65,7 @@ export default class Scope { // Deconflict all names within the scope, // using the given renaming function. // If no function is supplied, `underscorePrefix` is used. - deconflict ( rename = underscorePrefix ) { + deconflict ( rename = underscorePrefix() ) { const names = this.used; this.ids.filter( ref => ref instanceof Reference ).forEach( ref => { From 0c83250fae3b4d0c616a5152c4f0d731fdce7309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Segersv=C3=A4rd?= Date: Thu, 24 Sep 2015 17:59:21 +0200 Subject: [PATCH 3/4] Polish optimization of namespace lookups * Explain better how optimization of namespace lookups are done in comments. * Consider `hasReplacements` to be true if there exists any namespace replacements. * Improve error messages slightly --- src/Statement.js | 56 +++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/src/Statement.js b/src/Statement.js index 1a165b4..93b2a03 100644 --- a/src/Statement.js +++ b/src/Statement.js @@ -25,12 +25,19 @@ function isFunctionDeclaration ( node, parent ) { if ( node.type === 'FunctionExpression' && parent.type === 'VariableDeclarator' ) return true; } +// Extract the property access from a MemberExpression. +function property ( node ) { + return node.name ? `.${node.name}` : `[${node.value}]`; +} + +// Recursively traverse the chain of member expressions from `node`, +// returning the access, e.g. `foo.bar[17]` function chainedMemberExpression ( node ) { if ( node.object.type === 'MemberExpression' ) { - return chainedMemberExpression( node.object ) + '.' + node.property.name; + return chainedMemberExpression( node.object ) + property( node.property ); } - return node.object.name + '.' + node.property.name; + return node.object.name + property( node.property ); } export default class Statement { @@ -161,10 +168,10 @@ export default class Statement { // /update expressions) need to be captured let writeDepth = 0; - // Used to track - let topName; - let currentMemberExpression = null; - let namespace = null; + // Used to track and optimize lookups through namespaces. + let localName; // The local name of the top-most imported namespace. + let topNode = null; // The top-node of the member expression. + let namespace = null; // An instance of `Module`. if ( !this.isImportDeclaration ) { walk( this.node, { @@ -184,13 +191,21 @@ export default class Statement { if ( node._scope ) scope = scope.parent; // Optimize namespace lookups, which manifest as MemberExpressions. - if ( node.type === 'MemberExpression' && ( !currentMemberExpression || node.object === currentMemberExpression ) ) { - currentMemberExpression = node; + if ( node.type === 'MemberExpression' && ( !topNode || node.object === topNode ) ) { + // Ignore anything that doesn't begin with an identifier. + if ( !topNode && node.object.type !== 'Identifier') return; + topNode = node; + + // If we don't already have a namespace, + // we aren't currently exploring any chain of member expressions. if ( !namespace ) { - topName = node.object.name; - const id = this.module.locals.lookup( topName ); + localName = node.object.name; + + // At first, we don't have a namespace, so we'll try to look one up. + const id = this.module.locals.lookup( localName ); + // It only counts if it exists, is a module, and isn't external. if ( !id || !id.isModule || id.isExternal ) return; namespace = id; @@ -225,16 +240,16 @@ export default class Statement { namespace.mark(); namespace = null; - currentMemberExpression = null; + topNode = null; return; } const id = namespace.exports.lookup( name ); - // If the namespace doesn't define the given name, + // If the namespace doesn't export the given name, // we can throw an error (even for nested namespaces). if ( !id ) { - throw new Error( `Module doesn't define "${name}"!` ); + throw new Error( `Module "${namespace.id}" doesn't export "${name}"!` ); } // We can't resolve deeper. Replace the member chain. @@ -244,18 +259,18 @@ export default class Statement { } // FIXME: do this better - // If we depend on this name... - if ( this.dependsOn[ topName ] ) { + // If an earlier stage detected that we depend on this name... + if ( this.dependsOn[ localName ] ) { // ... decrement the count... - if ( !--this.dependsOn[ topName ] ) { + if ( !--this.dependsOn[ localName ] ) { // ... and remove it if the count is 0. - delete this.dependsOn[ topName ]; + delete this.dependsOn[ localName ]; } } - this.namespaceReplacements.push( [ node, id ] ); + this.namespaceReplacements.push( [ topNode, id ] ); namespace = null; - currentMemberExpression = null; + topNode = null; return; } @@ -486,7 +501,8 @@ export default class Statement { topLevel = false; let newNames = blank(); - let hasReplacements; + // Consider a scope to have replacements if there are any namespaceReplacements. + let hasReplacements = statement.namespaceReplacements.length > 0; keys( names ).forEach( name => { if ( !scope.declarations[ name ] ) { From 365f45c7f3dd0854c94abeaab184469063e48103 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Segersv=C3=A4rd?= Date: Thu, 24 Sep 2015 19:03:04 +0200 Subject: [PATCH 4/4] Made Scope.reference stricter. Helps to catch undefined/missing exports. --- src/Module.js | 13 +++++++++++++ src/Scope.js | 7 ++++++- .../import-of-unexported-fails/_config.js | 10 ++++++++++ .../function/import-of-unexported-fails/empty.js | 0 test/function/import-of-unexported-fails/main.js | 3 +++ test/testScope.js | 16 +++++----------- 6 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 test/function/import-of-unexported-fails/_config.js create mode 100644 test/function/import-of-unexported-fails/empty.js create mode 100644 test/function/import-of-unexported-fails/main.js diff --git a/src/Module.js b/src/Module.js index 2123103..8f0c212 100644 --- a/src/Module.js +++ b/src/Module.js @@ -50,6 +50,18 @@ class Id { } } +class LateBoundIdPlaceholder { + constructor ( module, name ) { + this.module = module; + this.name = name; + this.placeholder = true; + } + + mark () { + throw new Error(`The imported name "${this.name}" is never exported by "${this.module.id}".`); + } +} + export default class Module { constructor ({ id, source, ast, bundle }) { this.source = source; @@ -103,6 +115,7 @@ export default class Module { } // throw new Error( `The name "${name}" is never exported (from ${this.id})!` ); + this.exports.define( name, new LateBoundIdPlaceholder( this, name ) ); return reference.call( this.exports, name ); }; diff --git a/src/Scope.js b/src/Scope.js index 8185085..e93a8bb 100644 --- a/src/Scope.js +++ b/src/Scope.js @@ -81,6 +81,7 @@ export default class Scope { }); this.ids.filter( isntReference ).forEach( id => { + // TODO: can this be removed? if ( typeof id === 'string' ) { throw new Error( `Required name "${id}" is undefined!` ); } @@ -153,7 +154,11 @@ export default class Scope { // Get a reference to the identifier `name` in this scope. reference ( name ) { - return new Reference( this, this.index( name ) ); + if ( !( name in this.names ) ) { + throw new Error( `Cannot reference undefined identifier "${name}"` ); + } + + return new Reference( this, this.names[ name ] ); } // Return the used names of the scope. diff --git a/test/function/import-of-unexported-fails/_config.js b/test/function/import-of-unexported-fails/_config.js new file mode 100644 index 0000000..75e1586 --- /dev/null +++ b/test/function/import-of-unexported-fails/_config.js @@ -0,0 +1,10 @@ +var assert = require( 'assert' ); + +module.exports = { + description: 'marking an imported, but unexported, identifier should throw', + + error: function ( err ) { + assert.equal( err.message.slice( 0, 50 ), 'The imported name "default" is never exported by "' ); + assert.equal( err.message.slice( -10 ), 'empty.js".' ); + } +}; diff --git a/test/function/import-of-unexported-fails/empty.js b/test/function/import-of-unexported-fails/empty.js new file mode 100644 index 0000000..e69de29 diff --git a/test/function/import-of-unexported-fails/main.js b/test/function/import-of-unexported-fails/main.js new file mode 100644 index 0000000..37cf334 --- /dev/null +++ b/test/function/import-of-unexported-fails/main.js @@ -0,0 +1,3 @@ +import a from './empty.js'; + +a(); diff --git a/test/testScope.js b/test/testScope.js index 2658774..67e443d 100644 --- a/test/testScope.js +++ b/test/testScope.js @@ -93,7 +93,7 @@ describe( 'Scope', function () { }); }); - it( 'dedupes-external-imports', function () { + it( 'cannot reference undefined names', function () { var real = new Scope(); var external = real.virtual(), @@ -104,17 +104,11 @@ describe( 'Scope', function () { locals.bind( 'Comp', external.reference( 'Component' ) ); - exports.bind( 'default', locals.reference( 'Foo' ) ); - - try { - real.deconflict(); - assert.ok( false, 'Scope.deconflict should throw with "Foo" undefined' ); - } catch ( ignore ) { - // as expected - } + assert.throws( function () { + exports.bind( 'default', locals.reference( 'Foo' ) ); + }, 'Cannot reference undefined identifier "Foo"' ); locals.define( 'Foo' ); - - real.deconflict(); + exports.bind( 'default', locals.reference( 'Foo' ) ); }); });