Repository: intel/rohd (dart)

Issue:
In some scenarios where an output depends on an output, `Module` build can fail
## Describe the bug
The algorithm that determines which `Module`s are within which other `Module`s via input and output tracing has a bug which reveals itself in certain situations when outputs depend on other outputs of the same `Module`.

## To Reproduce
```dart
class TopModule extends Module {
  TopModule(Logic a) {
    a = addInput('a_top', a);
    SubModule(a);
  }
}

class SubModule extends Module {
  SubModule(Logic a) {
    a = addInput('a', a);
    var b = addOutput('b');
    var c = addOutput('c');
    b <= a;
    c <= b;
  }
}

void main() {
  test('out depends on out', () async {
    var mod = TopModule(Logic());
    await mod.build();
  });
}
```

## Expected behavior
Builds properly

## Actual behavior
Exception due to "Violation of input/output rules".  Fortunately ROHD identified something was wrong rather than generate something invalid.

## Additional details
**Please paste the output of `dart --version`**  
```
$ dart --version
Dart SDK version: 2.16.1 (stable) (Unknown timestamp) on "linux_x64"
```

**Please paste the contents of your pubspec.yaml file and identify the version of ROHD you're using**  
Unchanged for ROHD

Pull request:
<!-- Please make sure you check out the contribution guidelines before submitting a pull request! -->

## Description & Motivation

