ballerina-platform / ballerina-lang

The Ballerina Programming Language
https://ballerina.io/
Apache License 2.0
3.66k stars 750 forks source link

Typeof readonly map gives incorrect output #35092

Open rdulmina opened 2 years ago

rdulmina commented 2 years ago

Consider the below sample

    readonly & record {|int a;|} b = {a: 4};
    io:println(typeof b);

This prints typedesc {"a":4} but it should print typedesc readonly & record {|4 a;|}

HindujaB commented 2 years ago

As per the spec of typeof expression,

For containers, this is the same as the inherent type; when the container is immutable, it will be a singleton type.

Therefore, the typeof expression should return a singleton type that contains map value {"a":4} for the above case. IMHO, the string typedesc {"a":4} is expected because the string representation of a map value does not contain the type/immutability information.

HindujaB commented 2 years ago

As per the offline discussion, The {a: 4} in the above example is the string representation of a map value but not a type. We currently don't have a spec for the ToString() of a typedesc value. By implementation, we return the string representation of the value in the singleton.

As @rdulmina pointed out, readonly & record {|4 a;|} is a valid ballerina singleton type and providing the value as a string can be confusing to the user. And returning the string representation of the value can be okay with the singleton types that contain the simple basic values/list values. But the user may expect more information if the singleton type contains a map value ( field types, immutability, etc).

In that case, we might need to special-case the ToString() for structured singletons to check and include the field values and immutability information. But this approach can be complex because, at runtime, we don't narrow the types when we create new immutable values. (https://github.com/ballerina-platform/ballerina-lang/issues/13189)

If we are to change the string value of a singleton to return the value.getType().toString() instead of value.toString() still we will miss the narrowed type information for immutable values.

Eg. :

readonly & record {|int a;|} b = {a: 4};

the type of b will return record {| int a; |} & readonly not readonly & record {|4 a;|}

If we generate the string representation for the above case, iterating over fields of the map value can result in some complexities.

  1. If the record contains fields with complex types (like a record, union, table, etc.) and they are immutable, then we may need to recursively go through the fields to create the string.

  2. We need to decide upon the string representations if we have a pre-defined readonly record type with value.

    type Student readonly & record {|
        int a = 4;
    |};
    Student r = {};
    io:println(typeof r);

    Should it be readonly & record {|4 a;|} or Student? Do we need a recursive way to generate string values for singletons, or should we verify this with the spec first?

@warunalakshitha @MaryamZi What are your thoughts on this?

MaryamZi commented 2 years ago

I think the correct typedesc here is readonly & record {|4 a;|}.

The spec says

The type descriptor resulting from typeof will be the same as the type descriptor used to construct the value. For containers, this is the same as the inherent type; when the container is immutable, it will be a singleton type.

So even in the Student case it will be readonly & record {|4 a;|}.

For constants, at compile-time we now define the narrowest type (i.e., intersection of readonly and the singleton type), which is similar to readonly & record {|4 a;|}.

While we can fix just the string representation, I think we should look into narrowing the type to the singleton type (https://github.com/ballerina-platform/ballerina-lang/issues/13189) once and for all, which would automatically fix this.

rdulmina commented 2 years ago

https://github.com/ballerina-platform/ballerina-lang/issues/13189 right will not automatically fix this right?. Narrowed type information will be available after https://github.com/ballerina-platform/ballerina-lang/issues/13189 so that the fix will be easier.

HindujaB commented 2 years ago

We had some runtime conflicts when implementing the fix for https://github.com/ballerina-platform/ballerina-lang/issues/13189 because of the current singleton type implementation BFiniteType have some deviations from the spec. It was decided to wait until the semtype improvements were merged.

So, we can provide the toString() changes as a temporary solution.

warunalakshitha commented 2 years ago

Lets hold this issue until we properly represent singleton types with the sem type improvements.