Make devices package importing explicit

[lordspacehog]

Type of change: other enhancement

Impact: no functional change

Development Phase: implementation

Release Notes
Points all diplomacy imports in devices package to standalone diplomacy library

[jerryz123]

Is there a way to configure at what point a import ._ becomes preferable? I personally dislike long lists of imports.

[lordspacehog]

Is there a way to configure at what point a import ._ becomes preferable? I personally dislike long lists of imports.

I agree that long strings of imports look cluttered, and Scalafix does has a setting for coalescing imports. (https://scalacenter.github.io/scalafix/docs/rules/OrganizeImports.html#coalescetowildcardimportthreshold.)

There's a few reasons it helps to be more explicit about our imports.

For new people wanting to contribute to the project wildcard imports obfuscate the origins of imported definitions, and that makes it challenging to get a grip on the code base.

With implicit type conversions there's also a lot of hidden functionality when you use wildcard imports vs explicit importing. This generally also helps with overall context when reading through all the implementation. This is also something that later versions of scala are trying to move away from.
(https://contributors.scala-lang.org/t/can-we-wean-scala-off-implicit-conversions/4388))

A lot of editors now have options for folding the import section, so we can both have the explicit information available to us, and also make the files appear more concise.

My main aim right now is to lean into small, ergonomic improvements with the goal of helping onboard new contributors and generally helping us lower maintenance burdens. Both inside this project, and also for downstream consumers.

[jerryz123]

While I agree that discoverability is useful, and that implicit conversions are a dangerous pattern, I'm still reluctant to blow out all the imports.

I view import rocketchip.diplomacy._ and import rocketchip.util._ in the same way as import chisel3._ and import chisel3.util._.

Perhaps in places, it would be preferable to prepend the package scope to the code, rather than the import list. Ex: extends lazymodule.LazyModule

For discoverability, I feel like better documented IDE and language-server support would do more to improve the situation.

[lordspacehog]

yeah, i totally acknowledge that this ask is somewhat out of line with other scala projects, and also that i should have brought this up as a proposal first (honestly wasn't thinking about how big a change it was, and that's on me). My main goal with the change is to simplify the amount of context you need to hold at once to understand the implementations.

I think the biggest issue for me is the fact that these wildcards masked a bunch of implicits which made getting up to speed much more difficult. I'm still kinda in that process right now as well. I've been using scala metals and have LSP and hooks all set up, it's just not as effective at really delivering that context. It can sometimes take minutes to finish the install/compile steps, and there's times when the incremental compiles will just fail to update the LSP entirely. So it's been a pretty rough experience overall during this refactor.

At the very least i think it'd be a good idea to prefer explicitly importing these implicits to de-obfuscate the type conversions that happen for things other then the chisel DSL library (i haven't done any digging to see what the impact of alternatives to wildcard importing that would be).

There's a few other code maintenance benefits to moving away from wildcards project wide as well. Explicit imports at the top make understanding changes easier and additional feature usage more noticeable. And if dependent APIs change we'll have a lot easier time actually finding what broke and either changing the import, or modifying the code to use newer APIs.

Maybe the ideal way forward is for anything that we end up using a lot of things from, we prefer namespaced useages (lazymodule.LazyModule for example). This is also related to deciding at what point the import list should be coalesced or moved to namespaced usages. i'll admit i don't have a preference here for what that limit is, since i generally don't mind large import blocks at the starts of files.

I've done some other digging as well, and there's a scalafmt config that will let us compact the grouped imports more by allowing them to be either on a single line (tbh, i think this is less readable), or set some max number of columns to target before wrapping to a new line. I absolutely think it'd be useful to have a wider discussion on what scalafmt rules/config we'd like to use here as well, and i can open an issue to discuss that.

Im going to go ahead and open an issue for discussing more about if and/or how we'd like to change code style things (including imports) so we can capture this conversation and have space to discuss more.

[jackkoenig]

Scalafix has a threshold for when it turns things into a wildcard import: https://scalacenter.github.io/scalafix/docs/rules/OrganizeImports.html#coalescetowildcardimportthreshold

I think that's generally a good thing to use, Somewhere in the range of 5-15 seems to be a good value, YMMV.

I would also note that while implicit conversions[1] are generally considered dangerous, extension methods[2] are not and the vast majority of these things are actually extension methods. While the names may suggest conversion (DataToAugmentedData, SeqBoolBitwiseOps, SeqToAugmentedSeq, BooleanToAugmentedBoolean), that's just because we (Chisel/rocket-chip devs) didn't understand the difference when most of these were created. They are implemented with a similar mechanism, but as a pattern / language feature, they are distinct.

No doubt extension methods have discoverability issues and I don't disagree with stylistic choices to import them by name (rather than by wildcard), I do want to ensure people understand that these are different than implicit conversions.

A common pattern in the Scala community these days is to put extension methods in packages called syntax and then wildcard import, eg. import freechips.rocketchip.util.syntax._. Alternatively I think something like extensions would be a decent name, or dsl. I think putting these in package util and wildcard importing does lead to some confusion, so maybe grouping them in a more intentionally named package may help a bit (and adding ScalaDoc to the package object describing that they are extension methods).

• [1] Implicit conversions are methods of the form implicit def convert(x: A): B. When in implicit scope, A will be implicitly converted to B where necessary.
• [2] Extension methods are classes of the form implicit class AExtensions(x: A) { def func ... }. When in implicit scope, you can call method .func on instances of A.

[lordspacehog]

@jackkoenig Thanks for the additional info here, I'm also generally new to scala so im still working on getting my terminology correct. scala 2.x specifically is rather confusing, my understanding is the implicit class declarations operate by implicit type conversion, where in the new extension syntax in 3 is built on the type system a little differently.

I think namespacing them is also a generally good idea, especially since it would let us ensure they're always the last import statements ensuring that there's less possibility of them getting overridden by other imports (and we can make this automatic with scalafix).

If you've got the time/energy to contribute to the discussion in #3597 it'd be great to capture context around what y'all are doing in chisel, that way this project will feel structurally similar and hopefully reduce barrier of entry for anyone coming from the chisel side.

[sequencer]

Here is my two cents: neither do I like import _ in RocketChip. This is a common case from the ancient world when the rocket-chip devs doesn't have a good IDE support, h/t those pioneers. However I think it's the wordy to import every class from chisel and and diplomacy.

I'm proposing an alignment for rocket-chip that: we can add import package._ for chisel3, chisel3.utils, diplomacy._, cde._, since these libraries are quite common and really to exist as a library. For other busips(subsystem, rocketcore) I personally wanna have a explicit importing, but will also respect others suggestions.

And for implicit conversions in rocket-chip. I suggest removing them or upstream them to chisel. I do not think RocketChip is proper place for them to live: if they are good patterns, they should live in chisel; if they are from prehistory, we should purge them and forbid downstream using them.

[jerryz123]

I'm proposing an alignment for rocket-chip that: we can add import package._ for chisel3, chisel3.utils, diplomacy._, cde._, since these libraries are quite common and really to exist as a library. For other busips(subsystem, rocketcore) I personally wanna have a explicit importing, but will also respect others suggestions.

And for implicit conversions in rocket-chip. I suggest removing them or upstream them to chisel. I do not think RocketChip is proper place for them to live: if they are good patterns, they should live in chisel; if they are from prehistory, we should purge them and forbid downstream using them.

I agree

[lordspacehog]

That seems reasonable to me, and for diplomacy we'll probably want diplomacy.(submodule)._ There's not a ton of things that provide extension methods, im down to dig into chisel and open a PR with those changes.

a quick grep shows that there's not a ton of implicit classes, so it shouldn't be hard to clear.

./amba/axi4/package.scala:  implicit class AXI4ResetDomainCrossing(private val x: HasResetDomainCrossing) extends AnyVal {
./unittest/package.scala:  implicit class LazyUnitTestSeq(val seq: Seq[LazyUnitTest]) {
./util/ElaborationArtefactAnnotation.scala:  implicit class TokensInterpolator(private val sc: StringContext) extends AnyVal {
./util/ElaborationArtefactAnnotation.scala:  implicit def stringTokenizer: Tokenizer[String] = tokenizer(StringToken(_: String))
./util/ElaborationArtefactAnnotation.scala:  implicit def modulePathTokenizer: Tokenizer[BaseModule] = tokenizer((m: BaseModule) => ModulePathToken(m.toTarget))
./util/ElaborationArtefactAnnotation.scala:  implicit def memPathTokenizer[T <: Data]: Tokenizer[SyncReadMem[T]] = tokenizer((m: SyncReadMem[_]) => MemoryPathToken(m.toTarget))
./util/ElaborationArtefactAnnotation.scala:  implicit def refPathTokenizer[T <: Data]: Tokenizer[T] = tokenizer((d: T) => ReferencePathToken(d.toTarget))
./util/package.scala:  implicit class UnzippableOption[S, T](val x: Option[(S, T)]) {
./util/package.scala:  implicit class UIntIsOneOf(private val x: UInt) extends AnyVal {
./util/package.scala:  implicit class SeqToAugmentedSeq[T <: Data](private val x: Seq[T]) extends AnyVal {
./util/package.scala:  implicit class SeqBoolBitwiseOps(private val x: Seq[Bool]) extends AnyVal {
./util/package.scala:  implicit class DataToAugmentedData[T <: Data](private val x: T) extends AnyVal {
./util/package.scala:  implicit class SeqMemToAugmentedSeqMem[T <: Data](private val x: SyncReadMem[T]) extends AnyVal {
./util/package.scala:  implicit class StringToAugmentedString(private val x: String) extends AnyVal {
./util/package.scala:  implicit def uintToBitPat(x: UInt): BitPat = BitPat(x)
./util/package.scala:  implicit def wcToUInt(c: WideCounter): UInt = c.value
./util/package.scala:  implicit class UIntToAugmentedUInt(private val x: UInt) extends AnyVal {
./util/package.scala:  implicit class OptionUIntToAugmentedOptionUInt(private val x: Option[UInt]) extends AnyVal {
./util/package.scala:  implicit class BooleanToAugmentedBoolean(private val x: Boolean) extends AnyVal {
./util/package.scala:  implicit class IntToAugmentedInt(private val x: Int) extends AnyVal {
./util/Annotations.scala:  // TODO: replace this with an implicit class from UserModule that uses getPorts
./util/Blockable.scala:  implicit def BlockableDataCanBeValid[T <: DataCanBeValid]: Blockable[T] = new Blockable[T] {
./util/Blockable.scala:  implicit def BlockableDecoupled[T <: Data]: Blockable[DecoupledIO[T]] = new Blockable[DecoupledIO[T]] {
./util/Blockable.scala:  implicit def BlockableCredited[T <: Data]: Blockable[CreditedIO[T]] = new Blockable[CreditedIO[T]] {
./util/Blockable.scala:  implicit def BlockableVec[T <: Data : Blockable]: Blockable[Vec[T]] = new Blockable[Vec[T]] {
./interrupts/package.scala:  implicit class IntClockDomainCrossing(private val x: HasClockDomainCrossing) extends AnyVal {
./interrupts/package.scala:  implicit class IntResetDomainCrossing(private val x: HasResetDomainCrossing) extends AnyVal {
./interrupts/Parameters.scala:  implicit def apply(end: Int): IntRange = apply(0, end)
./tilelink/package.scala:  implicit class TLClockDomainCrossing(private val x: HasClockDomainCrossing) extends AnyVal {
./tilelink/package.scala:  implicit class TLResetDomainCrossing(private val x: HasResetDomainCrossing) extends AnyVal {
./tilelink/ErrorEvaluator.scala:  implicit def apply(pattern: Seq[AddressSet]): RequestPattern = new RequestPattern(overlaps(pattern) _)
./jtag/package.scala:  implicit def instructionIntKeyToBigInt[V <: Chain](x: (Int, V)) = (BigInt(x._1), x._2)
./jtag/JtagStateMachine.scala:    implicit def toInt(x: State) = x.id
./jtag/JtagStateMachine.scala:    implicit def toBigInt(x: State):BigInt = x.id
./diplomacy/package.scala:  implicit class BigIntHexContext(private val sc: StringContext) extends AnyVal {
./diplomacy/package.scala:  implicit class IntToProperty(x: Int) {
./diplomacy/package.scala:  implicit class BigIntToProperty(x: BigInt) {
./diplomacy/package.scala:  implicit class StringToProperty(x: String) {
./diplomacy/package.scala:  implicit class DeviceToProperty(x: Device) {
./diplomacy/package.scala:  implicit def noCrossing(value: NoCrossing.type): ClockCrossingType = SynchronousCrossing(BufferParams.none)
./diplomacy/package.scala:  implicit def moduleValue[T](value: ModuleValue[T]): T = _root_.org.chipsalliance.diplomacy.moduleValue(value)
./diplomacy/package.scala:  implicit def SourcecodeNameExt(x: sourcecode.Name) = _root_.org.chipsalliance.diplomacy.SourcecodeNameExt(x)
./diplomacy/ClockDomain.scala:case object NoCrossing // converts to SynchronousCrossing(BufferParams.none) via implicit def in package
./diplomacy/Parameters.scala:  implicit def asBool(x: TransferSizes) = !x.none
./diplomacy/Parameters.scala:  implicit def apply(depth: Int): BufferParams = BufferParams(depth, false, false)
./diplomacy/Parameters.scala:  implicit def apply(value: Boolean): TriStateValue = TriStateValue(value, true)
./regmapper/RegField.scala:  implicit def apply(x: (Bool, Bool) => (Bool, Bool, UInt)) =
./regmapper/RegField.scala:  implicit def apply(x: RegisterReadIO[UInt]): RegReadFn =
./regmapper/RegField.scala:  implicit def apply(x: Bool => (Bool, UInt)) =
./regmapper/RegField.scala:  implicit def apply(x: ReadyValidIO[UInt]):RegReadFn = RegReadFn(ready => { x.ready := ready; (x.valid, x.bits) })
./regmapper/RegField.scala:  implicit def apply(x: UInt):RegReadFn = RegReadFn(ready => (true.B, x))
./regmapper/RegField.scala:  implicit def apply(x: Unit):RegReadFn = RegReadFn(0.U)
./regmapper/RegField.scala:  implicit def apply(x: (Bool, Bool, UInt) => (Bool, Bool)) =
./regmapper/RegField.scala:  implicit def apply(x: RegisterWriteIO[UInt]): RegWriteFn =
./regmapper/RegField.scala:  implicit def apply(x: (Bool, UInt) => Bool) =
./regmapper/RegField.scala:  implicit def apply(x: DecoupledIO[UInt]): RegWriteFn = RegWriteFn((valid, data) => { x.valid := valid; x.bits := data; x.ready })
./regmapper/RegField.scala:  implicit def apply(x: UInt): RegWriteFn = RegWriteFn((valid, data) => { when (valid) { x := data }; true.B })
./regmapper/RegField.scala:  implicit def apply(x: Unit): RegWriteFn = RegWriteFn((valid, data) => { true.B })

[lordspacehog]

closing in favor of #3602