[go_router_builder] Report duplicate route paths at build time

packages/go_router_builder/CHANGELOG.md

@@ -1,3 +1,15 @@
+## 4.5.0
+
+- Detects routes that resolve to the same URL pattern. Routes are compared by
+  the whole URL each one resolves to, so a collision is caught wherever the two
+  routes sit in the route tree, including across shell routes and
+  `StatefulShellRoute` branches, between relative routes, and between separate
+  annotations in one library. Paths that differ only in a parameter name, or
+  only in casing where the earlier route sets `caseSensitive: false`, count as
+  the same pattern. These are reported as build warnings by default. The new
+  `duplicate_route_paths` builder option raises them to build errors with
+  `error`, or silences them with `ignore`.
+
 ## 4.4.0
 
 - Adds `hasOverriddenOnExit` parameter to `GoRouteData.$route` and `RelativeGoRouteData.$route` helper methods for type-safe routes. When set to `true`, enables custom `onExit` callback invocation from route data classes extending `GoRouteData` or `RelativeGoRouteData` when the route is removed from the navigation stack.

packages/go_router_builder/README.md

@@ -42,6 +42,49 @@ dart run build_runner build
 Read more about using
 [`build_runner` on pub.dev](https://pub.dev/packages/build_runner).
 
+### Builder options
+
+#### `duplicate_route_paths`
+
+When two routes resolve to the same URL, `go_router` matches the first and the
+second becomes unreachable. Navigating to the second one's `location` shows the
+first one's page. The builder warns about this at build time.
+
+Routes are compared by the URL they resolve to, not by the path they declare, so
+depth in the route tree does not matter. A route at `section/detail` collides with
+a route at `section` holding a child at `detail`. Parameter names are ignored, so
+`product/:id` and `product/:productId` are the same URL, though a parameter's
+regex constraint still counts, so `product/:id(\d+)` and `product/:id(\w+)` are
+not. Casing is ignored when the earlier route sets `caseSensitive: false`, since
+it then matches any casing.
+
+The whole library is compared, `part` files included. Shell routes and
+`StatefulShellRoute` branches contribute nothing to the URLs beneath them.
+
+Use `build.yaml` to change what a duplicate does:
+
+```yaml
+targets:
+  $default:
+    builders:
+      go_router_builder:
+        options:
+          duplicate_route_paths: error
+```
+
+Accepted values are `warning` (the default), `error`, and `ignore`.
+
+Warning is the default because some duplicates work. Matching backtracks, so
+naming one route class twice at the same path, each declaration carrying
+different children, is fine. Both resolve to the same class and every child stays
+reachable, which makes it a way to group children by feature. The builder still
+warns, because it cannot tell that from a mistake, so use `ignore` if you write
+it deliberately. Their children are a different story: a child path repeated
+across the two declarations is a genuine collision and is reported on its own.
+
+`error` applies to the whole package with no per-route exception, so it fails on
+the deliberate grouping too.
+
 ## Migration Guides
 - [Migrating to 4.0.0](https://flutter.dev/go/go-router-builder-v4-breaking-changes).
 
@@ -505,4 +548,13 @@ Relative routing methods are not idempotent and will cause an error when the rel
 
 To run unit tests, run command `dart tool/run_tests.dart` from `packages/go_router_builder/`.
 
+Each `.dart` file in `test_inputs/` is a test case, paired with a `.expect` file
+holding either the generated output or the error message the builder must
+produce. Two optional companion files tune a case:
+
+* `<name>.dart.options` holds a JSON map of builder options, matching what
+  `build.yaml` would pass to the builder.
+* `<name>.dart.warnings` holds the warnings the builder must log, one per line.
+  An empty file asserts that the builder logs nothing.
+
 To run tests in examples, run `flutter test` from `packages/go_router_builder/example`.

packages/go_router_builder/example/lib/all_extension_types.dart

@@ -26,9 +26,6 @@ part 'all_extension_types.g.dart';
     ),
     TypedGoRoute<IntExtensionRoute>(path: 'int-route/:requiredIntField'),
     TypedGoRoute<NumExtensionRoute>(path: 'num-route/:requiredNumField'),
-    TypedGoRoute<DoubleExtensionRoute>(
-      path: 'double-route/:requiredDoubleField',
-    ),
     TypedGoRoute<EnumExtensionRoute>(path: 'enum-route/:requiredEnumField'),
     TypedGoRoute<EnhancedEnumExtensionRoute>(
       path: 'enhanced-enum-route/:requiredEnumField',

packages/go_router_builder/example/lib/all_types.dart

@@ -20,7 +20,6 @@ part 'all_types.g.dart';
     TypedGoRoute<DoubleRoute>(path: 'double-route/:requiredDoubleField'),
     TypedGoRoute<IntRoute>(path: 'int-route/:requiredIntField'),
     TypedGoRoute<NumRoute>(path: 'num-route/:requiredNumField'),
-    TypedGoRoute<DoubleRoute>(path: 'double-route/:requiredDoubleField'),
     TypedGoRoute<EnumRoute>(path: 'enum-route/:requiredEnumField'),
     TypedGoRoute<EnhancedEnumRoute>(
       path: 'enhanced-enum-route/:requiredEnumField',

packages/go_router_builder/lib/go_router_builder.dart

@@ -15,11 +15,13 @@ library go_router_builder;
 import 'package:build/build.dart';
 import 'package:source_gen/source_gen.dart';
 
+import 'src/duplicate_path_severity.dart';
 import 'src/go_router_generator.dart';
 
 /// Supports `package:build_runner` creation and configuration of
 /// `go_router`.
 ///
 /// Not meant to be invoked by hand-authored code.
-Builder goRouterBuilder(BuilderOptions options) =>
-    SharedPartBuilder(const <Generator>[GoRouterGenerator()], 'go_router');
+Builder goRouterBuilder(BuilderOptions options) => SharedPartBuilder(<Generator>[
+  GoRouterGenerator(duplicatePathSeverity: duplicatePathSeverityFromOptions(options)),
+], 'go_router');

packages/go_router_builder/lib/src/duplicate_path_severity.dart

@@ -0,0 +1,56 @@
+// Copyright 2013 The Flutter Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:build/build.dart';
+import 'package:collection/collection.dart';
+
+/// The `build.yaml` option that selects a [DuplicatePathSeverity].
+const String duplicateRoutePathsOption = 'duplicate_route_paths';
+
+/// How the builder reports sibling routes that resolve to the same URL pattern.
+enum DuplicatePathSeverity {
+  /// Duplicate paths are not reported at all.
+  ignore,
+
+  /// Duplicate paths are reported as build warnings.
+  ///
+  /// Code is still generated for every route. This is the default, because a
+  /// duplicate path is legal at runtime and is not always dead code.
+  ///
+  /// `go_router` tries sibling routes in declaration order and takes the first
+  /// one that matches the whole URL. So when two different route classes share
+  /// a path, navigating to the second class's location lands on the first
+  /// class's page, which is almost always a mistake. But matching backtracks:
+  /// when a route matches only a prefix and none of its children complete the
+  /// URL, matching moves on to the next sibling. Declaring one route class
+  /// twice with different children is therefore sound, and is one way to group
+  /// children by feature area. Both shapes are reported, since the builder
+  /// cannot tell a deliberate grouping from an accidental duplicate.
+  warning,
+
+  /// Duplicate paths fail the build.
+  error,
+}
+
+/// Reads the [DuplicatePathSeverity] from `build.yaml` builder [options].
+///
+/// Defaults to [DuplicatePathSeverity.warning] when the option is absent.
+DuplicatePathSeverity duplicatePathSeverityFromOptions(BuilderOptions options) {
+  final Object? value = options.config[duplicateRoutePathsOption];
+  if (value == null) {
+    return DuplicatePathSeverity.warning;
+  }
+  final DuplicatePathSeverity? severity = DuplicatePathSeverity.values.firstWhereOrNull(
+    (DuplicatePathSeverity severity) => severity.name == value,
+  );
+  if (severity == null) {
+    throw ArgumentError.value(
+      value,
+      duplicateRoutePathsOption,
+      'Must be one of '
+      '${DuplicatePathSeverity.values.map((DuplicatePathSeverity e) => e.name).join(', ')}',
+    );
+  }
+  return severity;
+}

packages/go_router_builder/lib/src/go_router_generator.dart

@@ -9,6 +9,7 @@ import 'package:analyzer/dart/element/type.dart';
 import 'package:build/build.dart';
 import 'package:source_gen/source_gen.dart';
 
+import 'duplicate_path_severity.dart';
 import 'route_config.dart';
 import 'type_helpers.dart';
 
@@ -25,7 +26,10 @@ const Map<String, String> _annotations = <String, String>{
 /// A [Generator] for classes annotated with a typed go route annotation.
 class GoRouterGenerator extends Generator {
   /// Creates a new instance of [GoRouterGenerator].
-  const GoRouterGenerator();
+  const GoRouterGenerator({this.duplicatePathSeverity = DuplicatePathSeverity.warning});
+
+  /// How sibling routes that resolve to the same URL pattern are reported.
+  final DuplicatePathSeverity duplicatePathSeverity;
 
   TypeChecker get _typeChecker => TypeChecker.any(
     _annotations.keys.map((String annotation) => TypeChecker.fromUrl('$_routeDataUrl#$annotation')),
@@ -57,17 +61,26 @@ ${getters.map((String e) => "$e,").join('\n')}
   /// This public method is for testing purposes and should not be called
   /// directly.
   void generateForAnnotation(LibraryReader library, Set<String> values, Set<String> getters) {
+    // Every annotation in the library contributes a top-level route to the
+    // generated `$appRoutes`, so they all have to be built before their paths
+    // can be compared against each other.
+    final configs = <RouteBaseConfig>[];
     for (final AnnotatedElement annotatedElement in library.annotatedWith(_typeChecker)) {
-      final InfoIterable generatedValue = _generateForAnnotatedElement(
-        annotatedElement.element,
-        annotatedElement.annotation,
+      configs.add(
+        _configForAnnotatedElement(annotatedElement.element, annotatedElement.annotation),
       );
+    }
+
+    reportDuplicateRoutePaths(configs, duplicatePathSeverity);
+
+    for (final config in configs) {
+      final InfoIterable generatedValue = config.generateMembers();
       getters.add(generatedValue.routeGetterName);
       values.addAll(generatedValue.members);
     }
   }
 
-  InfoIterable _generateForAnnotatedElement(Element element, ConstantReader annotation) {
+  RouteBaseConfig _configForAnnotatedElement(Element element, ConstantReader annotation) {
     final String typedAnnotation = withoutNullability(
       annotation.objectValue.type!.getDisplayString(),
     );
@@ -89,6 +102,6 @@ ${getters.map((String e) => "$e,").join('\n')}
       );
     }
 
-    return RouteBaseConfig.fromAnnotation(annotation, element).generateMembers();
+    return RouteBaseConfig.fromAnnotation(annotation, element);
   }
 }

packages/go_router_builder/lib/src/path_utils.dart

@@ -18,6 +18,21 @@ Set<String> pathParametersFromPattern(String pattern) => <String>{
   for (final RegExpMatch match in _parameterRegExp.allMatches(pattern)) match[1]!,
 };
 
+/// Replaces the parameter names in a [pattern] with a placeholder, so that
+/// patterns differing only in those names compare as equal.
+///
+/// A parameter's regex constraint is kept, since it changes which URLs the
+/// pattern matches.
+///
+/// For example:
+///
+/// ```dart
+/// normalizePathParameters('item/:id'); // 'item/:_'
+/// normalizePathParameters(r'item/:id(\d+)'); // r'item/:_(\d+)'
+/// ```
+String normalizePathParameters(String pattern) =>
+    pattern.replaceAllMapped(_parameterRegExp, (Match match) => ':_${match[2] ?? ''}');
+
 /// Reconstructs the full path from a [pattern] and path parameters.
 ///
 /// For example: