Enable LeakingImplicitClassVal lint rule

[richardxia]

Related issue:

Type of change: other enhancement

Impact: no functional change

Development Phase: implementation

Release Notes

---

Followup to #2648.

This PR enables the LeakingImplicitClassVal lint rule in Scalafix. In Scala, val constructor parameters are part of the public interface of a class. However, in most cases, implicit classes do not intend to publicly expose the val parameter, as the parameter is generally only there to assign an internal name to the object being implicitly converted to another type. This lint rule will require that all val parameters to implicit classes be explicitly marked as private.

To the Scala experts: please take a look at the changes to the code required by this new lint rule and let me know if this is a reasonable rule to enable in rocket-chip.

[aswaterman]

For the classes I wrote, this is clearly an improvement.

I suspect this is addressing much less than 1% of the problem of too many public vals in Chisel modules. Is this the first of many steps in a broader effort?

[richardxia]

For the classes I wrote, this is clearly an improvement.

I suspect this is addressing much less than 1% of the problem of too many public vals in Chisel modules. Is this the first of many steps in a broader effort?

Not exactly, haha. My current effort is mostly about enabling as many of the builtin rules to Scalafix as possible. I think most of those rules are uncontroversially good rules to enable.

Public vals in Chisel modules is going to more difficult to handle because Scala defaults to public visibility and because Chisel's DSL involves writing the implementation of the hardware in the constructor of the Scala class. All named variables in the constructor are actually instance variables of the class rather than local variables of a method. Even if we wanted to encourage declaring them with private/protected, I don't think a lint rule or mechanical enforcement will work here, since we clearly will want to have public members of Chisel modules.

[aswaterman]

Yeah, it’s hard to see how to employ lint rules to address that problem.

(I’ve long wished Scala’s default visibility wasn’t public.)

[jackkoenig]

For anyone who isn't aware why this is a good lint rule, not doing this effectively adds the val in the implicit class as a function defined on the class being implicitly converted:

implicit class PartyString(val x: String) extends AnyVal {
  def celebrate(): Unit = println(s"Woohoo from '$x'")
}

val myString = "Hello World!"
myString.celebrate() // Woohoo from 'Hello World!'

myString.x // <------- huh?

Historical note: it used to be required for the val to be public for the Value Class pattern. It has been acceptable to make them private ever since Scala 2.11.0 but it seems almost no examples have been updated with that in mind.

[hcook]

Side note but I also have no idea why we are extending AnyVal for all of these. Are we actually deriving some kind of real performance benefit from this or are we just cargo-culting some ancient example?