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

Consider implementing `import * from "x"` #307

Open
hausdorff opened this Issue Mar 28, 2017 · 22 comments

Comments

Projects
None yet
3 participants
@hausdorff

hausdorff commented Mar 28, 2017

We use Jsonnet to express the Kubernetes API objects. The namespace is quite dense, so we end up with a lot of imports, something like [EDITED FOR CLARITY]:

local core = import "core.libsonnet";

local container = core.v1.container;
local claim = core.v1.volume.claim;
local probe = core.v1.probe;
local service = core.v1.service;
local secret = core.v1.secret;
local volume = core.v1.volume;
local configMap = core.v1.configMap;

It would be great to have sugar for this, e.g., JavaScript-style import * from "core.libsonnet", which would generate local definitions similar to those above.

Would doing this upend some important part of the language?

@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Mar 28, 2017

Contributor

A simple workaround is local core = core.v1, or even local c = ….

Contributor

marcelocantos commented Mar 28, 2017

A simple workaround is local core = core.v1, or even local c = ….

@hausdorff

This comment has been minimized.

Show comment
Hide comment
@hausdorff

hausdorff Mar 28, 2017

I'm not sure I understand. It seems like that makes each line smaller (e.g., local container = core.v1.container -> local container = core.container), but what we want is to eliminate all of these lines by doing something like import * from core.v1, or something. So in other words, the idea is that now container, claim, probe, etc., don't have to be defined with a local container = [...] because we've "imported" the namespace core.v1.

Does that make sense?

hausdorff commented Mar 28, 2017

I'm not sure I understand. It seems like that makes each line smaller (e.g., local container = core.v1.container -> local container = core.container), but what we want is to eliminate all of these lines by doing something like import * from core.v1, or something. So in other words, the idea is that now container, claim, probe, etc., don't have to be defined with a local container = [...] because we've "imported" the namespace core.v1.

Does that make sense?

@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Mar 29, 2017

Contributor

You wouldn't define local container = …, just use core.container or c.container everywhere.

Contributor

marcelocantos commented Mar 29, 2017

You wouldn't define local container = …, just use core.container or c.container everywhere.

@hausdorff

This comment has been minimized.

Show comment
Hide comment
@hausdorff

hausdorff Mar 29, 2017

I think there are definitely scenarios where something like this would work (personally, I would prefer local v1 = core.v1, which leads to, e.g., v1.container, which I reckon is nearly as terse as c.container, and a bit more precise), but I'm hoping that we also agree that there's value in something like the form of import above. Or, if we decide explicitness is really important, perhaps something like import { v1.container, v1.claim } from "core.libsonnet";.

hausdorff commented Mar 29, 2017

I think there are definitely scenarios where something like this would work (personally, I would prefer local v1 = core.v1, which leads to, e.g., v1.container, which I reckon is nearly as terse as c.container, and a bit more precise), but I'm hoping that we also agree that there's value in something like the form of import above. Or, if we decide explicitness is really important, perhaps something like import { v1.container, v1.claim } from "core.libsonnet";.

@sparkprime

This comment has been minimized.

Show comment
Hide comment
@sparkprime

sparkprime Apr 3, 2017

Member

Variables are supposed to be statically bound, so the "import * from" would defeat that. Code like

local x = 1;
import * from "foo.libsonnet";
x

might break if somebody added x to foo.libsonnet. Our Python style guide prohibits the import * idiom.

There may be a case for syntax sugar to reduce the size of this though:

local container = core.v1.container;
local claim = core.v1.volume.claim;
local probe = core.v1.probe;
local service = core.v1.service;
local secret = core.v1.secret;
local volume = core.v1.volume;
local configMap = core.v1.configMap;

I.e. without the repeated core.v1 and without the locals. In fact you can already drop the locals:

local
  container = core.v1.container,
  claim = core.v1.volume.claim,
  probe = core.v1.probe,
  service = core.v1.service,
  secret = core.v1.secret,
  volume = core.v1.volume,
  configMap = core.v1.configMap;

To drop the other repeated parts, you could have

local container, claim, probe, service, secret, volume, configMap in core.v1;

This is like what you said, except I'm re-using the existing keyword 'in' and there's no need to tie it into the import syntax, since it might be useful independently. You can always do:

local container, claim, probe, service, secret, volume, configMap in (import "core.libsonnet").v1;
Member

