Repository: intel/rohd (dart)

Issue:
Custom definition names of `Module`s are not sanitized
## Describe the bug
It is possible to specify a `definitionName` that is not sanitary (as sanitized by `Sanitizer`), which means you can generate invalid SystemVerilog.

## To Reproduce
Define a module with an unsanitary `definitionName` passed to `Module`'s constructor.

## Expected behavior
ROHD sanitizes the definition name so that it generates valid SystemVerilog.

## Actual behavior
The unsanitary definition name is included directly in the generated SystemVerilog.

**Additional context**  
Sanitization may as well occur directly within the constructor.

Pull request:
<!-- Please make sure you check out the contribution guidelines before submitting a pull request! -->

## Description & Motivation
Fix bug of the ``definition name`` is not sanitized on ``Module`` class extend. 

<!-- Description of changes, and motivation for adding them. -->

## Related Issue(s)
Issue #138 

<!-- If there are any issues related to this PR, please link to the issues here -->

## Testing

- ``dart test`` passed on local. 
- Add a new unit test to test for the sanitization on the unclean variable name

## 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/exceptions/exceptions.dart b/lib/src/exceptions/exceptions.dart
new file mode 100644
index 000000000..bad954400
--- /dev/null
+++ b/lib/src/exceptions/exceptions.dart
@@ -0,0 +1,4 @@
+/// Copyright (C) 2022 Intel Corporation
+/// SPDX-License-Identifier: BSD-3-Clause
+
+export 'name_exceptions.dart';
diff --git a/lib/src/exceptions/name_exceptions.dart b/lib/src/exceptions/name_exceptions.dart
new file mode 100644
index 000000000..7dc5f7b86
--- /dev/null
+++ b/lib/src/exceptions/name_exceptions.dart
@@ -0,0 +1,48 @@
+/// Copyright (C) 2022 Intel Corporation
+/// SPDX-License-Identifier: BSD-3-Clause
+///
+/// name_exception.dart
+/// Name Exception that have custom type to be thrown,
+///
+/// 2022 October 25
+/// Author: Yao Jing Quek <yao.jing.quek@intel.com>
+///
+import 'package:rohd/rohd.dart';
+
+/// This Exception show that reserved name eg. definitionName is NULL but
+/// reserve flag eg. reserveDefinitionName is set to True.
+///
+/// Please check on the class [Module] for the constructor argument.
+class NullReservedNameException implements Exception {
+  late final String _message;
+
+  /// constructor for NullReservedNameException,
+  /// pass custom message to the constructor
+  NullReservedNameException(
+      [String message = 'Reserved Name cannot be null '
+          'if reserved name set to true'])
+      : _message = message;
+
+  @override
+  String toString() => _message;
+}
+
+/// This Exception show that reserved name eg. definitionName naming convention
+/// is invalid but reserve flag eg. reserveDefinitionName is set to True.
+/// Please check on the syntax of the reservedName.
+///
+/// Please check on the class [Module] for the constructor argument.
+class InvalidReservedNameException implements Exception {
+  late final String _message;
+
+  /// constructor for InvalidReservedNameException,
+  /// pass custom message to the constructor
+  InvalidReservedNameException(
+      [String message = 'Reserved Name need to follow proper naming '
+          'convention if reserved'
+          ' name set to true'])
+      : _message = message;
+
+  @override
+  String toString() => _message;
+}
diff --git a/lib/src/logic.dart b/lib/src/logic.dart
index cbf93d653..ec9671bdf 100644
--- a/lib/src/logic.dart
+++ b/lib/src/logic.dart
@@ -8,11 +8,12 @@
 /// 2021 August 2
 /// Author: Max Korbel <max.korbel@intel.com>
 ///
-
 import 'dart:async';
+
 import 'package:collection/collection.dart';
 import 'package:meta/meta.dart';
 import 'package:rohd/rohd.dart';
+import 'package:rohd/src/utilities/sanitizer.dart';
 import 'package:rohd/src/utilities/synchronous_propagator.dart';
 
 /// Represents the event of a [Logic] changing value.