- Fixes a bug (#84) where in certain scenarios with an output depending on an output of the same module the hierarchy failed to be determined.
- Added a check against inputs depending on other inputs

## Related Issue(s)

Fix #84 

## Testing

- Added new tests that reproduce #84 and confirm the fix
- Added a new test for the new input check to ensure the `Exception` was hit
- Existing test suite protects old functionality

## Backwards-compatibility

> Is this a breaking change that will not be backwards-compatible?  If yes, how so?

No

## Documentation

> Does the change require any updates to documentation?  If so, where?  Are they included?

No

Patch:
diff --git a/lib/src/module.dart b/lib/src/module.dart
index 200856200..8849e2ed7 100644
--- a/lib/src/module.dart
+++ b/lib/src/module.dart
@@ -1,4 +1,4 @@
-/// Copyright (C) 2021 Intel Corporation
+/// Copyright (C) 2021-2022 Intel Corporation
 /// SPDX-License-Identifier: BSD-3-Clause
 ///
 /// module.dart
@@ -98,6 +98,9 @@ abstract class Module {
   /// Returns true iff [net] is the same [Logic] as the output port of this [Module] with the same name.
   bool isOutput(Logic net) => _outputs[net.name] == net;
 
+  /// Returns true iff [net] is the same [Logic] as an input or output port of this [Module] with the same name.
+  bool isPort(Logic net) => isInput(net) || isOutput(net);
+
   /// If this module has a [parent], after [build()] this will be a guaranteed unique name within its scope.
   String get uniqueInstanceName => hasBuilt
       ? _uniqueInstanceName
@@ -157,15 +160,11 @@ abstract class Module {
     // construct the list of modules within this module
     // 1) trace from outputs of this module back to inputs of this module
     for (var output in _outputs.values) {
-      _traceOutputForModuleContents(output, dontAddSignal: true);
+      await _traceOutputForModuleContents(output, dontAddSignal: true);
     }
     // 2) trace from inputs of all modules to inputs of this module
     for (var input in _inputs.values) {
-      _traceInputForModuleContents(input, dontAddSignal: true);
-    }
-
-    for (var module in _modules) {
-      await module.build();
+      await _traceInputForModuleContents(input, dontAddSignal: true);
     }
 
     // set unique module instance names for submodules
@@ -179,7 +178,7 @@ abstract class Module {
   }
 
   /// Adds a [Module] to this as a subModule.
-  void _addModule(Module module) {
+  Future<void> _addAndBuildModule(Module module) async {
     if (module.parent != null) {
       throw Exception('This Module "$this" already has a parent. '
           'If you are hitting this as a user of ROHD, please file '
@@ -190,6 +189,7 @@ abstract class Module {
       _modules.add(module);
     }
     module._parent = this;
+    await module.build();
   }
 
   /// A prefix to add to the beginning of any port name that is "unpreferred".
@@ -216,8 +216,8 @@ abstract class Module {
   }
 
   /// Searches for [Logic]s and [Module]s within this [Module] from its inputs.
-  void _traceInputForModuleContents(Logic signal,
-      {bool dontAddSignal = false}) {
+  Future<void> _traceInputForModuleContents(Logic signal,
+      {bool dontAddSignal = false}) async {
     if (isOutput(signal)) return;
 
     if (!signal.isInput && !signal.isOutput && signal.parentModule != null) {
@@ -231,8 +231,7 @@ abstract class Module {
 
     if (!dontAddSignal && signal.isOutput) {
       // somehow we have reached the output of a module which is not a submodule nor this module, bad!
-      //TODO: add tests that this exception hits!
-      throw Exception('Violation of input/output rules.'
+      throw Exception('Violation of input/output rules in $this on $signal.'
           '  Logic within a Module should only consume inputs and drive outputs of that Module.'
           '  See https://github.com/intel/rohd#modules for more information.');
     }
@@ -247,27 +246,45 @@ abstract class Module {
         (subModuleParent == null || subModuleParent == this)) {
       // if the subModuleParent hasn't been set, or it is the current module, then trace it
       if (subModuleParent != this) {
-        _addModule(subModule);
+        await _addAndBuildModule(subModule);
       }
       for (var subModuleOutput in subModule._outputs.values) {
-        _traceInputForModuleContents(subModuleOutput, dontAddSignal: true);
+        await _traceInputForModuleContents(subModuleOutput,
+            dontAddSignal: true);
       }
       for (var subModuleInput in subModule._inputs.values) {
-        _traceOutputForModuleContents(subModuleInput, dontAddSignal: true);
+        await _traceOutputForModuleContents(subModuleInput,
+            dontAddSignal: true);
       }
     } else {
       if (!dontAddSignal && !isInput(signal) && subModule == null) {
         _addInternalSignal(signal);
       }
+
+      if (!dontAddSignal && isInput(signal)) {
+        throw Exception('Input $signal of module $this is dependent on'
+            ' another input of the same module.');
+      }
+
       for (var dstConnection in signal.dstConnections) {
-        _traceInputForModuleContents(dstConnection);
+        if (signal.isOutput &&
+            dstConnection.isOutput &&
+            signal.parentModule! == dstConnection.parentModule!) {
+          // since both are outputs, we can't easily use them to
+          // check if they have already been traversed, so we must
+          // explicitly check that we're not running them back-to-back.
+          // another iteration will take care of continuing the trace
+          continue;
+        }
+
+        await _traceInputForModuleContents(dstConnection);
       }
     }
   }
 
   /// Searches for [Logic]s and [Module]s within this [Module] from its outputs.
-  void _traceOutputForModuleContents(Logic signal,
-      {bool dontAddSignal = false}) {
+  Future<void> _traceOutputForModuleContents(Logic signal,
+      {bool dontAddSignal = false}) async {
     if (isInput(signal)) return;
 
     if (!signal.isInput && !signal.isOutput && signal.parentModule != null) {
@@ -281,7 +298,7 @@ abstract class Module {
 
     if (!dontAddSignal && signal.isInput) {
       // somehow we have reached the input of a module which is not a submodule nor this module, bad!
-      throw Exception('Violation of input/output rules.'
+      throw Exception('Violation of input/output rules in $this on $signal.'
           '  Logic within a Module should only consume inputs and drive outputs of that Module.'
           '  See https://github.com/intel/rohd#modules for more information.');
     }
@@ -296,26 +313,30 @@ abstract class Module {
         (subModuleParent == null || subModuleParent == this)) {
       // if the subModuleParent hasn't been set, or it is the current module, then trace it
       if (subModuleParent != this) {
-        _addModule(subModule);
+        await _addAndBuildModule(subModule);
       }
       for (var subModuleInput in subModule._inputs.values) {
-        _traceOutputForModuleContents(subModuleInput, dontAddSignal: true);
+        await _traceOutputForModuleContents(subModuleInput,
+            dontAddSignal: true);
       }
       for (var subModuleOutput in subModule._outputs.values) {
-        _traceInputForModuleContents(subModuleOutput, dontAddSignal: true);
+        await _traceInputForModuleContents(subModuleOutput,
+            dontAddSignal: true);
       }
     } else {
       if (!dontAddSignal && !isOutput(signal) && subModule == null) {
         _addInternalSignal(signal);
       }
       if (signal.srcConnection != null) {
-        _traceOutputForModuleContents(signal.srcConnection!);
+        await _traceOutputForModuleContents(signal.srcConnection!);
       }
     }
   }
 
   /// Registers a signal as an internal signal.
   void _addInternalSignal(Logic signal) {
+    assert(!signal.isPort);
+
     _internalSignals.add(signal);
 
     // ignore: invalid_use_of_protected_member


Test patch:
diff --git a/test/trace_bounce_test.dart b/test/trace_bounce_test.dart
new file mode 100644
index 000000000..310f9c593
--- /dev/null
+++ b/test/trace_bounce_test.dart
@@ -0,0 +1,57 @@
+/// Copyright (C) 2022 Intel Corporation
+/// SPDX-License-Identifier: BSD-3-Clause
+///
+/// trace_bounce_test.dart
+///
+/// 2022 March 4
+/// Author: Max Korbel <max.korbel@intel.com>
+///
+
+import 'package:rohd/rohd.dart';
+import 'package:rohd/src/utilities/simcompare.dart';
+import 'package:test/test.dart';
+
+class TopModule extends Module {
+  TopModule(Logic a) : super(name: 'topmodule') {
+    a = addInput('a_top', a);
+    var bundle = addOutput('bundle_top', width: 3);
+    bundle <= SubModule(a).bundle;
+  }
+}
+
+class SubModule extends Module {
+  Logic get bundle => output('bundle');
+  SubModule(Logic a) : super(name: 'submodule') {
+    a = addInput('a', a);
+    var b = addOutput('b');
+    var c = addOutput('c');
+    var d = addOutput('d');
+    var e = addOutput('e');
+    var bundle = addOutput('bundle', width: 3);
+
+    b <= a;
+    c <= b;
+    d <= (Logic()..gets(b));
+    e <= ~b;
+    bundle <= [c, d, e].swizzle();
+  }
+}
+
+void main() {
+  test('out depends on out', () async {
+    var mod = TopModule(Logic());
+    await mod.build();
+
+    var vectors = [
+      Vector({'a_top': 0}, {'bundle_top': 1}),
+      Vector({'a_top': 1}, {'bundle_top': 6}),
+    ];
+    await SimCompare.checkFunctionalVector(mod, vectors);
+    var simResult = SimCompare.iverilogVector(
+        mod.generateSynth(), mod.runtimeType.toString(), vectors,
+        signalToWidthMap: {'bundle_top': 3});
+    expect(simResult, equals(true));
+  });
+
+  //TODO: what if someone mistakenly assigns an input to another input within the module?
+}
diff --git a/test/trace_test.dart b/test/trace_test.dart
index 97ffe91b0..3a9001a3c 100644
--- a/test/trace_test.dart
+++ b/test/trace_test.dart
@@ -1,7 +1,7 @@
-/// Copyright (C) 2021 Intel Corporation
+/// Copyright (C) 2021-2022 Intel Corporation
 /// SPDX-License-Identifier: BSD-3-Clause
 ///
-/// trace_dest.dart
+/// trace_test.dart
 ///
 /// 2021 July 22
 /// Author: Max Korbel <max.korbel@intel.com>
@@ -42,6 +42,20 @@ class BadSubModuleIn extends Module {
   }
 }
 
+class DoubledInputModule extends Module {
+  DoubledInputModule(Logic a) : super(name: 'doubledinput') {
+    var aInner = addInput('a', a);
+    addInput('b', aInner);
+  }
+}
+
+class DoubledGappedInputModule extends Module {
+  DoubledGappedInputModule(Logic a) : super(name: 'doubledgappedinput') {
+    var aInner = addInput('a', a);
+    addInput('b', Logic()..gets(~aInner));
+  }
+}
+
 void main() {
   tearDown(() {
     Simulator.reset();
@@ -60,4 +74,18 @@ void main() {
       await mod.build();
     }, throwsException);
   });
+
+  test('doubled input', () async {
+    var mod = DoubledInputModule(Logic());
+    expect(() async {
+      await mod.build();
+    }, throwsException);
+  });
+
+  test('doubled gapped input', () async {
+    var mod = DoubledGappedInputModule(Logic());
+    expect(() async {
+      await mod.build();
+    }, throwsException);
+  });
 }