Use PackageWarning system for multiple file overwrite errors (#2111)

* Do not assert on multiple file crashes, use PackageWarning error instead.

* Clean up error messages

Author: jcollins-g

lib/dartdoc.dart

@@ -105,7 +105,7 @@ class Dartdoc extends PackageBuilder {
 
       for (var generator in generators) {
         await generator.generate(packageGraph, outputDir.path);
-        writtenFiles.addAll(generator.writtenFiles.map(path.normalize));
+        writtenFiles.addAll(generator.writtenFiles.keys.map(path.normalize));
       }
       if (config.validateLinks && writtenFiles.isNotEmpty) {
         validateLinks(packageGraph, outputDir.path);
@@ -140,7 +140,7 @@ class Dartdoc extends PackageBuilder {
         dartdocResults.packageGraph.packageWarningCounter.errorCount;
     if (errorCount > 0) {
       throw DartdocFailure(
-          "dartdoc encountered $errorCount} errors while processing.");
+          "dartdoc encountered $errorCount errors while processing.");
     }
     logInfo(
         'Success! Docs generated into ${dartdocResults.outDir.absolute.path}');

lib/src/empty_generator.dart

@@ -5,9 +5,10 @@ import 'dart:async';
 import 'package:dartdoc/src/generator.dart';
 import 'package:dartdoc/src/model/model.dart';
 import 'package:dartdoc/src/model_utils.dart';
+import 'package:dartdoc/src/warnings.dart';
 
 /// A generator that does not generate files, but does traverse the [PackageGraph]
-/// and access [ModelElement.documetationAsHtml] for every element as though
+/// and access [ModelElement.documentationAsHtml] for every element as though
 /// it were.
 class EmptyGenerator extends Generator {
   @override
@@ -36,5 +37,5 @@ class EmptyGenerator extends Generator {
   Stream<void> get onFileCreated => _onFileCreated.stream;
 
   @override
-  Set<String> get writtenFiles => Set();
+  final Map<String, Warnable> writtenFiles = {};
 }

lib/src/generator.dart

@@ -8,6 +8,7 @@ library dartdoc.generator;
 import 'dart:async' show Stream, Future;
 
 import 'package:dartdoc/src/model/model.dart' show PackageGraph;
+import 'package:dartdoc/src/warnings.dart';
 
 /// An abstract class that defines a generator that generates documentation for
 /// a given package.
@@ -22,5 +23,5 @@ abstract class Generator {
   Stream<void> get onFileCreated;
 
   /// Fetches all filenames written by this generator.
-  Set<String> get writtenFiles;
+  Map<String, Warnable> get writtenFiles;
 }

lib/src/html/html_generator.dart

@@ -15,6 +15,7 @@ import 'package:dartdoc/src/html/html_generator_instance.dart';
 import 'package:dartdoc/src/html/template_data.dart';
 import 'package:dartdoc/src/html/templates.dart';
 import 'package:dartdoc/src/model/model.dart';
+import 'package:dartdoc/src/warnings.dart';
 import 'package:path/path.dart' as path;
 
 typedef Renderer = String Function(String input);
@@ -48,7 +49,7 @@ class HtmlGenerator extends Generator {
   Stream<void> get onFileCreated => _onFileCreated.stream;
 
   @override
-  final Set<String> writtenFiles = Set<String>();
+  final Map<String, Warnable> writtenFiles = {};
 
   static Future<HtmlGenerator> create(
       {HtmlGeneratorOptions options,
@@ -83,14 +84,23 @@ class HtmlGenerator extends Generator {
     assert(_instance == null);
 
     var enabled = true;
-    void write(String filePath, Object content, {bool allowOverwrite}) {
+    void write(String filePath, Object content,
+        {bool allowOverwrite, Warnable element}) {
       allowOverwrite ??= false;
       if (!enabled) {
         throw StateError('`write` was called after `generate` completed.');
       }
-      // If you see this assert, we're probably being called to build non-canonical
-      // docs somehow.  Check data.self.isCanonical and callers for bugs.
-      assert(allowOverwrite || !writtenFiles.contains(filePath));
+      if (!allowOverwrite) {
+        if (writtenFiles.containsKey(filePath)) {
+          assert(element != null,
+              'Attempted overwrite of ${filePath} without corresponding element');
+          Warnable originalElement = writtenFiles[filePath];
+          Iterable<Warnable> referredFrom =
+              originalElement != null ? [originalElement] : null;
+          element?.warn(PackageWarning.duplicateFile,
+              message: filePath, referredFrom: referredFrom);
+        }
+      }
 
       var file = File(path.join(outputDirectoryPath, filePath));
       var parent = file.parent;
@@ -107,7 +117,7 @@ class HtmlGenerator extends Generator {
             content, 'content', '`content` must be `String` or `List<int>`.');
       }
       _onFileCreated.add(file);
-      writtenFiles.add(filePath);
+      writtenFiles[filePath] = element;
     }
 
     try {

lib/src/html/html_generator_instance.dart

@@ -21,7 +21,7 @@ import 'package:mustache/mustache.dart';
 import 'package:path/path.dart' as path;
 
 typedef FileWriter = void Function(String path, Object content,
-    {bool allowOverwrite});
+    {bool allowOverwrite, Warnable element});
 
 class HtmlGeneratorInstance {
   final HtmlGeneratorOptions _options;
@@ -412,8 +412,8 @@ class HtmlGeneratorInstance {
     // Replaces '/' separators with proper separators for the platform.
     String outFile = path.joinAll(filename.split('/'));
     String content = template.renderString(data);
-
-    _writer(outFile, content);
+    _writer(outFile, content,
+        element: data.self is Warnable ? data.self : null);
     if (data.self is Indexable) _indexedElements.add(data.self as Indexable);
   }
 }

lib/src/model/package_graph.dart

@@ -405,6 +405,11 @@ class PackageGraph {
       case PackageWarning.unresolvedExport:
         warningMessage = 'unresolved export uri: ${message}';
         break;
+      case PackageWarning.duplicateFile:
+        warningMessage = 'failed to write file at: ${message}';
+        warnablePrefix = 'for symbol';
+        referredFromPrefix = 'conflicting with file already generated by';
+        break;
     }
 
     List<String> messageParts = [warningMessage];

lib/src/warnings.dart

@@ -193,6 +193,19 @@ final Map<PackageWarning, PackageWarningDefinition> packageWarningDefinitions =
       "unresolved-export",
       "An export refers to a URI that cannot be resolved.",
       defaultWarningMode: PackageWarningMode.error),
+  PackageWarning.duplicateFile: PackageWarningDefinition(
+      PackageWarning.duplicateFile,
+      "duplicate-file",
+      "Dartdoc is trying to write to a duplicate filename based on the names of Dart symbols.",
+      longHelp: [
+        "Dartdoc generates a path and filename to write to for each symbol.",
+        "@@name@@ conflicts with another symbol in the generated path, and",
+        "therefore can not be written out.  Changing the name, library name, or",
+        "class name (if appropriate) of one of the conflicting items can resolve",
+        "the conflict.   Alternatively, use the @nodoc tag in one symbol's",
+        "documentation comments to hide it."
+      ],
+      defaultWarningMode: PackageWarningMode.error),
 };
 
 /// Something that package warnings can be called on.  Optionally associated
@@ -222,6 +235,7 @@ enum PackageWarning {
   unknownMacro,
   unknownHtmlFragment,
   brokenLink,
+  duplicateFile,
   orphanedFile,
   unknownFile,
   missingFromSearchIndex,

test/html_generator_test.dart

@@ -9,9 +9,13 @@ import 'dart:io' show File, Directory;
 import 'package:dartdoc/src/html/html_generator.dart';
 import 'package:dartdoc/src/html/templates.dart';
 import 'package:dartdoc/src/html/resources.g.dart';
+import 'package:dartdoc/src/model/package_graph.dart';
+import 'package:dartdoc/src/warnings.dart';
 import 'package:path/path.dart' as path;
 import 'package:test/test.dart';
 
+import 'src/utils.dart' as utils;
+
 void main() {
   group('Templates', () {
     Templates templates;
@@ -90,6 +94,37 @@ void main() {
         }
       });
     });
+
+    group('for a package that causes duplicate files', () {
+      HtmlGenerator generator;
+      PackageGraph packageGraph;
+      Directory tempOutput;
+
+      setUp(() async {
+        generator = await HtmlGenerator.create();
+        packageGraph = await utils
+            .bootBasicPackage(utils.testPackageDuplicateDir.path, []);
+        tempOutput = await Directory.systemTemp.createTemp('doc_test_temp');
+      });
+
+      tearDown(() {
+        if (tempOutput != null) {
+          tempOutput.deleteSync(recursive: true);
+        }
+      });
+
+      test('run generator and verify duplicate file error', () async {
+        await generator.generate(packageGraph, tempOutput.path);
+        expect(generator, isNotNull);
+        expect(tempOutput, isNotNull);
+        String expectedPath =
+            path.join('aDuplicate', 'aDuplicate-library.html');
+        expect(
+            packageGraph.localPublicLibraries,
+            anyElement((l) => packageGraph.packageWarningCounter
+                .hasWarning(l, PackageWarning.duplicateFile, expectedPath)));
+      }, timeout: Timeout.factor(2));
+    });
   });
 }
 

test/src/utils.dart

@@ -59,6 +59,8 @@ Future<PackageGraph> get testPackageGraphSdk =>
 
 final Directory testPackageBadDir = Directory('testing/test_package_bad');
 final Directory testPackageDir = Directory('testing/test_package');
+final Directory testPackageDuplicateDir =
+    Directory('testing/test_package_duplicate');
 final Directory testPackageExperimentsDir =
     Directory('testing/test_package_experiments');
 final Directory testPackageMinimumDir =

testing/test_package_duplicate/lib/first.dart

@@ -0,0 +1 @@
+library aDuplicate;