@@ -218,7 +219,7 @@ class Logic {
   /// The default value for [width] is 1.  The [name] should be synthesizable
   /// to the desired output (e.g. SystemVerilog).
   Logic({String? name, this.width = 1})
-      : name = name ?? 's${_signalIdx++}',
+      : name = name == null ? 's${_signalIdx++}' : Sanitizer.sanitizeSV(name),
         _currentValue = LogicValue.filled(width, LogicValue.z) {
     if (width < 0) {
       throw Exception('Logic width must be greater than or equal to 0.');
diff --git a/lib/src/module.dart b/lib/src/module.dart
index b187a185c..a6208998a 100644
--- a/lib/src/module.dart
+++ b/lib/src/module.dart
@@ -7,12 +7,13 @@
 /// 2021 May 7
 /// Author: Max Korbel <max.korbel@intel.com>
 ///
-
 import 'dart:async';
 import 'dart:collection';
+
 import 'package:collection/collection.dart';
 import 'package:meta/meta.dart';
 import 'package:rohd/rohd.dart';
+import 'package:rohd/src/exceptions/name_exceptions.dart';
 import 'package:rohd/src/utilities/sanitizer.dart';
 import 'package:rohd/src/utilities/uniquifier.dart';
 
@@ -122,6 +123,23 @@ abstract class Module {
           '  Call build() before accessing this.');
   String _uniqueInstanceName;
 
+  /// Return string type definition name if validation passed
+  /// else throw exception.
+  ///
+  /// This validation method ensure that [definitionName] is valid if
+  /// [reserveDefinitionName] set to `true`.
+  static String? _nameValidation(
+      String? definitionName, bool reserveDefinitionName) {
+    if (reserveDefinitionName && definitionName == null) {
+      throw NullReservedNameException();
+    } else if (reserveDefinitionName &&
+        !Sanitizer.isSanitary(definitionName!)) {
+      throw InvalidReservedNameException();
+    } else {
+      return definitionName;
+    }
+  }
+
   /// If true, guarantees [uniqueInstanceName] matches [name] or else the
   /// [build] will fail.
   final bool reserveName;
@@ -134,7 +152,9 @@ abstract class Module {
   ///
   /// This could become uniquified by a [Synthesizer] unless
   /// [reserveDefinitionName] is set.
-  String get definitionName => _definitionName ?? runtimeType.toString();
+  String get definitionName =>
+      Sanitizer.sanitizeSV(_definitionName ?? runtimeType.toString());
+
   final String? _definitionName;
 
   /// If true, guarantees [definitionName] is maintained by a [Synthesizer],
@@ -155,7 +175,8 @@ abstract class Module {
       String? definitionName,
       this.reserveDefinitionName = false})
       : _uniqueInstanceName = name,
-        _definitionName = definitionName;
+        _definitionName =
+            _nameValidation(definitionName, reserveDefinitionName);
 
   /// Returns an [Iterable] of [Module]s representing the hierarchical path to
   /// this [Module].


Test patch:
diff --git a/test/definition_name_test.dart b/test/definition_name_test.dart
new file mode 100644
index 000000000..080a0a222
--- /dev/null
+++ b/test/definition_name_test.dart
@@ -0,0 +1,99 @@
+/// Copyright (C) 2022 Intel Corporation
+/// SPDX-License-Identifier: BSD-3-Clause
+///
+/// definition_name_test.dart
+/// Unit tests for name definition usage
+///
+/// 2022 October 26
+/// Author: Yao Jing Quek <yao.jing.quek@intel.com>
+///
+import 'package:rohd/rohd.dart';
+import 'package:rohd/src/exceptions/name_exceptions.dart';
+import 'package:test/test.dart';
+
+class DefinitionName {
+  String? name;
+  bool isReserved;
+
+  DefinitionName({required this.name, required this.isReserved});
+
+  static String getValidName() => 'specialName';
+  static String getInvalidName() => '/--**definitionName+';
+}
+
+class ValidDefNameModule extends Module {
+  ValidDefNameModule(Logic a, DefinitionName defName)
+      : super(
+          name: 'specialNameInstance',
+          reserveName: false,
+          definitionName: defName.name,
+          reserveDefinitionName: defName.isReserved,
+        ) {
+    addInput('a', a, width: a.width);
+  }
+}
+
+// ignore: camel_case_types
+class byte extends Module {
+  byte(Logic a) {
+    addInput('a', a, width: a.width);
+  }
+}
+
+void main() {
+  group('GIVEN that Reserved Definition Name is TRUE,', () {
+    test('WHEN definition name is valid, THEN expect to compile successfully.',
+        () async {
+      final defName =
+          DefinitionName(name: DefinitionName.getValidName(), isReserved: true);
+      final mod = ValidDefNameModule(Logic(), defName);
+      expect(mod.definitionName, equals('specialName'));
+    });
+    test('WHEN definition name is invalid, THEN expect to throw exception.',
+        () async {
+      final defName = DefinitionName(
+          name: DefinitionName.getInvalidName(), isReserved: true);
+      expect(() async {
+        ValidDefNameModule(Logic(), defName);
+      }, throwsA((dynamic e) => e is InvalidReservedNameException));
+    });
+    test('WHEN definition name is null, THEN expect to throw exception.',
+        () async {
+      final defName = DefinitionName(name: null, isReserved: true);
+      expect(() async {
+        ValidDefNameModule(Logic(), defName);
+      }, throwsA((dynamic e) => e is NullReservedNameException));
+    });
+  });
+
+  group('GIVEN that Reserved Definition Name is FALSE,', () {
+    test(
+        'WHEN definition name is valid, THEN expected to compile successfully.',
+        () async {
+      final defName = DefinitionName(
+          name: DefinitionName.getValidName(), isReserved: false);
+      final mod = ValidDefNameModule(Logic(), defName);
+      expect(mod.definitionName, equals('specialName'));
+    });
+    test('WHEN definition name is invalid, THEN expected to sanitize name.',
+        () async {
+      final defName = DefinitionName(
+          name: DefinitionName.getInvalidName(), isReserved: false);
+      final mod = ValidDefNameModule(Logic(), defName);
+      expect(mod.definitionName, equals('_____definitionName_'));
+    });
+    group('WHEN definition name is null,', () {
+      test('THEN expected to auto initialize name.', () async {
+        final defName = DefinitionName(name: null, isReserved: false);
+        final mod = ValidDefNameModule(Logic(), defName);
+        expect(mod.definitionName, equals('ValidDefNameModule'));
+      });
+      test(
+          'AND runtime type name is invalid, '
+          'THEN expect to sanitize the result', () async {
+        final mod = byte(Logic());
+        expect(mod.definitionName, equals('byte_'));
+      });
+    });
+  });
+}
diff --git a/test/logic_name_test.dart b/test/logic_name_test.dart
new file mode 100644
index 000000000..ef8b498d1
--- /dev/null
+++ b/test/logic_name_test.dart
@@ -0,0 +1,39 @@
+/// Copyright (C) 2022 Intel Corporation
+/// SPDX-License-Identifier: BSD-3-Clause
+///
+/// logic_name_test.dart
+/// Unit tests for logic name initialization
+///
+/// 2022 October 26
+/// Author: Yao Jing Quek <yao.jing.quek@intel.com>
+///
+
+import 'package:rohd/rohd.dart';
+import 'package:test/test.dart';
+
+class LogicTestModule extends Module {
+  LogicTestModule(Logic a) {
+    addInput(a.name, a, width: a.width);
+  }
+}
+
+void main() {
+  test(
+      'GIVEN logic name is valid '
+      'THEN expected to see proper name being generated', () async {
+    final bus = Logic(name: 'validName');
+    expect(bus.name, equals('validName'));
+  });
+
+  test('GIVEN logic name is invalid THEN expected to see sanitized name',
+      () async {
+    final bus = Logic(name: '&*-FinvalidN11Me');
+    expect(bus.name, equals('___FinvalidN11Me'));
+  });
+
+  test('GIVEN logic name is null THEN expected to see autogeneration of name',
+      () async {
+    final bus = Logic();
+    expect(bus.name, equals('s0'));
+  });
+}
diff --git a/test/name_test.dart b/test/name_test.dart
index af60043c3..0a29608fc 100644
--- a/test/name_test.dart
+++ b/test/name_test.dart
@@ -8,7 +8,6 @@
 /// Author: Max Korbel <max.korbel@intel.com>
 
 // ignore_for_file: avoid_positional_boolean_parameters
-
 import 'package:rohd/rohd.dart';
 import 'package:test/test.dart';