getter for oneof field not returning value of field; returns name of field

[jackpunt]

Sahin, sorry to be late getting to this, I've been off for the summer.
(I commented on the closed #53 (or #39 ?) but am unsure how to re-open at this point)

It seems that the oneof getter (or computeOneofCase ?) needs a bit more work; consider:

message TypedMsg {
  oneof value {
    bool boolValue = 3;
    int32 intValue = 4;
    string strValue = 5;
  }
}

I believe it should be the case that:

let msgInt = new TypedMsg({intValue: 23})
let msgStr = new TypedMsg({strValue: "foo"})
assert( msgInt.intValue === 23)      // works
assert( msgStr.strValue === "foo")   // works
assert( msgInt.value === 23)         // fails: msgInt.value === "intValue"
assert( msgStr.value === "foo")      // fails: msgStr.value === "strValue"

That is: the type of TypedValue.value should be: number | string | boolean
rather than the slot name: "intValue" | "strValue" | "boolValue" | "none"

Conceptually an extra dereference [but maybe not this ugly]:

get value() { 
     ...
     return this[cases[pb_1.Message.computeOneofCase(this, [3, 4, 5])]] ; 
} 

(happy to help, if you point me to the relevant code)

After further review (28-Oct; see #96 (comment) below, about "none"),
I suspect the generated code could be something like:

    get value(): boolean | string | number {
        const cases: {
            [index: number]: undefined | boolean | number | string;
        } = {
            0: undefined,
            3: this["boolValue"],
            4: this["intValue"],
            5: this["strValue"]
        };
        return cases[pb_1.Message.computeOneofCase(this, [3, 4, 5])]; // Note: 0 -> undefined
    }

[jackpunt]

While we're in there, this (especially "none" to mark field 0) may be problematic:

message usage {
  oneof quant {
    bool all = 1;
    bool some = 2;
    bool none = 3;
    bool undefined = 4;
  }
}

This contrived message produces this code:

    get quant() {
        const cases: {
            [index: number]: "none" | "all" | "some" | "none" | "undefined";
        } = {
            0: "none",
            1: "all",
            2: "some",
            3: "none",
            4: "undefined"
        };
        return cases[pb_1.Message.computeOneofCase(this, [1, 2, 3, 4])];
    }

Probably want to reengineer without a string name, or use a name that is not legal as a message field name.
[happily, when fixed, the name should not be visible ]

[jackpunt]

Ah... after further review, I think I see what's happened.
Somehow, the getter and the 'which_<name>' got conflated.
(see #39 (comment))
The current 'getter' is coded to be the 'which_<name>'

Per original #96 (comment) above, the getter should return the value of the supplied oneof field.
The which_<name> accessor should return: "name1" | "name2" | ... | undefined
Per second #96 (comment) above, I think it is unwise to return an arbitrary name such as "none"
(and it makes some sense to say that: the field that was set/supplied in the oneof is undefined)

[thesayyn]

Ah... after further review, I think I see what's happened. Somehow, the getter and the 'which_' got conflated. (see #39 (comment)) The current 'getter' is coded to be the 'which_'

Per original #96 (comment) above, the getter should return the value of the supplied oneof field. The which_ accessor should return: "name1" | "name2" | ... | undefined Per second #96 (comment) above, I think it is unwise to return an arbitrary name such as "none" (and it makes some sense to say that: the field that was set/supplied in the oneof is undefined)

Hmm, all of this makes sense. Would like to send a PR for this?

[jackpunt]

I'm not familiar with your code-base; but if you point me to a likely file, I'll do the code/test/PR ...

... Ok, i looked into descripter.js & createOneofGetter()...
I see that ts.factory defines a "language" for generating code, but I don't know that language;
So it would take me a while to decode all that;
(and I don't expect I have the tool chain to build/test, not a Bazel user, etc)

So: if there's a developer's guide (explaining the proto factory code-generator and conventions) the maybe I could be useful... but it would be quickly/easily

[thesayyn]

Hey @jackpunt. sorry, I missed this. Actually, you don’t need to worry about bazel. just invoke yarn test or npm run test it will run the tests for you.

The right place for this is to create an AST generator function named createOneOfWhichGetter and copy the contents of the createOneofGetter func and change it in a way that would generate the AST mentioned above.

There is a PR #94 to migrate the project to TypeScript which I'd like to see land so that people can contribute easily.

[thesayyn]

I just had time to take a look at this. and I realized that protoc-gen-ts does the right thing here. builtin js does not generate a union getter that gives you the value. instead, it just gives you a discriminator getter and that's it.

[jackpunt]

Ok. I see what you're saying;
It is mainly the name that is conflated.

The other languages distinguish between msg.getValue() and msg.getValueType:
[java] msg.getValueCase()
[javascript] msg.getValueCase()
[dart] msg.whichValue()
[python] message.WhichOneof("value")
[golang] msg.Value.(type)

The method returns the string discriminator [rather than the actual one-of value() : union of types]
So: change the name from msg.value() to msg.whichValue() or msg.valueCase()

Since you have done the awesomeness of defining get methods for the fields
the motivated user can then create:

get value() :  [union type] {
  return this[this.whichValue]
}

although: since the protoc-gen-ts code has all the type info,
it would be nice if it could provide that method

The other issue is still there:
Make safe code by changing: "none" to undefined

Also: with tsc 4.3+, my tsconfig.json says: noImplicitOverride: true
So it would be excellent if deserializeBinary was decorated as: static override deserializeBinary(...)

[jackpunt]

Breath some life into this...
A) it would be better if the [current] accessor for the 'field discriminator' was not using the field's 'name'.
(so that one could use .name to access the actual field value)
Other languages compose the discriminator function name with: 'Case' or 'which'

B) It is really important to change the implementation of the discriminator to not use "none" as a string value.
It works just as well using 'undefined' and avoids corruption when there is an actual value identified by "none"
(per example above; and really: just c/"none"/undefined/g in the two places it occurs in the generated code)