sparkprime commented Apr 3, 2017

Variables are supposed to be statically bound, so the "import * from" would defeat that. Code like

local x = 1;
import * from "foo.libsonnet";
x

might break if somebody added x to foo.libsonnet. Our Python style guide prohibits the import * idiom.

There may be a case for syntax sugar to reduce the size of this though:

local container = core.v1.container;
local claim = core.v1.volume.claim;
local probe = core.v1.probe;
local service = core.v1.service;
local secret = core.v1.secret;
local volume = core.v1.volume;
local configMap = core.v1.configMap;

I.e. without the repeated core.v1 and without the locals. In fact you can already drop the locals:

local
  container = core.v1.container,
  claim = core.v1.volume.claim,
  probe = core.v1.probe,
  service = core.v1.service,
  secret = core.v1.secret,
  volume = core.v1.volume,
  configMap = core.v1.configMap;

To drop the other repeated parts, you could have

local container, claim, probe, service, secret, volume, configMap in core.v1;

This is like what you said, except I'm re-using the existing keyword 'in' and there's no need to tie it into the import syntax, since it might be useful independently. You can always do:

local container, claim, probe, service, secret, volume, configMap in (import "core.libsonnet").v1;
@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Apr 3, 2017

Contributor

Would JavaScript destructuring syntax work?

local { container, claim, … } = core.v1;
Contributor

marcelocantos commented Apr 3, 2017

Would JavaScript destructuring syntax work?

local { container, claim, … } = core.v1;
@sparkprime

This comment has been minimized.

Show comment
Hide comment
@sparkprime

sparkprime Apr 3, 2017

Member

Yeah it's preferable to use an existing syntax if one exists. That looks like it shouldn't clash with anything, although it's a bit weird to see { } used for a sequence instead of [ ]

Member

sparkprime commented Apr 3, 2017

Yeah it's preferable to use an existing syntax if one exists. That looks like it shouldn't clash with anything, although it's a bit weird to see { } used for a sequence instead of [ ]

@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Apr 4, 2017

Contributor

Technically, it's not a sequence. In JavaScript, {a:1, b} is shorthand for {a:1, b:b}. Destructuring assignment reuses the same pattern.

Contributor

marcelocantos commented Apr 4, 2017

Technically, it's not a sequence. In JavaScript, {a:1, b} is shorthand for {a:1, b:b}. Destructuring assignment reuses the same pattern.

@hausdorff

This comment has been minimized.

Show comment
Hide comment
@hausdorff

hausdorff Apr 4, 2017

I think the local [...] in (import "blah") syntax works quite nicely for the above use case. What are the next steps you would like to see?

hausdorff commented Apr 4, 2017

I think the local [...] in (import "blah") syntax works quite nicely for the above use case. What are the next steps you would like to see?

@sparkprime

This comment has been minimized.

Show comment
Hide comment
@sparkprime

sparkprime Apr 4, 2017

Member

I think local { foo, bar } = expr isn't so bad actually, because it's an object being deconstructed, so the left hand side semantically matches the right hand side. We might want to also allow local [foo, bar] = expr for deconstructing a list as well, but no need to do them both at once.

But any change to local should include both the local x = e; form and the local x = e, form (i.e. the one inside objects).

Also, the mutually recursive form should work too, i.e.

local
  {a, b, c} = {a: y, b: c, c: x},
  {x, y, z} = {x: a, y: 2, z: b};
z

should evaluate to 2

We need to decide whether the object on the right hand side is allowed to have more fields than the one on the left. My instinct is that it should be allowed to have more fields than you're pulling out. Does Javascript have those semantics?

Although it's not so hard to implement, I'm not going to have time to do this any time soon. But if you want to, the steps are:

  1. Modify the local AST in ast.h, preserving all the comments and whitespace that are possible.
  2. Modify the parser to generate the new AST
  3. Modify the formatter to handle the new kind of AST
  4. Modify the desugarer to convert the new kind of AST to the existing kind, this will allow execution.
  5. Write tests
  6. Update spec (just desugaring and syntax parts)
  7. Update tutorial

The actual desugaring is going to be somewhat complex. For the general form:

local {a, b, c} = expr1, { x, y } = expr2; expr

It's not strictly necessary to factor out the expr1, but in the case where it is expensive, it will only be executed once if you do it as so:

local
$expr1 = expr1,
a = $expr1.a,
b = $expr1.b,
c = $expr1.c,
$expr2 = expr2,
x = $expr2.x,
y = $expr2.y;
expr

Member

sparkprime commented Apr 4, 2017

I think local { foo, bar } = expr isn't so bad actually, because it's an object being deconstructed, so the left hand side semantically matches the right hand side. We might want to also allow local [foo, bar] = expr for deconstructing a list as well, but no need to do them both at once.

But any change to local should include both the local x = e; form and the local x = e, form (i.e. the one inside objects).

Also, the mutually recursive form should work too, i.e.

local
  {a, b, c} = {a: y, b: c, c: x},
  {x, y, z} = {x: a, y: 2, z: b};
z

should evaluate to 2

We need to decide whether the object on the right hand side is allowed to have more fields than the one on the left. My instinct is that it should be allowed to have more fields than you're pulling out. Does Javascript have those semantics?

Although it's not so hard to implement, I'm not going to have time to do this any time soon. But if you want to, the steps are:

  1. Modify the local AST in ast.h, preserving all the comments and whitespace that are possible.
  2. Modify the parser to generate the new AST
  3. Modify the formatter to handle the new kind of AST
  4. Modify the desugarer to convert the new kind of AST to the existing kind, this will allow execution.
  5. Write tests
  6. Update spec (just desugaring and syntax parts)
  7. Update tutorial

The actual desugaring is going to be somewhat complex. For the general form:

local {a, b, c} = expr1, { x, y } = expr2; expr

It's not strictly necessary to factor out the expr1, but in the case where it is expensive, it will only be executed once if you do it as so:

local
$expr1 = expr1,
a = $expr1.a,
b = $expr1.b,
c = $expr1.c,
$expr2 = expr2,
x = $expr2.x,
y = $expr2.y;
expr

@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Apr 4, 2017

Contributor

If you're going to go down this path, several elements could be considered in addition to the above suggestions:

  1. Optional attribute:variable syntax: local {a:x} = {a:1} is equivalent to local x = {a:1}.a. This makes it easier to assign from multiple objects with common attribute names.
  2. Remainder syntax via ...var. For instance, local [a, ...b, c] = [1, 2, 3, 4]; [a, b, c] outputs [1, [2, 3], 4], and local {a, ...b} = {a:1, b:2, c:3}; [a, b] outputs [1, {b:2, c:3}].
  3. Nested destructuring: local [a, {b, c:[d]}] = [42, {a:1, b:2, c:[3, 4]}]; [a, b, d] outputs [42, 2, 3].
  4. Function parameter destructuring: (function({a, b}) a + b)({a:2, b:3}) outputs 5.
  5. Literal matching: local {type:"foo", value} = {type:"bar", value:42} would fail, and local [a, b, ...[]] = x would fail unless std.length(x) == 2.
  6. Default assignment: local {a=1, b=2} = {a:42}; [a, b] outputs [42, 2].
  7. Require-but-ignore syntax: local {a, b:} = o requires o.b to exist, but doesn't assign it to a variable.

You could also allow attribute shorthand in object-construction, e.g.: {a, b:x} = {a:a, b:x}. This isn't a feature of destructuring assignment, but it would make destructuring and object-construction more symmetrical. This even suggests allowing {a:1, ...o} for object construction, but this is redundant syntax for {a:1} + o, so I don't know if it's a good idea. Likewise for arrays. I suppose you could go the other way for remainder destructuring: local {a} + o = …. Technically, that would work, but have I jumped the shark?

All the above together would be fairly ambitious, and in some cases the cost-benefit wouldn't be obvious. So I'd propose starting simple, but keeping the above in mind so that decisions made now don't exclude future possibilities.

I'd love to tackle the base case, so if no one else plans to work on this, I'll take it.

Contributor

marcelocantos commented Apr 4, 2017

