Skip to content
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

"warning: already initialized constant Companion" with Kotlin companion objects #6196

Closed
kalenp opened this issue Apr 30, 2020 · 8 comments · Fixed by #6289
Closed

"warning: already initialized constant Companion" with Kotlin companion objects #6196

kalenp opened this issue Apr 30, 2020 · 8 comments · Fixed by #6289

Comments

@kalenp
Copy link

kalenp commented Apr 30, 2020

When using Kotlin companion objects from JRuby, we get an "already initialized constant Companion" warning simply by loading the class. The companion object works correctly and is accessible as a constant from the parent class.

We're seeing this on JRuby 9.2.11.1. I tested prior versions and this warning first appears in JRuby 9.2.9.0.

Detailed Description

Given a Kotlin class like:

package kotlintest

class Test {
	companion object {
		const val FOO = "foo"
	}
}

And a ruby class like:

require 'java'
require './build/libs/test-all.jar'

p Java::Kotlintest::Test::FOO

We get the following output:

$ ruby test.rb
test.rb:4: warning: already initialized constant Companion
"foo"

The warning is triggered just by loading the class. So if you just reference Java::Kotlintest::Test, without the companion object, you still get the warning.

Gradlefile used to build the jar:

plugins {
    id("org.jetbrains.kotlin.jvm") version "1.3.70"
    id("com.github.johnrengelman.shadow") version "5.2.0"
}

repositories {
    jcenter()
}

dependencies {
    implementation("org.jetbrains.kotlin:kotlin-stdlib-jdk8")
}
@headius
Copy link
Member

headius commented May 19, 2020

I'm guessing this behaves similarly to Scala's singleton objects?

In the case of Scala, we use our knowledge of how the classes and bytecodes get stitched together to properly handle singletons. I think we'd need to do the same thing here, but I am unfamiliar with how Kotlin compiles this. Will have to dump bytecode to see.

@headius
Copy link
Member

headius commented May 19, 2020

I believe I see the problem here.

When binding elements from a JVM class, there are two things we put into constants:

  • Inner classes
  • Static final "constant" values

The warning here happens because in the case of a Kotlin companion object, there's both of these on the given class:

$ javap -cp . kotlintest.Test
Compiled from "kotlin_text.kt"
public final class kotlintest.Test {
  public static final java.lang.String FOO;
  public static final kotlintest.Test$Companion Companion;
  public kotlintest.Test();
  static {};
}

So in some order, we try to bind both the inner class and the constant field to the name Companion in Test and standard Ruby behavior is to warn when redefining a constant.

This is somewhat problematic since there's no indication which one we should use, looking at just the class structure. Practically speaking, I think the fix here would be to hardcode any inner classes named "Companion" on a Kotlin parent class and avoid binding them.

headius added a commit to headius/jruby that referenced this issue May 19, 2020
Kotlin's companions are implemented similarly to Scala singletons,
except that both a Companion inner class and static final field
are generated. As a result, we warn when trying to redefine that
Companion constant. This is the cause of the warning in jruby#6196.

The fix here avoids binding the inner class, since it's likely not
intended to be visible to non-Kotlin consumers, and because the
actual instance of it seems like the more appropriate value to
bind.

Fixes jruby#6196
@headius
Copy link
Member

headius commented May 19, 2020

I've pushed #6232 which fixes this warning but requires some discussion.

[] ~/projects/jruby/tmp $ jruby -J-cp kotlintest.jar -e 'p Java::kotlintest::Test::FOO'
"foo"

[] ~/projects/jruby/tmp $ jruby -J-cp kotlintest.jar -e 'p Java::kotlintest::Test::Companion'
#<Java::Kotlintest::Test::Companion:0x75527e36>

@headius headius added this to the JRuby 9.3.0.0 milestone May 19, 2020
@kalenp
Copy link
Author

kalenp commented May 19, 2020

Thanks for looking into this.

