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

Clarification regarding Query Expression construct types #1252

Closed
pcnfernando opened this issue May 31, 2023 · 9 comments
Closed

Clarification regarding Query Expression construct types #1252

pcnfernando opened this issue May 31, 2023 · 9 comments
Assignees
Labels
Area/Lang Relates to the Ballerina language specification status/pending Design is agreed and waiting to be added Type/Improvement Enhancement to language design
Milestone

Comments

@pcnfernando
Copy link
Member

Description:

Regarding the spec description related to query expression:
Ref: https://ballerina.io/spec/lang/master/#section_6.35

A query-expr that constructs a stream must start with the stream keyword and is evaluated differently from a query-expr that constructs a value of other basic types...

A query-expr that constructs a mapping must start with the map keyword. An emitted value [k, v] results in adding a field with key k and value v.

If the query-expr starts with table, then the query-expr will construct a table; the key-specifier specifies the key sequence of the constructed table in the same way as with a table-constructor-expr. Otherwise, the applicable contextually expected type determines the basic type of the value constructed. If there is no contextually expected type, then the basic type of the value constructed is the basic type of expression following in; it is a compile-time error if the static type of this expression is not a subtype of one of the basic types that a query expression can construct.

Let me phrase my confusion as follows:

Can a map value be created without map keyword?

function foo(map<string> mp) {
  var _ = from var _ in mp
             select ["a", "b"]; 
}

The spec specifies below points seperately
A query-expr that constructs a mapping **must start with the map keyword**.

Confusion whether the below statement is applicable for maps since the following comes just under the table explanation
Otherwise, the applicable contextually expected type determines the basic type of the value constructed. If there is no contextually expected type, then the basic type of the value constructed is the basic type of expression following in; it is a compile-time error if the static type of this expression is not a subtype of one of the basic types that a query expression can construct.

Atm, the implementation supports constructing any basic type out of list, map, table, string, xml, stream when there's no contextually expected type looking at the basic type of expression following in.

Suggested Labels:

Code sample that shows issue:

Related Issues:

@jclark
Copy link
Collaborator

jclark commented Jun 6, 2023

We need to change

it is a compile-time error if the static type of this expression is not a subtype of one of the basic types that a query expression can construct

to explicitly disallow mapping type, so that it is consistent with

A query-expr that constructs a mapping must start with the map keyword

The reason why map is special here is that what you get when you iterate over a map is the values, but what you need to construct is [key, value] tuples.

@jclark jclark added Area/Lang Relates to the Ballerina language specification spec/unclear Spec does not clearly explain the design labels Jun 6, 2023
@jclark jclark added this to the 2023R1 milestone Jun 14, 2023
jclark added a commit that referenced this issue Jun 14, 2023
Major rework of query expressions.
Also affects spreads.
Fixes #1134,  #1137, #1144, #1252.
@jclark
Copy link
Collaborator

jclark commented Jun 14, 2023

Fixed by 5e76414

@jclark jclark closed this as completed Jun 14, 2023
@pcnfernando
Copy link
Member Author

Can I know the reason why we should restrict stream typed values being created without stream query-construct-type?

Totally agree with restricting maps

The reason why map is special here is that what you get when you iterate over a map is the values, but what you need to construct is [key, value] tuples.

@pcnfernando pcnfernando reopened this Jul 24, 2023
@jclark
Copy link
Collaborator

jclark commented Jul 26, 2023

The reason is that the semantics of constructing streams is significantly different from constructing other types. I took the view that the difference was significant enough that it should be marked by syntax.

@jclark jclark closed this as completed Jul 26, 2023
@jclark
Copy link
Collaborator

jclark commented Jul 27, 2023

Thinking about this some more, the fundamental difference with stream is that clauses are evaluated lazily; this implies that typing is different for check occurring with these clauses. Although this is a big difference from the implementation semantics point of view, I think one could argue that it doesn't really affect the code that a user has to write, at least not to the same extent that it does with map.

So I think it is worth considering whether we should change the spec to match the implementation here.
@hasithaa WDYT?

@jclark jclark reopened this Jul 27, 2023
@pcnfernando
Copy link
Member Author

If we make stream keyword mandatory we improve readability giving the user a notion that the generated collection would be looped lazily

import ballerina/io;

public function main() {
    stream<int> intStrm = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10].toStream();
    var evenStrm = stream from int i in intStrm
        where isEven(i)
        select i;

    io:println("Do some tasks eagerly");

    from int i in evenStrm
    // isEven()'s io:println() will be called here
    do {
        io:println(i);
    };
}

function isEven(int i) returns boolean {
    io:println("isEven called");
    return i % 2 == 0;
}

But that restriction causes scenarios like below to throw a CE. Hence, I prefer avoiding the inferring restriction

public function dummyTest() {
    stream<int> intStrm = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10].toStream();
    from int i in (from int i in intStrm
        where isEven(i)
        select i)
    do {
        print(i);
    };
}

@jclark
Copy link
Collaborator

jclark commented Jul 27, 2023

I agree that when the stream is temporary (like in your scenario) requiring the stream keyword feels rather cumbersome.

@hasithaa
Copy link
Contributor

I agree; inferring the type does indeed make it easier for developers to write code. If the type is already specified on the LHS, repeating it on the RHS seems redundant. Therefore, if we can safely infer the type, I'm all for omitting the keyword stream.

However, in cases like Chiran's second example, having the keyword stream simplifies the task for the compiler.

@jclark
Copy link
Collaborator

jclark commented Aug 23, 2023

We will change the spec to match the implementation.

@jclark jclark added Type/Improvement Enhancement to language design status/pending Design is agreed and waiting to be added and removed spec/unclear Spec does not clearly explain the design labels Aug 23, 2023
@jclark jclark closed this as completed in c46fa86 Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/Lang Relates to the Ballerina language specification status/pending Design is agreed and waiting to be added Type/Improvement Enhancement to language design
Projects
None yet
Development

No branches or pull requests

4 participants