If you're going to go down this path, several elements could be considered in addition to the above suggestions:

  1. Optional attribute:variable syntax: local {a:x} = {a:1} is equivalent to local x = {a:1}.a. This makes it easier to assign from multiple objects with common attribute names.
  2. Remainder syntax via ...var. For instance, local [a, ...b, c] = [1, 2, 3, 4]; [a, b, c] outputs [1, [2, 3], 4], and local {a, ...b} = {a:1, b:2, c:3}; [a, b] outputs [1, {b:2, c:3}].
  3. Nested destructuring: local [a, {b, c:[d]}] = [42, {a:1, b:2, c:[3, 4]}]; [a, b, d] outputs [42, 2, 3].
  4. Function parameter destructuring: (function({a, b}) a + b)({a:2, b:3}) outputs 5.
  5. Literal matching: local {type:"foo", value} = {type:"bar", value:42} would fail, and local [a, b, ...[]] = x would fail unless std.length(x) == 2.
  6. Default assignment: local {a=1, b=2} = {a:42}; [a, b] outputs [42, 2].
  7. Require-but-ignore syntax: local {a, b:} = o requires o.b to exist, but doesn't assign it to a variable.

You could also allow attribute shorthand in object-construction, e.g.: {a, b:x} = {a:a, b:x}. This isn't a feature of destructuring assignment, but it would make destructuring and object-construction more symmetrical. This even suggests allowing {a:1, ...o} for object construction, but this is redundant syntax for {a:1} + o, so I don't know if it's a good idea. Likewise for arrays. I suppose you could go the other way for remainder destructuring: local {a} + o = …. Technically, that would work, but have I jumped the shark?

All the above together would be fairly ambitious, and in some cases the cost-benefit wouldn't be obvious. So I'd propose starting simple, but keeping the above in mind so that decisions made now don't exclude future possibilities.

I'd love to tackle the base case, so if no one else plans to work on this, I'll take it.

@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Apr 5, 2017

Contributor

Hmm, I'm not in a position to sign a CLA. I'll follow it up with my employer, but I wouldn't hold your breath. Nothing moves fast at a bank.

Contributor

marcelocantos commented Apr 5, 2017

Hmm, I'm not in a position to sign a CLA. I'll follow it up with my employer, but I wouldn't hold your breath. Nothing moves fast at a bank.

@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Dec 4, 2017

Contributor

This is still on my radar, and I'm working on getting an IP release from my employer in the next week or two. Meanwhile, I've implemented the basic syntax: local {x, y} = {x:6, y:7}; x * y in both local and object-field context.

I'm currently looking at remainder syntax: local {x, ...r} = {x:6, y:7}; x + r.y. This brings up a question. The JavaScript syntax I've shown here doesn't comport with jsonnet's current object construction syntax: {x: 42} + r. I'm wondering whether I should match jsonnet: local {x} + r = ... or extend object construction: {x: 42, ...r}. I find {x} + r to be somewhat awkward as an assignment target, but {x: 42, ...r} construction syntax is mostly redundant with current syntax: {x: 42} + r. It's not quite the same, since super references would be resolved within the object, not after it, which may have negative and/or positive ramifications. Thoughts?

Contributor

marcelocantos commented Dec 4, 2017

This is still on my radar, and I'm working on getting an IP release from my employer in the next week or two. Meanwhile, I've implemented the basic syntax: local {x, y} = {x:6, y:7}; x * y in both local and object-field context.

I'm currently looking at remainder syntax: local {x, ...r} = {x:6, y:7}; x + r.y. This brings up a question. The JavaScript syntax I've shown here doesn't comport with jsonnet's current object construction syntax: {x: 42} + r. I'm wondering whether I should match jsonnet: local {x} + r = ... or extend object construction: {x: 42, ...r}. I find {x} + r to be somewhat awkward as an assignment target, but {x: 42, ...r} construction syntax is mostly redundant with current syntax: {x: 42} + r. It's not quite the same, since super references would be resolved within the object, not after it, which may have negative and/or positive ramifications. Thoughts?

@sparkprime

This comment has been minimized.

Show comment
Hide comment
@sparkprime

sparkprime Dec 6, 2017

Member

Thanks for the update.

Does local { x } = {x::1}; x produce 1?
Does local { ...r } = {x::1}; r produce {} or {x: 1} (i.e. does it preserve the ::)?

It's actually a bit tricky to preserve the hidden-ness of fields right now because you can't do { [k]:: v for k in ['a', 'b', 'c'] }. That could be fixed, however, allowing you to compose one comprehension with : and one with ::. Unfortunately we don't have a way to programmatically choose the number of colons, and it's not obvious what syntax we could even use to add that.

Implementing ...r would be a lot easier if we had an explicit way to delete fields.

I would punt ...r for now. It's not being asked for in the original request from @hausdorff

Member

sparkprime commented Dec 6, 2017

Thanks for the update.

Does local { x } = {x::1}; x produce 1?
Does local { ...r } = {x::1}; r produce {} or {x: 1} (i.e. does it preserve the ::)?

It's actually a bit tricky to preserve the hidden-ness of fields right now because you can't do { [k]:: v for k in ['a', 'b', 'c'] }. That could be fixed, however, allowing you to compose one comprehension with : and one with ::. Unfortunately we don't have a way to programmatically choose the number of colons, and it's not obvious what syntax we could even use to add that.

Implementing ...r would be a lot easier if we had an explicit way to delete fields.

I would punt ...r for now. It's not being asked for in the original request from @hausdorff

@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Dec 9, 2017

Contributor

local { x } = {x::1}; x produces 1. It desugars to local $local_1 = {["x"]:: local $ = self; 1,}, x = $local_1.x; x.

I haven't started on ...r, so I haven't worked through the implications of hiddenness yet. Thinking-out-loud, I would expect to simply propagate all fields, preserving hiddenness. The implementation might require adding some std helpers (or internal equivalents):

std = {
    // …
    reduce(f, acc, list)::
        if list==[] then
            acc
        else
            self.reduce(f,f(acc,list[0]),list[1:]),
    elideFields(object, fields)::
        local del = self.set(fields);
        local vis = self.setDiff(self.sort(self.objectFields(object)), del);
        local all = self.setDiff(self.sort(self.objectFieldsAll(object)), del);
        local a = self.reduce(
            function(acc, f) acc + {[f]: object[f]}, {}, vis);
        self.reduce(
            function(acc, f) acc + {[f]:: object[f]}, a, self.setDiff(all, vis)),
}

I also haven't thought through how super interacts with any of this.

I'll hold off implementing ...r for now and keep you posted on the IP release (any day now).

Contributor

marcelocantos commented Dec 9, 2017

local { x } = {x::1}; x produces 1. It desugars to local $local_1 = {["x"]:: local $ = self; 1,}, x = $local_1.x; x.

I haven't started on ...r, so I haven't worked through the implications of hiddenness yet. Thinking-out-loud, I would expect to simply propagate all fields, preserving hiddenness. The implementation might require adding some std helpers (or internal equivalents):

std = {
    // …
    reduce(f, acc, list)::
        if list==[] then
            acc
        else
            self.reduce(f,f(acc,list[0]),list[1:]),
    elideFields(object, fields)::
        local del = self.set(fields);
        local vis = self.setDiff(self.sort(self.objectFields(object)), del);
        local all = self.setDiff(self.sort(self.objectFieldsAll(object)), del);
        local a = self.reduce(
            function(acc, f) acc + {[f]: object[f]}, {}, vis);
        self.reduce(
            function(acc, f) acc + {[f]:: object[f]}, a, self.setDiff(all, vis)),
}

I also haven't thought through how super interacts with any of this.

I'll hold off implementing ...r for now and keep you posted on the IP release (any day now).

@sparkprime

This comment has been minimized.

Show comment
Hide comment
@sparkprime

sparkprime Dec 11, 2017

Member

Ignoring the :: in the deconstruction sounds good to me.

(Referring to local { x } = {x::1}; x)

Member

sparkprime commented Dec 11, 2017

Ignoring the :: in the deconstruction sounds good to me.

(Referring to local { x } = {x::1}; x)

@sparkprime

This comment has been minimized.

Show comment
Hide comment
@sparkprime

sparkprime Dec 11, 2017

Member

A way to delete a field (without ::) is something people ask for from time to time so I think it's better to just add that. Then you don't need any elideFields call that then has to preserve the : and ::

Member

sparkprime commented Dec 11, 2017

A way to delete a field (without ::) is something people ask for from time to time so I think it's better to just add that. Then you don't need any elideFields call that then has to preserve the : and ::

@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Feb 28, 2018

Contributor