Comparing documentation, Kotlin's companion objects do indeed look similar to Scala's singleton objects. For companions, the singleton object is associated with a class, typically functioning as a replacement for static values and methods. (https://kotlinlang.org/docs/reference/object-declarations.html#companion-objects)

To add an extra wrinkle to this, you can specify an optional name for the companion object. Companion is just the default naming.

I just included a constant here, but given the questions in #6232, I experimented with defining a function as well. In this case, the function shows up on the companion object instance. This can be modified by adding the @JvmStatic annotation (https://kotlinlang.org/docs/reference/java-to-kotlin-interop.html#static-methods), in which case the method appear both on the parent class as well as the companion object instance. This is probably the best practice for JRuby + Kotlin interop.

I think that exposing the companion object instance and not the class is probably the best option from the client's perspective. With the @JvmStatic annotation, you don't even need to expose the companion object instance (everything is accessible via the parent class), but somebody may want to use an explicitly named companion, like Foo::Factory.create.

Hopefully this is helpful context.

headius added a commit to headius/jruby that referenced this issue Jun 9, 2020
Kotlin's companions are implemented similarly to Scala singletons,
except that both a Companion inner class and static final field
are generated. As a result, we warn when trying to redefine that
Companion constant. This is the cause of the warning in jruby#6196.

The fix here avoids binding the inner class, since it's likely not
intended to be visible to non-Kotlin consumers, and because the
actual instance of it seems like the more appropriate value to
bind.

Fixes jruby#6196
@headius
Copy link
Member

headius commented Jun 11, 2020

@kalenp Ah so "Companion" is just a default name. That makes my patch a bit problematic.

If I'm reading you right, using @JvmStatic makes this work properly without any patching? If so I think that would be the best recommendation. I am not sure if there's a reliable way to detect the non-JvmStatic companion object.

I guess the other option is to prioritize either the inner class or the static field and avoid the warning that way, but I feel like we'll be wrong as often as we're right.

@kalenp
Copy link
Author

kalenp commented Jun 11, 2020

@headius Thanks for taking another look at this.

Here's an expansion on the original information provided above to include concrete examples of the companion object methods with and without the @JvmStatic annotation.

$ cat src/main/kotlin/kotlintest/Test.kt
package kotlintest

class Test {
	companion object NamedCompanion {
		const val COMPANION_CONSTANT = "cc"

		fun companionMethod(): String {
			return COMPANION_CONSTANT
		}

		@JvmStatic
		fun annotatedCompanionMethod(): String {
			return COMPANION_CONSTANT
		}
	}
}
$ javap -cp build/libs/jruby-kotlin-interop-all.jar kotlintest.Test
Compiled from "Test.kt"
public final class kotlintest.Test {
  public static final java.lang.String COMPANION_CONSTANT;
  public static final kotlintest.Test$NamedCompanion NamedCompanion;
  public kotlintest.Test();
  static {};
  public static final java.lang.String annotatedCompanionMethod();
}
$ javap -cp build/libs/jruby-kotlin-interop-all.jar 'kotlintest.Test$NamedCompanion'
Compiled from "Test.kt"
public final class kotlintest.Test$NamedCompanion {
  public final java.lang.String companionMethod();
  public final java.lang.String annotatedCompanionMethod();
  public kotlintest.Test$NamedCompanion(kotlin.jvm.internal.DefaultConstructorMarker);
}

So when using the @JvmStatic annotation on the methods, everything is accessible from the parent class like a typical Java class with static methods. This seems to be the recommended pattern for Java interop, and makes sense for JRuby interop as well. With the @JvmStatic annotation, you can access the constants and methods from the parent class from JRuby just fine today. But it still emits the constant redefinition warnings, since it's still attempting to bind both the inner class and the companion instance with the same name.

Maybe the simplest check is to look for a public static final member being bound to an inner class with the same name as the member (plus whatever is-this-kotlin checks you had previously). That might be reliable enough to filter on. I think if we had to pick either the inner class or the instance to bind to the constant, the instance would be potentially more useful. But with @JvmStatic, neither really needs to be exposed to JRuby, so in the end it might not matter which one is chosen.

@headius headius modified the milestones: JRuby 9.3.0.0, JRuby 9.2.12.0 Jun 16, 2020
@headius
Copy link
Member

headius commented Jun 16, 2020

Try to fix for 9.2.12.

headius added a commit to headius/jruby that referenced this issue Jun 18, 2020
This commit makes two changes to fix jruby#6196:

* When both an inner class and a static final field have the same
  name, only define the field. Typically this is used for a
  singleton pattern where the field points at a singleton instance
  of the inner class, as with Kotlin companion classes. Since we
  can't define both, we prefer the field.
* Do not define the inner class constant as a side effect of
  initializing that class's proxy, like we do for package
  constants for normal classes. Defer that constant definition to
  the initialization of the declaring class, which will make the
  decision whether to define (or not) the constant at that point.

If both the field and the class are needed, it's still possible to
import the inner class under a specific name using `java_import`:

```ruby
java_import("OuterClass$InnerClass") { "MyWeirdInnerClass" }
MyWeirdInnerClass.whatever()
```
@headius headius linked a pull request Jun 18, 2020 that will close this issue
@headius
Copy link
Member

headius commented Jun 19, 2020

This has been "fixed", in that it no longer warns. However we may want to add magic to make the companion methods appear on the containing class, which I think is how they would appear in Kotlin.

@headius headius closed this as completed Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants