ExtendedCfgNodeMethods: rename type parameter #4936
Conversation
There was a problem hiding this comment.
I absolutely think we should merge this and also enable the type param shadowing warning by default (will create a separate PR shortly), for the sake of readability and disambiguity.
That being said, I'm not sure I understand which specific problem this PR solves, maybe you can elaborate..?
From my POV, the two type parameters in your Example class are not related at all. They happen to have the same name, which only means that within def foo we cannot reference the NodeType type from the class Example, but apart from that there's no connection at all between them.
Semantically it's similar to something like this on the value level: the two bar variables are not related at all, they just happen to have the same name, and therefor we cannot reference the class-level bar variable from within def baz.
class Foo {
val bar = 42
def baz: Unit = {
val bar = "123"
()
}
}
as brought up in #4936 it can lead to confusing situations if type parameters are shadowed, so for the sake of readability and disambiguity alone we should enable this compiler warning IMO. That being said, I'd like to stress that it's not something fundamentally complicated, afaics. Simple example copied from #4936: ```scala class Example[NodeType <: Object](node: NodeType) extends AnyVal{ def foo[NodeType](x: List[NodeType]): List[NodeType] = x } ``` These two type parameters in your Example class are not related at all. They happen to have the same name, which only means that within `def foo` we cannot reference the `NodeType` type from the `class Example`, but apart from that there's no connection at all between them. Semantically it's similar to something like this on the value level: the two `bar` variables are not related at all, they just happen to have the same name, and therefor we cannot reference the class-level `bar` variable from within `def baz`. ```scala class Foo { val bar = 42 def baz: Unit = { val bar = "123" () } } ```
as brought up in #4936 it can lead to confusing situations if type parameters are shadowed, so for the sake of readability and disambiguity alone we should enable this compiler warning IMO. That being said, I'd like to stress that it's not something fundamentally complicated, afaics. Simple example copied from #4936: ```scala class Example[NodeType <: Object](node: NodeType) extends AnyVal{ def foo[NodeType](x: List[NodeType]): List[NodeType] = x } ``` These two type parameters in your Example class are not related at all. They happen to have the same name, which only means that within `def foo` we cannot reference the `NodeType` type from the `class Example`, but apart from that there's no connection at all between them. Semantically it's similar to something like this on the value level: the two `bar` variables are not related at all, they just happen to have the same name, and therefor we cannot reference the class-level `bar` variable from within `def baz`. ```scala class Foo { val bar = 42 def baz: Unit = { val bar = "123" () } } ```
|
Indeed, there's no ambiguity at Scala level: it's just another form of good ol' lexical scoping. It's only when it gets down to byte code that we find ourselves in a pickle, as we get 2 same-named type parameters in the same method, causing havoc with (I think) kotlin-reflect. Briefly, and in the context of #4796, @tajobe has been working on a plugin to automatically generate ergonomic Kotlin bindings for the most relevant APIs. It starts by finding which methods to codegen for (based on their signature), and proceeds emitting a more ergonomic (Kotlin-wise) method definition for it. So for so good... until a wild method signature with 2 same-named type parameters appeared and kotlin-reflect bailed out. Changing the type parameter name was the trivial workaround. No one was expecting to find a method with 2 same-named type parameters. I couldn't find anything about this edge case in various JVM-related docs online: not sure if it's undefined behaviour or I just missed something. At any rate, if someone knows more about this, I'd love to hear! |
|
Thanks for elaborating, yeah that all makes sense. |
@tajobe has been doing some byte code surgery in order to create a Kotlin interoperability layer and found an interesting edge case when type parameters are shadowed.
Taking a simpler example, consider the following Scala code:
When decompiling the
foomethod, we'll find the following:Notice how
NodeType(the type parameter) occurs twice in the definition offoo$extension. Apparently the JVM (assuming Oracle as canonical) is designed to accept this when loading/linking, but guarantees nothing if the byte code is interpreted through reflection/etc, cf. https://docs.oracle.com/javase/specs/jvms/se23/html/jvms-4.html#jvms-4.7.9:This byte code poses a problem that is trivially fixed if we prevent shadowing by just renaming one of the type parameters, e.g.
resulting in
Compiling Joern with
-Wshadow:type-parameter-shadowonly warns about 2 sites: the one in this PR and another one buried in javasrc2cpg. The latter is not public-facing, so I won't even bother proposing to change it at this point.This rename is purely syntactical and seems to easily solve a bigger hurdle. Wdyt?