fix(dotnet): documentation strings sometimes invalid (#1127)

* fix(dotnet): documentation strings sometimes invalid

The XML-doc generation for the C# classes was not correctly accounting
for the necesaity to encode certain characters (e.g: `&`) for use in XML
context, which resulted in invalid documentation strings being generated.
Additionally, slugified parameter names need to be represented in their
"pure" form (without the `@` slug) in the `<param>` tag.

This uses `xmlbuilder` to generate the minimal XML tag pairs, ensuring
the content is correctly encoded. The `@` slug is rmeoved from `<param>`
tags, and the `<summary>` block is generated inconditionally (even if
empty), silencing a C# warning about there being no documentation XML on
a public member.

Finally, moved some `await` in `for` loops out, to hopefully positively
impact the generation speeds overall.

* fix linter offenses

packages/jsii-pacmak/bin/jsii-pacmak.ts

@@ -149,7 +149,6 @@ import { ALL_BUILDERS, TargetName } from '../lib/targets';
 
     // We run all target sets in parallel for minimal wall clock time
     await Promise.all(targetSets.map(async targetSet => {
-    // for (const targetSet of targetSets) {
       logging.info(`Packaging '${targetSet.targetType}' for ${describePackages(targetSet)}`);
       await timers.recordAsync(targetSet.targetType, () =>
         buildTargetsForLanguage(targetSet.targetType, targetSet.modules, perLanguageDirectory)

packages/jsii-pacmak/lib/builder.ts

@@ -66,13 +66,10 @@ export class OneByOneBuilder implements TargetBuilder {
   }
 
   public async buildModules(): Promise<void> {
-    for (const module of this.modules) {
-      if (this.options.codeOnly) {
-        await this.generateModuleCode(module, this.options);
-      } else {
-        await this.buildModule(module, this.options);
-      }
-    }
+    const promises = this.modules.map(module => this.options.codeOnly
+      ? this.generateModuleCode(module, this.options)
+      : this.buildModule(module, this.options));
+    await Promise.all(promises);
   }
 
   private async generateModuleCode(module: JsiiModule, options: BuildOptions) {

packages/jsii-pacmak/lib/npm-modules.ts

@@ -18,9 +18,10 @@ import { topologicalSort } from './toposort';
 export async function findJsiiModules(directories: string[], recurse: boolean) {
   const ret: JsiiModule[] = [];
   const visited = new Set<string>();
-  for (const dir of directories.length > 0 ? directories : ['.']) {
-    await visitPackage(dir, true);
-  }
+
+  const toVisit = directories.length > 0 ? directories : ['.'];
+  await Promise.all(toVisit.map(dir => visitPackage(dir, true)));
+
   return topologicalSort(ret, m => m.name, m => m.dependencyNames);
 
   async function visitPackage(dir: string, isRoot: boolean) {
@@ -45,10 +46,8 @@ export async function findJsiiModules(directories: string[], recurse: boolean) {
 
     // if --recurse is set, find dependency dirs and build them.
     if (recurse) {
-      for (const dep of dependencyNames) {
-        const depDir = resolveDependencyDirectory(realPath, dep);
-        await visitPackage(depDir, false);
-      }
+      await Promise.all(dependencyNames.map(dep => resolveDependencyDirectory(realPath, dep))
+        .map(depDir => visitPackage(depDir, false)));
     }
 
     // outdir is either by package.json/jsii.outdir (relative to package root) or via command line (relative to cwd)
@@ -66,10 +65,7 @@ export async function findJsiiModules(directories: string[], recurse: boolean) {
 }
 
 export async function updateAllNpmIgnores(packages: JsiiModule[]) {
-  for (const pkg of packages) {
-    // updates .npmignore to exclude the output directory and include the .jsii file
-    await updateNpmIgnore(pkg.moduleDirectory, pkg.outputDirectory);
-  }
+  await Promise.all(packages.map(pkg => updateNpmIgnore(pkg.moduleDirectory, pkg.outputDirectory)));
 }
 
 async function updateNpmIgnore(packageDir: string, excludeOutdir: string | undefined) {

packages/jsii-pacmak/lib/target.ts

@@ -61,6 +61,8 @@ export abstract class Target {
      * @param targetDir the directory to copy into.
      */
   protected async copyFiles(sourceDir: string, targetDir: string) {
+    // Preemptively create target directory, to avoid unsafely racing on it's creation.
+    await fs.mkdirp(targetDir);
     await fs.copy(sourceDir, targetDir, { recursive: true });
   }
 

packages/jsii-pacmak/lib/targets/dotnet.ts

@@ -9,6 +9,8 @@ import { DotNetGenerator } from './dotnet/dotnetgenerator';
 import { TargetBuilder, BuildOptions } from '../builder';
 import { JsiiModule } from '../packaging';
 
+export const TARGET_FRAMEWORK = 'netcoreapp3.0';
+
 /**
  * Build .NET packages all together, by generating an aggregate solution file
  */
@@ -23,9 +25,7 @@ export class DotnetBuilder implements TargetBuilder {
 
     if (this.options.codeOnly) {
       // Simple, just generate code to respective output dirs
-      for (const module of this.modules) {
-        await this.generateModuleCode(module, this.outputDir(module.outputDirectory));
-      }
+      await Promise.all(this.modules.map(module => this.generateModuleCode(module, this.outputDir(module.outputDirectory))));
       return;
     }
 
@@ -56,13 +56,14 @@ export class DotnetBuilder implements TargetBuilder {
       const csProjs = [];
       const ret: TemporaryDotnetPackage[] = [];
 
-      for (const module of modules) {
-        // Code generator will make its own subdirectory
-        await this.generateModuleCode(module, tmpDir);
-        const loc = projectLocation(module);
+      // Code generator will make its own subdirectory
+      const generatedModules = modules.map(mod => this.generateModuleCode(mod, tmpDir).then(() => mod));
+
+      for await (const mod of generatedModules) {
+        const loc = projectLocation(mod);
         csProjs.push(loc.projectFile);
         ret.push({
-          outputTargetDirectory: module.outputDirectory,
+          outputTargetDirectory: mod.outputDirectory,
           artifactsDir: path.join(tmpDir, loc.projectDir, 'bin', 'Release')
         });
       }
@@ -79,14 +80,19 @@ export class DotnetBuilder implements TargetBuilder {
 
   private async copyOutArtifacts(packages: TemporaryDotnetPackage[]) {
     logging.debug('Copying out .NET artifacts');
-    for (const pkg of packages) {
+
+    await Promise.all(packages.map(copyOutIndividualArtifacts.bind(this)));
+
+    async function copyOutIndividualArtifacts(this: DotnetBuilder, pkg: TemporaryDotnetPackage) {
       const targetDirectory = this.outputDir(pkg.outputTargetDirectory);
 
       await fs.mkdirp(targetDirectory);
-      await fs.copy(pkg.artifactsDir, targetDirectory, { recursive: true });
-
-      // This copies more than we need, remove the directory with the bare assembly again
-      await fs.remove(path.join(targetDirectory, 'netcoreapp3.0'));
+      await fs.copy(pkg.artifactsDir, targetDirectory, {
+        recursive: true,
+        filter: (_, dst) => {
+          return dst !== path.join(targetDirectory, TARGET_FRAMEWORK);
+        },
+      });
     }
   }
 
@@ -112,8 +118,13 @@ export class DotnetBuilder implements TargetBuilder {
     // an <outdir>/dotnet directory. We will add those as local NuGet repositories.
     // This enables building against local modules.
     const allDepsOutputDirs = new Set<string>();
-    for (const module of this.modules) {
-      setExtend(allDepsOutputDirs, await findLocalBuildDirs(module.moduleDirectory, this.targetName));
+
+    const resolvedModules = this.modules.map(async module => ({
+      module,
+      localBuildDirs: await findLocalBuildDirs(module.moduleDirectory, this.targetName),
+    }));
+    for await (const { module, localBuildDirs } of resolvedModules) {
+      setExtend(allDepsOutputDirs, localBuildDirs);
 
       // Also include output directory where we're building to, in case we build multiple packages into
       // the same output directory.

packages/jsii-pacmak/lib/targets/dotnet/dotnetdocgenerator.ts

@@ -1,5 +1,6 @@
 import { CodeMaker } from 'codemaker';
 import * as spec from '@jsii/spec';
+import * as xmlbuilder from 'xmlbuilder';
 import { DotNetNameUtils } from './nameutils';
 import { prefixMarkdownTsCodeBlocks } from '../../util';
 
@@ -33,22 +34,15 @@ export class DotNetDocGenerator {
     const docs = obj.docs;
 
     // The docs may be undefined at the method level but not the parameters level
-    if (docs) {
-      if (docs.summary) {
-        this.code.line(`/// <summary>${docs.summary}</summary>`);
-      }
-    }
+    this.emitXmlDoc('summary', docs?.summary || '');
 
     // Handling parameters only if the obj is a method
     const objMethod = obj as spec.Method;
     if (objMethod.parameters) {
       objMethod.parameters.forEach(param => {
-        if (param.docs) {
-          const paramSummary = param.docs.summary;
-          if (paramSummary) {
-            this.code.line(`/// <param name = "${this.nameutils.convertParameterName(param.name)}">${paramSummary}</param>`);
-          }
-        }
+        // Remove any slug `@` from the parameter name - it's not supposed to show up here.
+        const paramName = this.nameutils.convertParameterName(param.name).replace(/^@/, '');
+        this.emitXmlDoc('param', param.docs?.summary || '', { attributes: { name: paramName } });
       });
     }
 
@@ -58,68 +52,63 @@ export class DotNetDocGenerator {
     }
 
     if (docs.returns) {
-      this.code.line('/// <returns>');
-      const returnsLines = docs.returns.split('\n');
-      returnsLines.forEach( line => this.code.line(`/// ${line}`));
-      this.code.line('/// </returns>');
+      this.emitXmlDoc('returns', docs.returns);
     }
 
-    const remarks: string[] = [];
-    let remarksOpen = false;
+    const remarks = xmlbuilder.create('remarks', { headless: true });
     if (docs.remarks) {
-      this.code.line('/// <remarks>');
-      remarksOpen = true;
-      const remarkLines = prefixMarkdownTsCodeBlocks(docs.remarks, SAMPLES_DISCLAIMER).split('\n');
-      remarkLines.forEach( line => this.code.line(`/// ${line}`));
+      remarks.text(`\n${prefixMarkdownTsCodeBlocks(docs.remarks, SAMPLES_DISCLAIMER)}\n`);
     }
 
     if (docs.default) {
-      const defaultLines = docs.default.split('\n');
-      remarks.push('default:');
-      defaultLines.forEach( line => remarks.push(`${line}`));
+      remarks.text(`\ndefault:\n${docs.default}\n`);
     }
 
     if (docs.stability) {
-      remarks.push(`stability: ${this.nameutils.capitalizeWord(docs.stability)}`);
+      remarks.text(`\nstability: ${this.nameutils.capitalizeWord(docs.stability)}\n`);
     }
 
     if (docs.example) {
-      const remarkLines = docs.example.split('\n');
-      remarks.push('example:');
-      remarks.push('<code>');
-      remarks.push('// Examples in C# are coming soon.');
-      remarkLines.forEach( line => remarks.push(`${line}`));
-      remarks.push('</code>');
+      remarks.text('\nexample:\n');
+      remarks.ele('code')
+        .text('\n// Examples in C# are coming soon.\n')
+        .text(`${docs.example}\n`);
+      remarks.text('\n');
     }
 
     if (docs.see) {
-      const seeLines = docs.see.split('\n');
-      remarks.push('see:');
-      seeLines.forEach( line => remarks.push(`${line}`));
+      remarks.text(`\nsee:\n${docs.see}\n`);
     }
 
     if (docs.subclassable) {
-      remarks.push('subclassable');
+      remarks.text('\nsubclassable\n');
     }
 
     if (docs.custom) {
       for (const [k, v] of Object.entries(docs.custom ?? {})) {
         const custom = k === 'link' ? `${k}: ${v} ` : `${k}: ${v}`; // Extra space for '@link' to keep unit tests happy
-        const customLines = custom.split('\n');
-        customLines.forEach( line => remarks.push(`${line}`));
+        remarks.text(`\n${custom}\n`);
       }
     }
 
-    if (remarks.length > 0) {
-      if (!remarksOpen) {
-        this.code.line('/// <remarks>');
-        remarksOpen = true;
+    const remarksXml = remarks.end({ allowEmpty: true });
+    if (remarksXml !== '<remarks></remarks>') {
+      for (const line of remarksXml.split('\n')) {
+        this.code.line(`/// ${line}`);
       }
-      remarks.forEach( line => this.code.line(`/// ${line}`));
     }
+  }
+
+  private emitXmlDoc(tag: string, content: string, { attributes = {} }: { attributes?: { [name: string]: string } } = {}): void {
+    const xml = xmlbuilder.create(tag, { headless: true })
+      .text(content);
+    for (const [name, value] of Object.entries(attributes)) {
+      xml.att(name, value);
+    }
+    const xmlstring = xml.end({ allowEmpty: true, pretty: false });
 
-    if (remarksOpen) {
-      this.code.line('/// </remarks>');
+    for (const line of xmlstring.split('\n').map(x => x.trim())) {
+      this.code.line(`/// ${line}`);
     }
   }
 }

packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts

@@ -7,7 +7,7 @@ import { Generator } from '../../generator';
 import { DotNetDocGenerator } from './dotnetdocgenerator';
 import { DotNetRuntimeGenerator } from './dotnetruntimegenerator';
 import { DotNetTypeResolver } from './dotnettyperesolver';
-import { DotNetDependency, FileGenerator } from './filegenerator';
+import { FileGenerator } from './filegenerator';
 import { DotNetNameUtils } from './nameutils';
 
 /**
@@ -73,12 +73,9 @@ export class DotNetGenerator extends Generator {
     const assm = this.assembly;
     const packageId: string = assm.targets!.dotnet!.packageId;
     if (!packageId) { throw new Error(`The module ${assm.name} does not have a dotnet.packageId setting`); }
-    await fs.mkdirs(path.join(outdir, packageId));
+    await fs.mkdirp(path.join(outdir, packageId));
     await fs.copyFile(tarball, path.join(outdir, packageId, tarballFileName));
 
-    // Create an anchor file for the current model
-    this.generateDependencyAnchorFile();
-
     // Copying the .jsii file
     await fs.copyFile(this.jsiiFilePath, path.join(outdir, packageId, spec.SPEC_FILE_NAME));
 
@@ -87,31 +84,15 @@ export class DotNetGenerator extends Generator {
   }
 
   /**
-     * Generates the Anchor file
-     */
-  protected generateDependencyAnchorFile(): void {
-    const namespace = `${this.assembly.targets!.dotnet!.namespace}.Internal.DependencyResolution`;
-    this.openFileIfNeeded('Anchor', namespace, false, false);
-    this.code.openBlock('public class Anchor');
-    this.code.openBlock('public Anchor()');
-    this.typeresolver.namespaceDependencies.forEach((value: DotNetDependency) => {
-      this.code.line(`new ${value.namespace}.Internal.DependencyResolution.Anchor();`);
-    });
-    this.code.closeBlock();
-    this.code.closeBlock();
-    this.closeFileIfNeeded('Anchor', namespace, false);
-  }
-
-  /**
-     * Not used as we override the save() method
-     */
+   * Not used as we override the save() method
+   */
   protected getAssemblyOutputDir(mod: spec.Assembly): string {
     return this.nameutils.convertPackageName(mod.name);
   }
 
   /**
-     * Namespaces are handled implicitly by openFileIfNeeded().
-     */
+   * Namespaces are handled implicitly by openFileIfNeeded().
+   */
   protected onBeginNamespace(_ns: string) { /* noop */ }
 
   protected onEndNamespace(_ns: string) { /* noop */ }
@@ -269,11 +250,15 @@ export class DotNetGenerator extends Generator {
       this.code.line();
     }
 
+    this.code.line('/// <summary>Used by jsii to construct an instance of this class from a Javascript-owned object reference</summary>');
+    this.code.line('/// <param name="reference">The Javascript-owned object reference</param>');
     this.dotnetRuntimeGenerator.emitDeprecatedAttributeIfNecessary(initializer);
     this.code.openBlock(`protected ${className}(ByRefValue reference): base(reference)`);
     this.code.closeBlock();
     this.code.line();
 
+    this.code.line('/// <summary>Used by jsii to construct an instance of this class from DeputyProps</summary>');
+    this.code.line('/// <param name="props">The deputy props</param>');
     this.dotnetRuntimeGenerator.emitDeprecatedAttributeIfNecessary(initializer);
     this.code.openBlock(`protected ${className}(DeputyProps props): base(props)`);
     this.code.closeBlock();

packages/jsii-pacmak/lib/targets/dotnet/filegenerator.ts

@@ -5,6 +5,7 @@ import * as xmlbuilder from 'xmlbuilder';
 import { DotNetNameUtils } from './nameutils';
 import * as logging from '../../logging';
 import { nextMajorVersion } from '../../util';
+import { TARGET_FRAMEWORK } from '../dotnet';
 
 // Represents a dependency in the dependency tree.
 export class DotNetDependency {
@@ -80,7 +81,7 @@ export class FileGenerator {
     propertyGroup.ele('IncludeSymbols', 'true');
     propertyGroup.ele('IncludeSource', 'true');
     propertyGroup.ele('SymbolPackageFormat', 'snupkg');
-    propertyGroup.ele('TargetFramework', 'netcoreapp3.0');
+    propertyGroup.ele('TargetFramework', TARGET_FRAMEWORK);
 
     if (dotnetInfo!.signAssembly != null) {
       const signAssembly = propertyGroup.ele('SignAssembly');
@@ -115,6 +116,9 @@ export class FileGenerator {
       }
     });
 
+    // Suppress warnings about [Obsolete] members, this is the author's choice!
+    rootNode.ele('PropertyGroup').ele('NoWarn').text('0612,0618');
+
     const xml = rootNode.end({ pretty: true, spaceBeforeSlash: true });
 
     // Sending the xml content to the codemaker to ensure the file is written