From 5afaa39e683297f6db3070b1629b91ef10daa6e7 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Thu, 11 May 2017 10:28:48 -0700 Subject: [PATCH] refactor(compiler): produce more dense generated code (#16666) This changes the formatting to be less verbose but still tries to be readable. --- .../compiler/src/output/abstract_emitter.ts | 65 ++++++++++++------- packages/compiler/src/output/js_emitter.ts | 2 +- packages/compiler/src/output/ts_emitter.ts | 2 +- packages/compiler/test/aot/compiler_spec.ts | 4 +- .../compiler/test/aot/jit_summaries_spec.ts | 44 ++++++------- .../compiler/test/output/js_emitter_spec.ts | 22 +++++-- .../compiler/test/output/ts_emitter_spec.ts | 41 +++++++----- 7 files changed, 108 insertions(+), 72 deletions(-) diff --git a/packages/compiler/src/output/abstract_emitter.ts b/packages/compiler/src/output/abstract_emitter.ts index f6ce372f72..88843bd576 100644 --- a/packages/compiler/src/output/abstract_emitter.ts +++ b/packages/compiler/src/output/abstract_emitter.ts @@ -24,6 +24,7 @@ export abstract class OutputEmitter { } class _EmittedLine { + partsLength = 0; parts: string[] = []; srcSpans: (ParseSourceSpan|null)[] = []; constructor(public indent: number) {} @@ -51,9 +52,14 @@ export class EmitterVisitorContext { lineIsEmpty(): boolean { return this._currentLine.parts.length === 0; } + lineLength(): number { + return this._currentLine.indent * _INDENT_WITH.length + this._currentLine.partsLength; + } + print(from: {sourceSpan: ParseSourceSpan | null}|null, part: string, newLine: boolean = false) { if (part.length > 0) { this._currentLine.parts.push(part); + this._currentLine.partsLength += part.length; this._currentLine.srcSpans.push(from && from.sourceSpan || null); } if (newLine) { @@ -69,12 +75,16 @@ export class EmitterVisitorContext { incIndent() { this._indent++; - this._currentLine.indent = this._indent; + if (this.lineIsEmpty()) { + this._currentLine.indent = this._indent; + } } decIndent() { this._indent--; - this._currentLine.indent = this._indent; + if (this.lineIsEmpty()) { + this._currentLine.indent = this._indent; + } } pushClass(clazz: o.ClassStmt) { this._classes.push(clazz); } @@ -424,24 +434,18 @@ export abstract class AbstractEmitterVisitor implements o.StatementVisitor, o.Ex return null; } visitLiteralArrayExpr(ast: o.LiteralArrayExpr, ctx: EmitterVisitorContext): any { - const useNewLine = ast.entries.length > 1; - ctx.print(ast, `[`, useNewLine); - ctx.incIndent(); - this.visitAllExpressions(ast.entries, ctx, ',', useNewLine); - ctx.decIndent(); - ctx.print(ast, `]`, useNewLine); + ctx.print(ast, `[`); + this.visitAllExpressions(ast.entries, ctx, ','); + ctx.print(ast, `]`); return null; } visitLiteralMapExpr(ast: o.LiteralMapExpr, ctx: EmitterVisitorContext): any { - const useNewLine = ast.entries.length > 1; - ctx.print(ast, `{`, useNewLine); - ctx.incIndent(); + ctx.print(ast, `{`); this.visitAllObjects(entry => { - ctx.print(ast, `${escapeIdentifier(entry.key, this._escapeDollarInStrings, entry.quoted)}: `); + ctx.print(ast, `${escapeIdentifier(entry.key, this._escapeDollarInStrings, entry.quoted)}:`); entry.value.visitExpression(this, ctx); - }, ast.entries, ctx, ',', useNewLine); - ctx.decIndent(); - ctx.print(ast, `}`, useNewLine); + }, ast.entries, ctx, ','); + ctx.print(ast, `}`); return null; } visitCommaExpr(ast: o.CommaExpr, ctx: EmitterVisitorContext): any { @@ -450,24 +454,35 @@ export abstract class AbstractEmitterVisitor implements o.StatementVisitor, o.Ex ctx.print(ast, ')'); return null; } - visitAllExpressions( - expressions: o.Expression[], ctx: EmitterVisitorContext, separator: string, - newLine: boolean = false): void { - this.visitAllObjects( - expr => expr.visitExpression(this, ctx), expressions, ctx, separator, newLine); + visitAllExpressions(expressions: o.Expression[], ctx: EmitterVisitorContext, separator: string): + void { + this.visitAllObjects(expr => expr.visitExpression(this, ctx), expressions, ctx, separator); } visitAllObjects( - handler: (t: T) => void, expressions: T[], ctx: EmitterVisitorContext, separator: string, - newLine: boolean = false): void { + handler: (t: T) => void, expressions: T[], ctx: EmitterVisitorContext, + separator: string): void { + let incrementedIndent = false; for (let i = 0; i < expressions.length; i++) { if (i > 0) { - ctx.print(null, separator, newLine); + if (ctx.lineLength() > 80) { + ctx.print(null, separator, true); + if (!incrementedIndent) { + // continuation are marked with double indent. + ctx.incIndent(); + ctx.incIndent(); + incrementedIndent = true; + } + } else { + ctx.print(null, separator, false); + } } handler(expressions[i]); } - if (newLine) { - ctx.println(); + if (incrementedIndent) { + // continuation are marked with double indent. + ctx.decIndent(); + ctx.decIndent(); } } diff --git a/packages/compiler/src/output/js_emitter.ts b/packages/compiler/src/output/js_emitter.ts index 5f478a83a8..9f8b7635f1 100644 --- a/packages/compiler/src/output/js_emitter.ts +++ b/packages/compiler/src/output/js_emitter.ts @@ -62,7 +62,7 @@ class JsEmitterVisitor extends AbstractJsEmitterVisitor { if (filePath != this._genFilePath) { let prefix = this.importsWithPrefixes.get(filePath); if (prefix == null) { - prefix = `import${this.importsWithPrefixes.size}`; + prefix = `i${this.importsWithPrefixes.size}`; this.importsWithPrefixes.set(filePath, prefix); } ctx.print(ast, `${prefix}.`); diff --git a/packages/compiler/src/output/ts_emitter.ts b/packages/compiler/src/output/ts_emitter.ts index 636fc9d383..0f7d91d17b 100644 --- a/packages/compiler/src/output/ts_emitter.ts +++ b/packages/compiler/src/output/ts_emitter.ts @@ -398,7 +398,7 @@ class _TsEmitterVisitor extends AbstractEmitterVisitor implements o.TypeVisitor if (filePath != this._genFilePath) { let prefix = this.importsWithPrefixes.get(filePath); if (prefix == null) { - prefix = `import${this.importsWithPrefixes.size}`; + prefix = `i${this.importsWithPrefixes.size}`; this.importsWithPrefixes.set(filePath, prefix); } ctx.print(null, `${prefix}.`); diff --git a/packages/compiler/test/aot/compiler_spec.ts b/packages/compiler/test/aot/compiler_spec.ts index fa675a92b0..1c02ea4350 100644 --- a/packages/compiler/test/aot/compiler_spec.ts +++ b/packages/compiler/test/aot/compiler_spec.ts @@ -412,7 +412,7 @@ describe('compiler (unbundled Angular)', () => { const mainNgFactory = genFiles.find(gf => gf.srcFileUrl === '/app/main.ts'); const flags = NodeFlags.TypeDirective | NodeFlags.Component | NodeFlags.OnDestroy; expect(mainNgFactory.source) - .toContain(`${flags},(null as any),0,import1.Extends,[import2.AParam]`); + .toContain(`${flags},(null as any),0,i1.Extends,[i2.AParam]`); }); })); @@ -464,7 +464,7 @@ describe('compiler (unbundled Angular)', () => { const mainNgFactory = genFiles.find(gf => gf.srcFileUrl === '/app/main.ts'); const flags = NodeFlags.TypeDirective | NodeFlags.Component | NodeFlags.OnDestroy; expect(mainNgFactory.source) - .toContain(`${flags},(null as any),0,import1.Extends,[import2.AParam_2]`); + .toContain(`${flags},(null as any),0,i1.Extends,[i2.AParam_2]`); }); })); diff --git a/packages/compiler/test/aot/jit_summaries_spec.ts b/packages/compiler/test/aot/jit_summaries_spec.ts index fb7e140003..934e37257d 100644 --- a/packages/compiler/test/aot/jit_summaries_spec.ts +++ b/packages/compiler/test/aot/jit_summaries_spec.ts @@ -44,11 +44,11 @@ describe('aot summaries for jit', () => { const genFile = compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts'); - expect(genFile.source).toContain(`import * as import0 from '/app/app.module'`); + expect(genFile.source).toContain(`import * as i0 from '/app/app.module'`); expect(genFile.source).toContain('export function MyServiceNgSummary()'); // Note: CompileSummaryKind.Injectable = 3 - expect(genFile.source).toMatch(/summaryKind: 3,\s*type: \{\s*reference: import0.MyService/); - expect(genFile.source).toContain('token: {identifier: {reference: import0.Dep}}'); + expect(genFile.source).toMatch(/summaryKind:3,\s*type:\{\s*reference:i0.MyService/); + expect(genFile.source).toContain('token:{identifier:{reference:i0.Dep}}'); })); it('should create @Pipe summaries', fakeAsync(() => { @@ -72,11 +72,11 @@ describe('aot summaries for jit', () => { const genFile = compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts'); - expect(genFile.source).toContain(`import * as import0 from '/app/app.module'`); + expect(genFile.source).toContain(`import * as i0 from '/app/app.module'`); expect(genFile.source).toContain('export function MyPipeNgSummary()'); // Note: CompileSummaryKind.Pipe = 1 - expect(genFile.source).toMatch(/summaryKind: 0,\s*type: \{\s*reference: import0.MyPipe/); - expect(genFile.source).toContain('token: {identifier: {reference: import0.Dep}}'); + expect(genFile.source).toMatch(/summaryKind:0,\s*type:\{\s*reference:i0.MyPipe/); + expect(genFile.source).toContain('token:{identifier:{reference:i0.Dep}}'); })); it('should create @Directive summaries', fakeAsync(() => { @@ -100,12 +100,11 @@ describe('aot summaries for jit', () => { const genFile = compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts'); - expect(genFile.source).toContain(`import * as import0 from '/app/app.module'`); + expect(genFile.source).toContain(`import * as i0 from '/app/app.module'`); expect(genFile.source).toContain('export function MyDirectiveNgSummary()'); // Note: CompileSummaryKind.Directive = 1 - expect(genFile.source) - .toMatch(/summaryKind: 1,\s*type: \{\s*reference: import0.MyDirective/); - expect(genFile.source).toContain('token: {identifier: {reference: import0.Dep}}'); + expect(genFile.source).toMatch(/summaryKind:1,\s*type:\{\s*reference:i0.MyDirective/); + expect(genFile.source).toContain('token:{identifier:{reference:i0.Dep}}'); })); it('should create @NgModule summaries', fakeAsync(() => { @@ -126,11 +125,11 @@ describe('aot summaries for jit', () => { const genFile = compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts'); - expect(genFile.source).toContain(`import * as import0 from '/app/app.module'`); + expect(genFile.source).toContain(`import * as i0 from '/app/app.module'`); expect(genFile.source).toContain('export function MyModuleNgSummary()'); // Note: CompileSummaryKind.NgModule = 2 - expect(genFile.source).toMatch(/summaryKind: 2,\s*type: \{\s*reference: import0.MyModule/); - expect(genFile.source).toContain('token: {identifier: {reference: import0.Dep}}'); + expect(genFile.source).toMatch(/summaryKind:2,\s*type:\{\s*reference:i0.MyModule/); + expect(genFile.source).toContain('token:{identifier:{reference:i0.Dep}}'); })); it('should embed useClass provider summaries in @Directive summaries', fakeAsync(() => { @@ -164,10 +163,10 @@ describe('aot summaries for jit', () => { const genFile = compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts'); - expect(genFile.source).toMatch(/useClass: \{\s*reference: import1.MyService/); + expect(genFile.source).toMatch(/useClass:\{\s*reference:i1.MyService/); // Note: CompileSummaryKind.Injectable = 3 - expect(genFile.source).toMatch(/summaryKind: 3,\s*type: \{\s*reference: import1.MyService/); - expect(genFile.source).toContain('token: {identifier: {reference: import1.Dep}}'); + expect(genFile.source).toMatch(/summaryKind:3,\s*type:\{\s*reference:i1.MyService/); + expect(genFile.source).toContain('token:{identifier:{reference:i1.Dep}}'); })); it('should embed useClass provider summaries into @NgModule summaries', fakeAsync(() => { @@ -197,10 +196,10 @@ describe('aot summaries for jit', () => { const genFile = compileApp(rootDir).genFiles.find(f => f.genFileUrl === '/app/app.module.ngsummary.ts'); - expect(genFile.source).toMatch(/useClass: \{\s*reference: import1.MyService/); + expect(genFile.source).toMatch(/useClass:\{\s*reference:i1.MyService/); // Note: CompileSummaryKind.Injectable = 3 - expect(genFile.source).toMatch(/summaryKind: 3,\s*type: \{\s*reference: import1.MyService/); - expect(genFile.source).toContain('token: {identifier: {reference: import1.Dep}}'); + expect(genFile.source).toMatch(/summaryKind:3,\s*type:\{\s*reference:i1.MyService/); + expect(genFile.source).toContain('token:{identifier:{reference:i1.Dep}}'); })); it('should reference declared @Directive and @Pipe summaries in @NgModule summaries', @@ -327,13 +326,12 @@ describe('aot summaries for jit', () => { lib3Gen.find(f => f.genFileUrl === '/lib3/reexport.ngsummary.ts'); // ngsummary.ts files should use the reexported values from direct and deep deps + expect(lib3ModuleNgSummary.source).toContain(`import * as i4 from '/lib2/module.ngsummary'`); expect(lib3ModuleNgSummary.source) - .toContain(`import * as import4 from '/lib2/module.ngsummary'`); - expect(lib3ModuleNgSummary.source) - .toContain(`import * as import5 from '/lib2/reexport.ngsummary'`); + .toContain(`import * as i5 from '/lib2/reexport.ngsummary'`); expect(lib3ModuleNgSummary.source) .toMatch( - /export function Lib3ModuleNgSummary()[^;]*,\s*import4.Lib1Module_1NgSummary,\s*import4.Lib2ModuleNgSummary,\s*import5.ReexportModule_2NgSummary\s*\]\s*;/); + /export function Lib3ModuleNgSummary()[^;]*,\s*i4.Lib1Module_1NgSummary,\s*i4.Lib2ModuleNgSummary,\s*i5.ReexportModule_2NgSummary\s*\]\s*;/); // ngsummaries should add reexports for imported NgModules from a deep dependency expect(lib3ModuleNgSummary.source) diff --git a/packages/compiler/test/output/js_emitter_spec.ts b/packages/compiler/test/output/js_emitter_spec.ts index fa6f65cbba..9b293f4830 100644 --- a/packages/compiler/test/output/js_emitter_spec.ts +++ b/packages/compiler/test/output/js_emitter_spec.ts @@ -115,7 +115,19 @@ export function main() { expect(emitStmt(o.literal(true).toStmt())).toEqual('true;'); expect(emitStmt(o.literal('someStr').toStmt())).toEqual(`'someStr';`); expect(emitStmt(o.literalArr([o.literal(1)]).toStmt())).toEqual(`[1];`); - expect(emitStmt(o.literalMap([['someKey', o.literal(1)]]).toStmt())).toEqual(`{someKey: 1};`); + expect(emitStmt(o.literalMap([['someKey', o.literal(1)]]).toStmt())).toEqual(`{someKey:1};`); + }); + + it('should break expressions into multiple lines if they are too long', () => { + const values: o.Expression[] = new Array(100); + values.fill(o.literal(1)); + values.splice(50, 0, o.fn([], [new o.ReturnStatement(o.literal(1))])); + expect(emitStmt(o.variable('fn').callFn(values).toStmt())).toEqual([ + 'fn(1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,', + ' 1,1,1,1,1,1,1,1,1,1,function() {', ' return 1;', + ' },1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,', + ' 1,1,1,1,1,1,1,1,1,1,1,1);' + ].join('\n')); }); it('should support blank literals', () => { @@ -126,9 +138,9 @@ export function main() { it('should support external identifiers', () => { expect(emitStmt(o.importExpr(sameModuleIdentifier).toStmt())).toEqual('someLocalId;'); expect(emitStmt(o.importExpr(externalModuleIdentifier).toStmt())).toEqual([ - `var import0 = re` + + `var i0 = re` + `quire('somePackage/someOtherPath');`, - `import0.someExternalId;` + `i0.someExternalId;` ].join('\n')); }); @@ -136,9 +148,9 @@ export function main() { spyOn(importResolver, 'getImportAs') .and.returnValue(new StaticSymbol('somePackage/importAsModule', 'importAsName', [])); expect(emitStmt(o.importExpr(externalModuleIdentifier).toStmt())).toEqual([ - `var import0 = re` + + `var i0 = re` + `quire('somePackage/importAsModule');`, - `import0.importAsName;` + `i0.importAsName;` ].join('\n')); }); diff --git a/packages/compiler/test/output/ts_emitter_spec.ts b/packages/compiler/test/output/ts_emitter_spec.ts index 1aec3dcbb9..962ad0ff68 100644 --- a/packages/compiler/test/output/ts_emitter_spec.ts +++ b/packages/compiler/test/output/ts_emitter_spec.ts @@ -80,8 +80,7 @@ export function main() { it('should create no reexport if the variable is not exported', () => { expect(emitStmt(someVar.set(o.importExpr(externalModuleIdentifier)).toDeclStmt())).toEqual([ - `import * as import0 from 'somePackage/someOtherPath';`, - `var someVar:any = import0.someExternalId;` + `import * as i0 from 'somePackage/someOtherPath';`, `var someVar:any = i0.someExternalId;` ].join('\n')); }); @@ -90,8 +89,8 @@ export function main() { someVar.set(o.importExpr(externalModuleIdentifier)).toDeclStmt(o.DYNAMIC_TYPE), ['someVar'])) .toEqual([ - `import * as import0 from 'somePackage/someOtherPath';`, - `export var someVar:any = import0.someExternalId;` + `import * as i0 from 'somePackage/someOtherPath';`, + `export var someVar:any = i0.someExternalId;` ].join('\n')); }); @@ -103,8 +102,8 @@ export function main() { someVar.set(o.importExpr(externalModuleIdentifierWithMembers)).toDeclStmt(), ['someVar'])) .toEqual([ - `import * as import0 from 'somePackage/someOtherPath';`, - `export var someVar:any = import0.someExternalId.a;` + `import * as i0 from 'somePackage/someOtherPath';`, + `export var someVar:any = i0.someExternalId.a;` ].join('\n')); }); @@ -195,7 +194,19 @@ export function main() { expect(emitStmt(o.literal(true).toStmt())).toEqual('true;'); expect(emitStmt(o.literal('someStr').toStmt())).toEqual(`'someStr';`); expect(emitStmt(o.literalArr([o.literal(1)]).toStmt())).toEqual(`[1];`); - expect(emitStmt(o.literalMap([['someKey', o.literal(1)]]).toStmt())).toEqual(`{someKey: 1};`); + expect(emitStmt(o.literalMap([['someKey', o.literal(1)]]).toStmt())).toEqual(`{someKey:1};`); + }); + + it('should break expressions into multiple lines if they are too long', () => { + const values: o.Expression[] = new Array(100); + values.fill(o.literal(1)); + values.splice(50, 0, o.fn([], [new o.ReturnStatement(o.literal(1))])); + expect(emitStmt(o.variable('fn').callFn(values).toStmt())).toEqual([ + 'fn(1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,', + ' 1,1,1,1,1,1,1,1,1,1,():void => {', ' return 1;', + ' },1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,', + ' 1,1,1,1,1,1,1,1,1,1,1,1);' + ].join('\n')); }); it('should apply quotes to each entry within a map produced with literalMap when true', () => { @@ -215,7 +226,7 @@ export function main() { it('should support external identifiers', () => { expect(emitStmt(o.importExpr(sameModuleIdentifier).toStmt())).toEqual('someLocalId;'); expect(emitStmt(o.importExpr(externalModuleIdentifier).toStmt())).toEqual([ - `import * as import0 from 'somePackage/someOtherPath';`, `import0.someExternalId;` + `import * as i0 from 'somePackage/someOtherPath';`, `i0.someExternalId;` ].join('\n')); }); @@ -223,7 +234,7 @@ export function main() { spyOn(importResolver, 'getImportAs') .and.returnValue(new StaticSymbol('somePackage/importAsModule', 'importAsName', [])); expect(emitStmt(o.importExpr(externalModuleIdentifier).toStmt())).toEqual([ - `import * as import0 from 'somePackage/importAsModule';`, `import0.importAsName;` + `import * as i0 from 'somePackage/importAsModule';`, `i0.importAsName;` ].join('\n')); }); @@ -423,8 +434,8 @@ export function main() { expect(emitStmt(writeVarExpr.toDeclStmt(o.importType(sameModuleIdentifier)))) .toEqual('var a:someLocalId = (null as any);'); expect(emitStmt(writeVarExpr.toDeclStmt(o.importType(externalModuleIdentifier)))).toEqual([ - `import * as import0 from 'somePackage/someOtherPath';`, - `var a:import0.someExternalId = (null as any);` + `import * as i0 from 'somePackage/someOtherPath';`, + `var a:i0.someExternalId = (null as any);` ].join('\n')); }); @@ -433,8 +444,8 @@ export function main() { .and.returnValue(new StaticSymbol('somePackage/importAsModule', 'importAsName', [])); const writeVarExpr = o.variable('a').set(o.NULL_EXPR); expect(emitStmt(writeVarExpr.toDeclStmt(o.importType(externalModuleIdentifier)))).toEqual([ - `import * as import0 from 'somePackage/importAsModule';`, - `var a:import0.importAsName = (null as any);` + `import * as i0 from 'somePackage/importAsModule';`, + `var a:i0.importAsName = (null as any);` ].join('\n')); }); @@ -449,8 +460,8 @@ export function main() { .set(o.NULL_EXPR) .toDeclStmt(o.importType(externalModuleIdentifier, [o.STRING_TYPE])))) .toEqual([ - `import * as import0 from 'somePackage/someOtherPath';`, - `var a:import0.someExternalId = (null as any);` + `import * as i0 from 'somePackage/someOtherPath';`, + `var a:i0.someExternalId = (null as any);` ].join('\n')); });