Skip to content

Commit

Permalink
Fix 'Malformed class name' errors in typeName and related implementat…
Browse files Browse the repository at this point in the history
…ions (#3533)

- Provide a public simpleClassName utility to generate simple class names in a non-error prone way, particularly when generating inner class names. All typeNames are modified to now use this utility.
- Mixing HasAutoTypename into anonymous bundles is now prohibited by the compiler, as this inevitably leads to name conflicts.
  • Loading branch information
jared-barocsi authored Sep 28, 2023
1 parent 556b2ca commit 4186303
Show file tree
Hide file tree
Showing 8 changed files with 67 additions and 38 deletions.
3 changes: 2 additions & 1 deletion core/src/main/scala/chisel3/Bits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import chisel3.internal.sourceinfo.{
import chisel3.internal.firrtl.PrimOp._
import _root_.firrtl.{ir => firrtlir}
import chisel3.internal.{castToInt, Builder, Warning, WarningID}
import chisel3.util.simpleClassName

/** Exists to unify common interfaces of [[Bits]] and [[Reset]].
*
Expand Down Expand Up @@ -54,7 +55,7 @@ sealed abstract class Bits(private[chisel3] val width: Width) extends Element wi
/** A non-ambiguous name of this `Bits` instance for use in generated Verilog names
* Inserts the width directly after the typeName, e.g. UInt4, SInt1
*/
override def typeName: String = s"${this.getClass.getSimpleName}$width"
override def typeName: String = s"${simpleClassName(this.getClass)}$width"

/** Tail operator
*
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/scala/chisel3/Data.scala
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import chisel3.internal._
import chisel3.internal.sourceinfo._
import chisel3.internal.firrtl._
import chisel3.reflect.DataMirror
import chisel3.util.simpleClassName

import scala.reflect.ClassTag
import scala.util.Try
Expand Down Expand Up @@ -781,7 +782,7 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc {
def toPrintable: Printable

/** A non-ambiguous name of this `Data` for use in generated Verilog names */
def typeName: String = this.getClass.getSimpleName
def typeName: String = simpleClassName(this.getClass)
}

object Data {
Expand Down
17 changes: 2 additions & 15 deletions core/src/main/scala/chisel3/Module.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import chisel3.reflect.DataMirror
import _root_.firrtl.annotations.{IsModule, ModuleName, ModuleTarget}
import _root_.firrtl.AnnotationSeq
import chisel3.internal.plugin.autoNameRecursively
import chisel3.util.simpleClassName

object Module extends SourceInfoDoc {

Expand Down Expand Up @@ -518,21 +519,7 @@ package experimental {
*
* @note If you want a custom or parametric name, override this method.
*/
def desiredName: String = {
/* The default module name is derived from the Java reflection derived class name. */
val baseName = this.getClass.getName

/* A sequence of string filters applied to the name */
val filters: Seq[String => String] =
Seq(((a: String) => raw"\$$+anon".r.replaceAllIn(a, "_Anon")) // Merge the "$$anon" name with previous name
)

filters
.foldLeft(baseName) { case (str, filter) => filter(str) } // 1. Apply filters to baseName
.split("\\.|\\$") // 2. Split string at '.' or '$'
.filterNot(_.forall(_.isDigit)) // 3. Drop purely numeric names
.last // 4. Use the last name
}
def desiredName: String = simpleClassName(this.getClass)

/** Legalized name of this module. */
final lazy val name =
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package chisel3.experimental

import chisel3.{Data, Record}
import chisel3.internal.{sanitize}
import chisel3.internal.{sanitize, Builder}
import chisel3.util.simpleClassName

/** Trait for [[Record]]s that signals the compiler plugin to generate a typeName for the
* inheriting [[Record]] based on the parameter values provided to its constructor.
Expand Down Expand Up @@ -37,12 +38,13 @@ trait HasAutoTypename {

/** Auto generate a type name for this Bundle using the bundle arguments supplied by the compiler plugin.
*/
override def typeName: String = autoTypeName(getClass.getSimpleName, _typeNameConParams)
override def typeName: String = autoTypeName(simpleClassName(this.getClass), _typeNameConParams)

private def autoTypeName(bundleName: String, typeNameParams: Iterable[Any]): String =
private def autoTypeName(bundleName: String, typeNameParams: Iterable[Any]): String = {
_typeNameConParams.foldLeft(sanitize(bundleName)) {
case (prev, accessor) => prev + s"_${accessorString(accessor)}"
}
}

private def accessorString(accessor: Any): String = accessor match {
case d: Data => d.typeName
Expand Down
23 changes: 23 additions & 0 deletions core/src/main/scala/chisel3/util/Naming.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package chisel3.util

/* Generates a safe 'simple class name' from the given class, avoiding `Malformed class name` exceptions from `getClass.getSimpleName`
* when Java 8 is used.
*/
object simpleClassName {

def apply[T](clazz: Class[T]): String = {
/* The default class name is derived from the Java reflection derived class name. */
val baseName = clazz.getName

/* A sequence of string filters applied to the name */
val filters: Seq[String => String] =
Seq(((a: String) => raw"\$$+anon".r.replaceAllIn(a, "_Anon")) // Merge the "$$anon" name with previous name
)

filters
.foldLeft(baseName) { case (str, filter) => filter(str) } // 1. Apply filters to baseName
.split("\\.|\\$") // 2. Split string at '.' or '$'
.filterNot(_.forall(_.isDigit)) // 3. Drop purely numeric names
.last // 4. Use the last name
}
}
43 changes: 28 additions & 15 deletions plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala
Original file line number Diff line number Diff line change
Expand Up @@ -177,21 +177,34 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi
elementsImpl
}

def generateAutoTypename(bundle: ClassDef, thiz: global.This, conArgsOpt: Option[List[Tree]]): Option[Tree] = {
conArgsOpt.map { conArgs =>
// Create an iterable out of all constructor argument accessors
val typeNameConParamsSym =
bundle.symbol.newMethod(
TermName("_typeNameConParams"),
bundle.symbol.pos.focus,
Flag.OVERRIDE | Flag.PROTECTED
def generateAutoTypename(record: ClassDef, thiz: global.This, conArgsOpt: Option[List[Tree]]): Option[Tree] = {
conArgsOpt.flatMap { conArgs =>
val recordIsAnon = record.symbol.isAnonOrRefinementClass

// Anonymous `Records` in any scope are forbidden
if (record.symbol.isAnonOrRefinementClass) {
global.reporter.error(
record.pos,
"Users cannot mix 'HasAutoTypename' into an anonymous Record. Create a named class."
)
typeNameConParamsSym.resetFlag(Flags.METHOD)
typeNameConParamsSym.setInfo(NullaryMethodType(itAnyTpe))

localTyper.typed(
DefDef(typeNameConParamsSym, q"scala.collection.immutable.Vector.apply[Any](..${conArgs})")
)
None
} else {
// Create an iterable out of all constructor argument accessors
val typeNameConParamsSym =
record.symbol.newMethod(
TermName("_typeNameConParams"),
record.symbol.pos.focus,
Flag.OVERRIDE | Flag.PROTECTED
)
typeNameConParamsSym.resetFlag(Flags.METHOD)
typeNameConParamsSym.setInfo(NullaryMethodType(itAnyTpe))

Some(
localTyper.typed(
DefDef(typeNameConParamsSym, q"scala.collection.immutable.Vector.apply[Any](..${conArgs})")
)
)
}
}
}

Expand Down Expand Up @@ -245,7 +258,7 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi
}

val autoTypenameOpt =
if (isBundle && isAutoTypenamed(record.symbol)) generateAutoTypename(record, thiz, conArgs.map(_.flatten))
if (isAutoTypenamed(record.symbol)) generateAutoTypename(record, thiz, conArgs.map(_.flatten))
else None

val withMethods = deriveClassDef(record) { t =>
Expand Down
3 changes: 2 additions & 1 deletion src/main/scala/chisel3/util/Decoupled.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package chisel3.util
import chisel3._
import chisel3.experimental.{requireIsChiselType, Direction}
import chisel3.reflect.DataMirror
import chisel3.util.simpleClassName

import scala.annotation.nowarn

Expand Down Expand Up @@ -40,7 +41,7 @@ abstract class ReadyValidIO[+T <: Data](gen: T) extends Bundle {
/** A stable typeName for this `ReadyValidIO` and any of its implementations
* using the supplied `Data` generator's `typeName`
*/
override def typeName = s"${this.getClass.getSimpleName}_${gen.typeName}"
override def typeName = s"${simpleClassName(this.getClass)}_${gen.typeName}"
}

object ReadyValidIO {
Expand Down
5 changes: 3 additions & 2 deletions src/main/scala/chisel3/util/Valid.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package chisel3.util

import chisel3._
import chisel3.experimental.prefix
import chisel3.util.simpleClassName

/** A [[Bundle]] that adds a `valid` bit to some data. This indicates that the user expects a "valid" interface between
* a producer and a consumer. Here, the producer asserts the `valid` bit when data on the `bits` line contains valid
Expand Down Expand Up @@ -40,7 +41,7 @@ class Valid[+T <: Data](gen: T) extends Bundle {
/** A non-ambiguous name of this `Valid` instance for use in generated Verilog names
* Inserts the parameterized generator's typeName, e.g. Valid_UInt4
*/
override def typeName = s"${this.getClass.getSimpleName}_${gen.typeName}"
override def typeName = s"${simpleClassName(this.getClass)}_${gen.typeName}"
}

/** Factory for generating "valid" interfaces. A "valid" interface is a data-communicating interface between a producer
Expand Down Expand Up @@ -182,7 +183,7 @@ class Pipe[T <: Data](val gen: T, val latency: Int = 1) extends Module {
* Includes the latency cycle count in the name as well as the parameterized
* generator's `typeName`, e.g. `Pipe4_UInt4`
*/
override def desiredName = s"${this.getClass.getSimpleName}${latency}_${gen.typeName}"
override def desiredName = s"${simpleClassName(this.getClass)}${latency}_${gen.typeName}"

/** Interface for [[Pipe]]s composed of a [[Valid]] input and [[Valid]] output
* @define notAQueue
Expand Down

0 comments on commit 4186303

Please sign in to comment.