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

Compile error when proto contains a oneof field with same name as the enclosing message #47

Closed
garyp opened this issue May 25, 2020 · 5 comments · Fixed by #104
Closed
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@garyp
Copy link
Collaborator

garyp commented May 25, 2020

From #18 (comment):

A proto definition such as:

message Value {
  oneof value {
    string string_value = 1;
    bool boolean_value = 2;
    int32 integer_value = 3;
    int64 long_value = 4;
    float float_value = 5;
    double double_value = 6;
    int64 date_value = 7;
  }
}

Generates code like:

data class Value(
    val value: Value<*>? = null
) {
    sealed class Value<V>(value: V) : pbandk.Message.OneOf<V>(value) {
        class StringValue(stringValue: String = "") : Value<String>(stringValue)
        ...
    }
    ...
}

Which fails to compile because there are two types named Value and the type of value in val value: Value<*>? = null can't be resolved correctly.

@garyp garyp added the bug Something isn't working label May 25, 2020
@sujithkris
Copy link

@garyp - Any update ? :)

@garyp garyp added the good first issue Good for newcomers label Jul 15, 2020
@garyp
Copy link
Collaborator Author

garyp commented Jul 15, 2020

@sujithkris Haven't had a chance to fix this yet. Should be pretty straightforward, if you happen to be interested in looking into it. I'm guessing the fix would be to change the code in pbandk's CodeGenerator.writeConstructorField() method to use the type's fully-qualified name rather than the short name.

@garyp garyp added this to the 0.10.0 milestone Sep 11, 2020
@nbaztec
Copy link
Contributor

nbaztec commented Nov 19, 2020

Hi, went overt he code real quick but could you perhaps point out to a test that could be used to validate the fix?
CodeGenerator takes in a file context, but I couldn't find any single point to test with a sample proto file.

@jan-xyz
Copy link

jan-xyz commented Nov 20, 2020

I would suggest to generate something like

syntax = "proto3";
package example;

message Person {
  oneof person {
    Foo foo = 1;
  }
}

message Foo {
}

into

package tutorial

data class Person(
    val person: Person<*>? = null,
    override val unknownFields: Map<Int, pbandk.UnknownField> = emptyMap()
) : pbandk.Message {
    sealed class Person<V>(value: V) : pbandk.Message.OneOf<V>(value) {
        class Foo(foo: tutorial.Foo) : Person<tutorial.Foo>(foo)
    }

    val foo: tutorial.Foo?
        get() = (person as? Person.Foo)?.value

    override operator fun plus(other: pbandk.Message?) = protoMergeImpl(other)
    override val descriptor get() = Companion.descriptor
    override val protoSize by lazy { super.protoSize }
    companion object : pbandk.Message.Companion<tutorial.Person> {
        val defaultInstance by lazy { Person() }
        override fun decodeWith(u: pbandk.MessageDecoder) = decodeWithImpl(u)

        override val descriptor: pbandk.MessageDescriptor<tutorial.Person> by lazy {
            pbandk.MessageDescriptor(
                messageClass = tutorial.Person::class,
                messageCompanion = this,
                fields = listOf(
                    pbandk.FieldDescriptor(
                        messageDescriptor = this::descriptor,
                        name = "foo",
                        number = 99,
                        type = pbandk.FieldDescriptor.Type.Message(messageCompanion = tutorial.Foo.Companion),
                        oneofMember = true,
                        jsonName = "foo",
                        value = tutorial.Person::foo
                    )
                )
            )
        }
    }
}

this is the diff:

20c20
<     companion object : pbandk.Message.Companion<tutorial.Person> {
---
>     companion object : pbandk.Message.Companion<Person> {
22c22
<         override fun decodeWith(u: pbandk.MessageDecoder) = decodeWithImpl(u)
---
>         override fun decodeWith(u: pbandk.MessageDecoder) = Person.decodeWithImpl(u)
24c24
<         override val descriptor: pbandk.MessageDescriptor<tutorial.Person> by lazy {
---
>         override val descriptor: pbandk.MessageDescriptor<Person> by lazy {
26c26
<                     messageClass = tutorial.Person::class,
---
>                     messageClass = Person::class,
36c36
<                                     value = tutorial.Person::foo
---
>                                     value = Person::foo

garyp pushed a commit that referenced this issue Dec 28, 2020
Fixes #47 

For the following protobuf where the `oneof` field had the same name as message, the companion object definition would shadow the `oneof` class instead of the `message` class. This patch fixes it by fully qualifying the `message` class name.

**Input:**
```protobuf
syntax = "proto3";
package foobar;

message Value {
    oneof value {
        int32 int_val = 1;
        string str_val = 2;
    }
}
```

**Before:**

```kotlin
package foobar

data class Value(
        val value: Value<*>? = null,
        override val unknownFields: Map<Int, pbandk.UnknownField> = emptyMap()
) : pbandk.Message {
       sealed class Value<V>(value: V) : pbandk.Message.OneOf<V>(value) {
              ...
       }
       ...
       companion object : pbandk.Message.Companion<Value> {
              ...
       }
       ...
}
```

**After:**

```kotlin
package foobar

data class Value(
        val value: Value<*>? = null,
        override val unknownFields: Map<Int, pbandk.UnknownField> = emptyMap()
) : pbandk.Message {
       sealed class Value<V>(value: V) : pbandk.Message.OneOf<V>(value) {
              ...
       }
       ...
       companion object : pbandk.Message.Companion<foobar.Value> {
              ...
       }
       ...
}
```
garyp pushed a commit that referenced this issue Dec 28, 2020
Fixes #47

For the following protobuf where the `oneof` field had the same name as message, the companion object definition would shadow the `oneof` class instead of the `message` class. This patch fixes it by fully qualifying the `message` class name.

**Input:**

```protobuf
syntax = "proto3";
package foobar;

message Value {
    oneof value {
        int32 int_val = 1;
        string str_val = 2;
    }
}
```

**Before:**

```kotlin
package foobar

data class Value(
        val value: Value<*>? = null,
        override val unknownFields: Map<Int, pbandk.UnknownField> = emptyMap()
) : pbandk.Message {
       sealed class Value<V>(value: V) : pbandk.Message.OneOf<V>(value) {
              ...
       }
       ...
       companion object : pbandk.Message.Companion<Value> {
              ...
       }
       ...
}
```

**After:**

```kotlin
package foobar

data class Value(
        val value: Value<*>? = null,
        override val unknownFields: Map<Int, pbandk.UnknownField> = emptyMap()
) : pbandk.Message {
       sealed class Value<V>(value: V) : pbandk.Message.OneOf<V>(value) {
              ...
       }
       ...
       companion object : pbandk.Message.Companion<foobar.Value> {
              ...
       }
       ...
}
```
@garyp
Copy link
Collaborator Author

garyp commented Dec 28, 2020

Fixed in #104 for the upcoming 0.9.1 release. I've also cherry-picked the fix into the master branch in #115.

@garyp garyp closed this as completed Dec 28, 2020
@garyp garyp modified the milestones: 0.10.0, 0.9.1 Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants