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

spreading a dynamic value #270

Open
gavinking opened this issue Sep 30, 2013 · 13 comments
Open

spreading a dynamic value #270

gavinking opened this issue Sep 30, 2013 · 13 comments
Labels
Milestone

Comments

@gavinking
Copy link
Member

The backend needs to support spreading a dynamic value with the * operator. This was something we agreed on with @corbinu.

@gavinking
Copy link
Member Author

This was ceylon/ceylon-spec#800, FTR.

@chochos
Copy link
Member

chochos commented Oct 1, 2013

This has a somewhat limited functionality. I can do this:

value tuple = [1,"2",'3'];
somethingDynamic.spread(*tuple); //turns into tuple.get(0), tuple.get(1), tuple.get(2)
somethingDynamic.spread(*someDynamicArray); //stays as someDynamicArray

please check the test code in the related commits @corbinu

chochos added a commit that referenced this issue Oct 1, 2013
chochos added a commit that referenced this issue Oct 1, 2013
@corbinu
Copy link
Contributor

corbinu commented Oct 1, 2013

First off I believe the second test should be?

issue270.push(*issue270);
check(issue270.length==10, "dynamic spread #270 [2]");

as the elements already in issue270 would be spread across push so issue270 would go from
{0,0,1,"2",'3'} -> {0,0,1,"2",'3',0,0,1,"2",'3'}

My only other thought is sometimes a dynamic value might be iterable or not. I assume if it is not that the spread will do nothing. So I think there should be a two more tests

dynamic addString = "test string";
issue270.push(*addString);
check(issue270.length==11, "dynamic spread #270 [3]");
dynamic addNull = null;
issue270.push(*addNull);
check(issue270.length==11, "dynamic spread #270 [4]");

@gavinking
Copy link
Member Author

If the value is not Iterable there should be an exception thrown by a runtime type check.

Sent from my iPhone

On 1 Oct 2013, at 8:40 am, Corbin Uselton notifications@github.com wrote:

First off I believe the second test should be?

issue270.push(*issue270);
check(issue270.length==10, "dynamic spread #270 [2]");
as the elements already in issue270 would be spread across push so issue270 would go from
{0,0,1,"2",'3'} -> {0,0,1,"2",'3',0,0,1,"2",'3'}

My only other thought is sometimes a dynamic value might be iterable or not. I assume if it is not that the spread will do nothing. So I think there should be a two more tests

dynamic addString = "test string";
issue270.push(_addString);
check(issue270.length==11, "dynamic spread #270 [3]");
dynamic addNull = null;
issue270.push(_addNull);
check(issue270.length==11, "dynamic spread #270 [4]");

Reply to this email directly or view it on GitHub.

@corbinu
Copy link
Contributor

corbinu commented Oct 1, 2013

Ok that works too. Given the major use case is for varadic JS functions though does seem a bit of a waste having to check if none, one or many args were passed to the wrapper function each and every time.

@chochos
Copy link
Member

chochos commented Oct 1, 2013

I know it seems like the second test should add 5 elements to the array, but it's simply impossible to spread that array like I spread the tuple, because I don't know the length of the array at compile time (I can't even know it's an array at compile time; it's a dynamic value).

As for iterables, I can check if the argument is an Iterable, but then what? How can I spread an Iterable to pass its elements to a native js call? I can't.

If I add the runtime type check as @gavinking suggests then the second test will throw an exception, since issue270 is not an Iterable. But like I said before, even if it's an Iterable, there's no way to spread it for a native js call.

@corbinu
Copy link
Contributor

corbinu commented Oct 1, 2013

Well maybe I am wrong but my understanding is Ceylon Tuples and Sequences are simplified to a native Array or a generic array-like object correct? couldn't the generated code just look something like this

if (dynamicVal === null) {
     methodSpreadOn();
} else if (Array.isArray(dynamicVal)) {
     methodSpreadOn.apply(null, dynamicVal);
} else {
    methodSpread(dynamicVal);
}

@chochos
Copy link
Member

chochos commented Oct 1, 2013

That's quite convoluted for a simple invocation. But the real problem is that we can't really use apply because that only works for top-level methods that have not been wrapped in a JsCallable.

And no, tuples and sequences are not simplified to narive arrays. Ceylon methods with variadic parameters work similar to Java methods with varargs: the variadic parameter is really a Sequential inside the method, and there's a validation that sets the argument to empty if it's undefined (in case it was called with no args). The invocations enclose the listed and/or spread arguments corresponding to a variadic param inside a simple array.

@corbinu
Copy link
Contributor

corbinu commented Oct 1, 2013

Well it doesn't sound like there is a simple unconvoluted solution. Could apply not be easily added to JsCallable? or the internal native js function extracted from JsCallable much like how a Ceylon String is unwrapped?

Well I never assumed it would use the Ceylon way of spreading varidic parameters as the dynamic type is not guaranteed to be a Ceylon object. Which I think is a lot of the issue. I defiantly agree this is more complicated to implement then I had anticipated though. I will will see if perhaps I can take a deeper look at JsCallable etc tomorrow

@chochos
Copy link
Member

chochos commented Nov 8, 2013

Moving to 1.1

@chochos
Copy link
Member

chochos commented Jun 20, 2014

So I guess we could have a native function that receives a Ceylon Iterable (Tuple, Sequence, Array, whatever) and returns a JS array, or checks if the received object is already a native JS array and just returns it; otherwise, throw an Error.

And we can use this function with the apply idea to make this work.

@corbinu
Copy link
Contributor

corbinu commented Jun 20, 2014

Sounds good .... Sorry I know my interop work finds the most crazy edge cases ever.

@chochos
Copy link
Member

chochos commented Jun 20, 2014

better sooner than later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants