-
Notifications
You must be signed in to change notification settings - Fork 186
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
New Rule: ImplicitClassPrivateParams (fix #542) #553
Conversation
29505aa
to
de27239
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this contribution! I apologize the slow responses, last week has been busy. I'm glad to see you got the tests running on Windows. A few comments
import scalafix.{Patch, Rule} | ||
|
||
case object ImplicitClassPrivateParams | ||
extends Rule("ImplicitClassPrivateParams") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImplicitClassPrivateVal
|
||
override def fix(ctx: RuleCtx): Patch = { | ||
ctx.tree.collect { | ||
case Defn.Class(mods, _, _, primCtor, _) if hasImplicitMod(mods) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can refine the condition to only apply for AnyVals that are missing private
t.collect {
case Defn.Class(
cmods,
_,
_,
Ctor.Primary(_, _, (Term.Param(pmods, _, _, _) :: Nil) :: Nil),
Template(_, init"AnyVal" :: Nil, _, _)
) =>
for {
_ <- cmods.find(_.is[Mod.Implicit])
if !pmods.exists(_.is[Mod.Private])
valmod <- pmods.find(_.is[Mod.ValParam])
} yield ctx.addLeft(valmod, "private ")
}
For non-anyvals it's possible to remove the val
parameter, implicit class Foo(x: Int) { .. }
. Classes that extends AnyVal
are however forced to add the val
modifier which makes it easy to accidentally forget private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Can you explain to me how is()
is working here ? I noticed it but thought it only worked with tokens. Where are the instances for Classifiable
/Classifier
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instances are generated by the scalameta @ast
macro annotations. It's a flexible mechanism to guard against categories of trees/tokens both via .is[T]
or in pattern matching with case T()
. You can build custom groups of classes like we do in scalafix https://github.com/scalacenter/scalafix/blob/00199145261b958fb12cf256773c2ed08f336359/scalafix-core/shared/src/main/scala/scalafix/util/TokenClasses.scala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks. One last question : your implementation ignores cases where final
or protected
is present. Don't we care ?
def doubled: String = str + str | ||
} | ||
|
||
implicit class XtensionVar(var str: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can ignore var
cases since those are less commonly (if ever) used for implicit classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it best to keep these as non reg tests ? Unless you think this is being overzealous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negative tests are good :) I probably don't write enough of them. We don't need too many here however since the implementation guards against val
def doubled: Int = int + int | ||
} | ||
|
||
implicit class Xtension(int: Int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can ignore non-AnyVals since those fields are private by default unless they're case classes, but case classes are rarely used for implicit classes.
3d847bb
to
e4fe208
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch with final val
!
val optPatch = for { | ||
_ <- cMods.find(_.is[Mod.Implicit]) | ||
if !pMods.exists(_.is[Mod.Private]) | ||
valMod <- pMods.find(_.is[Mod.ValParam]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct that this does not handle final val param: T
. We can use .find(!_.is[Mod.Annot])
instead, but adding private can still break cases like override val param: T
. We should probably exclude cases that contain override
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think override
isn't an issue since we're only looking for implicit value classes and AFAIK they can't extend both AnyVal
and a base class. I pushed another commit WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AnyVals can inherit from "universal traits"
@ trait Foo extends Any {
def bar: Int
def plusOne = bar + 1
}
defined trait Foo
@ implicit class UInt(override val bar: Int) extends AnyVal with Foo
defined class UInt
@ 2.plusOne
res4: Int = 3
See http://docs.scala-lang.org/overviews/core/value-classes.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olafurpg Indeed. This case won't be taken into account though, because in your implem you explicitly target a sole Init (AnyVal here).
2a5e005
to
bec3eb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments, otherwise I think this rule is good to go 🔥
}.asPatch | ||
} | ||
|
||
private def containsPrivateOrProtected(pMods: List[Mod]) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick, for simple implementations like here I think it's clearer to inline, by adding a method you need to look at it's implementation to know what it does.
|
||
# ImplicitClassPrivateVal | ||
|
||
`val` fields of implicit classes are accessible as an extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-private val
fields of implicit classes leak as publicly accessible extension methods.
"message".str // does not compile | ||
``` | ||
|
||
This rule safely ignores all other modifiers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note. This rule only triggers for val
fields, it ignores other modifiers such as var
.
_, | ||
_, | ||
Ctor.Primary(_, _, (Term.Param(pMods, _, _, _) :: Nil) :: Nil), | ||
Template(_, init"AnyVal" :: Nil, _, _)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can lift the restriction that it cannot mix in universal classes, if you are feeling more ambitious 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to wait and see if people actually need that because as you said, the error is more likely to happen with implicit value classes. Speaking of, should I rename the rule to ImplicitValueClassPrivateVal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to wait and see if people actually need that because as you said, the error is more likely to happen with implicit value classes.
I agree.
Speaking of, should I rename the rule to ImplicitValueClassPrivateVal ?
Good question. How about LeakingImplicitClassVal
? That gives us room to later support non-AnyVal in case there's demand. I think it's good to hint in the name what the problem is and the Leaking*
prefix is consistent with other upcoming rules like LeakingSealed
bec3eb8
to
dab04bc
Compare
@olafurpg Fixed it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thank you @LeonardMeyer for addressing all of the comments! 💃
No description provided.