I finally got CLA-signing approval (as you'll have seen on go-jsonnet). I've put together a commit for basic destructuring assignment as described above (still leaving out ...r) https://goo.gl/Lg275S. I can submit it as a PR, but I have a few concerns with it, and might end up discarding it, but I thought I'd canvass some feedback first.

The first issue is that, while implementing this simple change, exclusively for local { … } = …, I realised that it makes more sense to implement destructuring assignment anywhere a name can be bound to a value. That would include function parameters function({x, y}) x / y and comprehensions [x / y for {x, y} in foo]. Name binding also occurs in object constructors and comprehensions, but that would be way messy.

The second issue occurred to me when I started thinking about how to implement function-parameter destructuring, and I realised that the approach I took for the basic implementation doesn't readily extend to the other cases. As it was, even local assignment required duplication of logic in multiple places (https://goo.gl/QxHhTR vs https://goo.gl/hh3m5A, https://goo.gl/jWchdn vs https://goo.gl/BNf3VW, and https://goo.gl/iBZfXw vs https://goo.gl/qHvBZU).

A nastier third issue came to mind with respect to desugaring. It appears that each construct would require quite different desugarings. Functions might be straightforward, since function({x, y}) <expr> can first become function($1) local {x, y} = $1; <expr> and then go through local desugaring. Comprehension syntax, however, would be a trickier beast since it only allows binding of one name per for. Rewriting the final expression wouldn't work since subsequent for and if clauses would expect to see the destructured names. One solution is to rewrite [… for {x, y} in foo …] as [… for $1 in foo for x in [$1.x] for y in [$1.x] …], but this won't scale to more complex destructuring scenarios I want to implement later ([… for {x, [y, {z:w}]} in foo … ]). A more complex, but future-proof rewrite is [… for $1 in foo for $2 in [(function() local {x, y} = $1; [x, y])()] for x in [$2[0]] for y in [$2[1]] …]. This way, all destructuring scenarios would ultimately reduce to the one local construct, which can then be enriched over time without duplicating effort across different bindings.

Another thought was to extend the core language to support local in comprehensions: [y*y for x in z local y = 1 - 1/x]. This would then allow a simpler rewrite of the earlier case: [… for $1 in foo local {x, y} = $1 …]. It would also improve comprehensions in general by allowing intermediate calculations without the ugly … for x in [<expr>] … hack.

Contributor

marcelocantos commented Feb 28, 2018

I finally got CLA-signing approval (as you'll have seen on go-jsonnet). I've put together a commit for basic destructuring assignment as described above (still leaving out ...r) https://goo.gl/Lg275S. I can submit it as a PR, but I have a few concerns with it, and might end up discarding it, but I thought I'd canvass some feedback first.

The first issue is that, while implementing this simple change, exclusively for local { … } = …, I realised that it makes more sense to implement destructuring assignment anywhere a name can be bound to a value. That would include function parameters function({x, y}) x / y and comprehensions [x / y for {x, y} in foo]. Name binding also occurs in object constructors and comprehensions, but that would be way messy.

The second issue occurred to me when I started thinking about how to implement function-parameter destructuring, and I realised that the approach I took for the basic implementation doesn't readily extend to the other cases. As it was, even local assignment required duplication of logic in multiple places (https://goo.gl/QxHhTR vs https://goo.gl/hh3m5A, https://goo.gl/jWchdn vs https://goo.gl/BNf3VW, and https://goo.gl/iBZfXw vs https://goo.gl/qHvBZU).

A nastier third issue came to mind with respect to desugaring. It appears that each construct would require quite different desugarings. Functions might be straightforward, since function({x, y}) <expr> can first become function($1) local {x, y} = $1; <expr> and then go through local desugaring. Comprehension syntax, however, would be a trickier beast since it only allows binding of one name per for. Rewriting the final expression wouldn't work since subsequent for and if clauses would expect to see the destructured names. One solution is to rewrite [… for {x, y} in foo …] as [… for $1 in foo for x in [$1.x] for y in [$1.x] …], but this won't scale to more complex destructuring scenarios I want to implement later ([… for {x, [y, {z:w}]} in foo … ]). A more complex, but future-proof rewrite is [… for $1 in foo for $2 in [(function() local {x, y} = $1; [x, y])()] for x in [$2[0]] for y in [$2[1]] …]. This way, all destructuring scenarios would ultimately reduce to the one local construct, which can then be enriched over time without duplicating effort across different bindings.

Another thought was to extend the core language to support local in comprehensions: [y*y for x in z local y = 1 - 1/x]. This would then allow a simpler rewrite of the earlier case: [… for $1 in foo local {x, y} = $1 …]. It would also improve comprehensions in general by allowing intermediate calculations without the ugly … for x in [<expr>] … hack.

@marcelocantos

This comment has been minimized.

Show comment
Hide comment
@marcelocantos

marcelocantos Feb 28, 2018

Contributor

With all that in mind, my current plan is to start again and do the work in multiple phases:

  1. Refactor local assignment syntax to eliminate the aforementioned duplication.
  2. Reimplement local destructuring assignment.
  3. Implement the above desugaring for function parameters.
  4. Implement the above desugaring for comprehensions.
  5. Implement local in comprehensions as desugaring.
  6. Redo comprehension desugaring.
  7. Maybe reimplement comprehension local in core.

Thoughts?

Contributor

marcelocantos commented Feb 28, 2018

With all that in mind, my current plan is to start again and do the work in multiple phases:

  1. Refactor local assignment syntax to eliminate the aforementioned duplication.
  2. Reimplement local destructuring assignment.
  3. Implement the above desugaring for function parameters.
  4. Implement the above desugaring for comprehensions.
  5. Implement local in comprehensions as desugaring.
  6. Redo comprehension desugaring.
  7. Maybe reimplement comprehension local in core.

Thoughts?

@sparkprime

This comment has been minimized.

Show comment
Hide comment
@sparkprime

sparkprime Mar 4, 2018

Member

Sorry I only just saw this. I need to look closely at your work but I will say now that local in comprehensions has come up before: #90

Member

sparkprime commented Mar 4, 2018

Sorry I only just saw this. I need to look closely at your work but I will say now that local in comprehensions has come up before: #90

@sparkprime

This comment has been minimized.

Show comment
Hide comment
@sparkprime

sparkprime Mar 4, 2018

Member

Also, the go-jsonnet desugaring of comprehensions is completely different to the C++ one.

Member

sparkprime commented Mar 4, 2018

Also, the go-jsonnet desugaring of comprehensions is completely different to the C++ one.

@sparkprime

This comment has been minimized.

Show comment
Hide comment
@sparkprime

sparkprime Apr 23, 2018

Member

Sorry it took me so long to get back to you on this.

To your first point (whether we need to do it everywhere a binding occurs), I also prefer it to be symmetric as that's less surprising for users. However we could definitely stage this and I think the most important case right now is importing specific things from a library, so just the ; local is needed. Since the object-level local is so similar, I think it's reasonable to support that too in the first pass.

When you desugar the object locals, you can expand them to a local within each field that still has the destructuring sugar in it, then when you recurse the desugaring on those expressions, it will use the code for the regular local desugaring to eliminate the destructuring. In other words, the desugaring code need not be duplicated between the two. Other than that, you're talking about the feature cropping up in parsing, desugaring, and unparsing, twice in each. While there is a bit of duplication, it's somewhat minimal and you're able to make use of parseParams and unparseParams so it's fine IMO.

For functions, I agree with you that desugaring it into a destructured local would work well. For comprehensions, I think we should add the local to the comprehensions syntax as you proposed and as requested in #90. That can all wait though, the only important thing is that we're not painting ourselves into a corner (which we're not).

So in summary your work so far looks quite good and I wouldn't throw it away.

Member

sparkprime commented Apr 23, 2018

Sorry it took me so long to get back to you on this.

To your first point (whether we need to do it everywhere a binding occurs), I also prefer it to be symmetric as that's less surprising for users. However we could definitely stage this and I think the most important case right now is importing specific things from a library, so just the ; local is needed. Since the object-level local is so similar, I think it's reasonable to support that too in the first pass.

When you desugar the object locals, you can expand them to a local within each field that still has the destructuring sugar in it, then when you recurse the desugaring on those expressions, it will use the code for the regular local desugaring to eliminate the destructuring. In other words, the desugaring code need not be duplicated between the two. Other than that, you're talking about the feature cropping up in parsing, desugaring, and unparsing, twice in each. While there is a bit of duplication, it's somewhat minimal and you're able to make use of parseParams and unparseParams so it's fine IMO.

For functions, I agree with you that desugaring it into a destructured local would work well. For comprehensions, I think we should add the local to the comprehensions syntax as you proposed and as requested in #90. That can all wait though, the only important thing is that we're not painting ourselves into a corner (which we're not).

So in summary your work so far looks quite good and I wouldn't throw it away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment