[Scala3] Update BundleSelect to support refined structural type access#5362
[Scala3] Update BundleSelect to support refined structural type access#5362adkian-sifive wants to merge 6 commits into
Conversation
jackkoenig
left a comment
There was a problem hiding this comment.
I don't think this is the right way to fix whatever it's fixing, and trying to understand why this fixes anything made me realize something:
I think our whole approach to Selectable is slightly wrong. We're implementing it for the Data fields that we support (and this PR fixes it for fields whos names don't match their sanitized names), but Selectable should also work for non-Data fields, consider:
val io = IO(new Bundle {
val out = Output(UInt(8.W))
val foo = 3
})Our current implementation works for io.out, but it should also work for io.foo (and does in Scala 2).
Scala 3 provides an implementation of the reflective implementation in scala.reflect.Selectable. Now we could fix this by just mixing that in stead of scala.Selectable but I don't think we should that either.
If we ever want to move away from supporting Selectable (and urging users away from using anonymous types), it would be a hard breaking change to move to scala.Selectable from scala.reflect.Selectable. It is better to stick with scala.Selectable but still use the reflective implementation which we can do a number of different ways, but I think the easiest way is:
extension (b: Bundle) {
def selectDynamic(field: String): Any = {
val impl = new scala.reflect.Selectable {
override protected def selectedValue = b
}
impl.selectDynamic(field)
}
}This basically lets us continue mixin scala.Selectable but get the benefits of the reflective implementation.
Relatedly, I'm not sure why we have Record mixing in Selectable but then have this extension on Bundle, do you know why?
|
I feel like we shouldn't support non-Data fields being selectable -- I see non-data fields, like |
|
Although I guess if Scala 2 supports it we shouldn't be breaking it just yet, lemme try the |
0d88c01 to
25e87ad
Compare
This issue cropped up in tests.
BundleSelectdid not support refining types which led to errors on anonymous Bundles. After these changes the structural refinement more closely mimics Scala 2 behaviorContributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash)and clean up the commit message.Create a merge commit.