Invalid use of this.type for clone methods

[jix]

I noticed that Data.cloneType has a return type of this.type (Data.scala:64) which in scala isn't just the class of the receiver object, but a special type consisting of only the receiver object. It does not make much sense for a clone method to return the exact same instance it was invoked on, so at least it is misleading. See also this discussion about using this.type for clone.

It also requires explicit type casts for implementers (e.g. in Decoupled.scala:15). Given scala's type system those casts would be invalid. They only work because at runtime the weaker JVM's type system is used to check casts.

AFAICT, the scala compiler would also be free to ignore the actual return value, and just use the receiver object instead. I don't think this is likely to happen though.

The intended type signature is probably def cloneType: this.getClass, except that this is not valid scala. The usual work-around involves using a trait (trait CloneableType[T]) which is implemented by derived classes (class MyBusBundle(busWidth: Int) extends CloneableType[MyBusBundle]).

I assume this is not what you'd want in Chisel, given the excessive verboseness when implementing parametrized bundles. It probably also wouldn't work with the reflection code that gives a generic cloneType implementation for non-parametrized bundles.

I have some vague ideas on a different approach, but wanted to ask whether a fix for this would be welcome, before spending time on it. The idea is to change uses of foo.cloneType to Data.cloneType(foo) or just cloneType(foo) with the right import, and making foo.cloneType return just Data.

The implementation would be along the lines of this:

abstract class Data {
    ...
    def cloneType: Data
    ...
}
object Data {
    def cloneType[T <: Data](data: T): T = data.cloneType.asInstanceOf[T]
}

It would still be possible to write implementations of cloneType that return a Data object of the wrong type, causing an error at runtime. But this is the same as before with the explicit and incorrect cast to this.type needed for implementations.

There are some other invalid uses of this.type for clone like methods. The same would apply to them.

PS: Is there some chisel development mailing-list or chat? I've only found the chisel-users group, which did not have any development discussion.

[sdtwigg]

Instead of the trait using F-bounded types, I would strongly prefer the typeclass approach. I don't think designs usually/should rely on runtime polymorphism for the cloneType methods so that should be OK.

Separately, the workaround with object Data is reasonable although people are likely to complain about the extra verbosity.

[ducky64]

This is something we should implement, but we need to determine how to make this work in compatibility mode.

[ducky64]

It might be a good idea to have cloneType return type Data, so that when an implementation of cloneType is required, it doesn't require the boilerplate (and probably not-strictly-correct) .asInstanceOf[this.type].

This would be a API-breaking change, and existing uses of cloneType might break (but there really aren't good reasons to call that anymore). @jackkoenig are there direct uses of cloneType in rocket-chip code that might break?

Though since cloneType is no longer really a public API, this change can happen in the future.

[ducky64]

One place where cloneType continues to be used is in the implementation of Record subtypes. This needs further discussion, and if we can have an automatic way to clone Records.

[chick]

Can't fix. Cannot change type in cloneType

[ducky64]

Longer rationale for the record: cloneType needs to return the same type, and there's code that depends on this static type - so we shouldn't break that. The clean alternative is F-bounded types, where cloneType is type-parameterized on the enclosing object and returns that type parameter, though this would (probably?) require F-boudned types as part of the Data hierarchy, which would break even more code.

In any case, we're no longer recommending the use of cloneType, and clone-like operations like chiselTypeOf and friends follow the type parameterized pattern on the argument instead.