Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bundles with structurally typed fields in compatibility mode fail in Scala 2.12 #606

Closed
ucbjrl opened this issue May 3, 2017 · 11 comments
Closed
Labels
long-term Long-term unresolved issues
Milestone

Comments

@ucbjrl
Copy link
Contributor

ucbjrl commented May 3, 2017

Update (2019): this will continue to be an issue as structural types are going away in Scala 2.13/14+ and Scala 3.

Compiled with Scala 2.12, the following code:

    import Chisel._
    val myReset = true.B
    class ModuleExplicitReset(reset: Bool) extends Module(_reset = reset) {
      val io = new Bundle {
        val done = Bool(OUTPUT)
      }
      io.done := false.B
    }

produces

Error:(14, 10) value done is not a member of Chisel.Bundle
      io.done := false.B

Wrapping the io definition in IO() or extending chisel3.core.UserModule instead of chisel3.core.LegacyModuleeliminates the error. It seems to be a property of the LegacyModule definition.

@kammoh
Copy link
Contributor

kammoh commented May 3, 2017

This is actually coming from a deeper issue with recent versions of the Scala compiler, going back at least to 2.12.0-M5 and also present in dotty, and actually has nothing to do with Chisel:

scala> trait B {
     | }
defined trait B

scala> 

scala> trait BaseClass {
     |   val x : B
     | }
defined trait BaseClass

scala> 

scala> class MyClass extends BaseClass {
     |   val x = new B {
     |     val b = 0
     |   }
     | 
     |   x.b
     | }
<console>:18: error: value b is not a member of B
         x.b
           ^

It's probably introduced by #5141 or maybe even #5294 "Inferred types for fields" introduced in the 2.12 branch.

It's quite interesting that the IO() macro somehow gets through with this issue.

@jackkoenig
Copy link
Contributor

As mentioned here scala/bug#10047 and in "Inferred types for fields" on the 2.12.0 release notes (http://scala-lang.org/news/2.12.0/), this is intended behavior.

Type inference has changed so the type of a structurally typed val is inferred to be the parent type rather than the structural type.

Because IO(...) returns the structural type of its argument, the type of the val is inferred to be the structural type:

scala> trait A
defined trait A

scala> abstract class B {
     |   val a: A
     | }
defined class B

scala> class C extends B {
     |   def f[T <: A](a: T): a.type = a
     |   val a = f(new A {
     |     val foo = 3
     |   })
     | }
defined class C

scala> val c = new C
c: C = C@72b59b59

scala> c.a.foo
res0: Int = 3

This can be worked around with the scalac option -Xsource:2.11.

Assuming this workaround is general enough for large codebases like rocket chip, I think we should move forward with bumping to 2.12.3 and use the workaround for now. chisel3._ code should not be affected.

@ducky64
Copy link
Contributor

ducky64 commented Aug 17, 2017

That plan of action sounds reasonable. Interesting that wrapping it in an IO(...) call is a workaround, is that something Scala might also remove in the future?

@ducky64
Copy link
Contributor

ducky64 commented Sep 22, 2017

Is this still relevant? It appears there's really nothing we can do on our end, and the IO(...) seems to solve this problem for relevant use cases. Compatibility code (rocket-chip) is basically the only issue, for which there is the described scalac option workaround.

@jackkoenig
Copy link
Contributor

I think given that it's solved by IO(...) this issue is closed from our perspective.

To reiterate for posterity: Users of the compatibility wrapper should add the scalac option -Xsource:2.11

@ucbjrl
Copy link
Contributor Author

ucbjrl commented Sep 22, 2017 via email

@wachag
Copy link

wachag commented Nov 28, 2017

In the (very far...) future this problem will come again when Dotty is released.

@jackkoenig
Copy link
Contributor

This is a problem again (and has been for a while now). #754 changed the type definition of def IO:

// From
protected def IO[T<:Data](iodef: T): iodef.type
// To
protected def IO[T<:Data](iodef: T): T 

Returning iodef.type works around this issue, returning T does not.

@jackkoenig jackkoenig reopened this Aug 24, 2018
@jackkoenig jackkoenig added this to the 3.3.0 milestone Dec 18, 2018
@chick
Copy link
Contributor

chick commented Dec 18, 2018

Consider adding this as a 3.3 Issue

@azidar
Copy link
Contributor

azidar commented Oct 27, 2020

This will be fixed in 3.5.0 once we delete val io.

@azidar azidar modified the milestones: 3.3.0, 3.5.0 Oct 27, 2020
@jackkoenig
Copy link
Contributor

Fixed in 3.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
long-term Long-term unresolved issues
Projects
None yet
Development

No branches or pull requests

8 participants