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

Open
ucbjrl opened this Issue May 3, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@ucbjrl
Contributor

ucbjrl commented May 3, 2017

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.

@xnorme

This comment has been minimized.

xnorme 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

This comment has been minimized.

Contributor

jackkoenig commented Aug 17, 2017

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

jackkoenig commented Sep 22, 2017

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

@jackkoenig jackkoenig closed this Sep 22, 2017

@ucbjrl

This comment has been minimized.

Contributor

ucbjrl commented Sep 22, 2017

@wachag

This comment has been minimized.

wachag commented Nov 28, 2017

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

@jackkoenig

This comment has been minimized.

Contributor

jackkoenig commented Aug 24, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment