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

variable fieldnames do not seem to have a context #217

Closed
jwise opened this issue Jul 13, 2016 · 8 comments
Closed

variable fieldnames do not seem to have a context #217

jwise opened this issue Jul 13, 2016 · 8 comments

Comments

@jwise
Copy link

jwise commented Jul 13, 2016

I tried to use a variable field name as part of a mixin, and it seems that field names don't seem to be evaluated in the scope of the object they came from. If I try using self, I get the error "Can't use self outside of an object", and if I try using the standard technique to bind it with a local, I get the local as an "Unknown variable".

Additionally, semantics for fieldnames of pattern '[' e ']' don't appear to be specified in the spec.

Here's an example of what I'm trying to accomplish:

local Base = {
  a0_ctrl: { foo: 0 },
  a1_ctrl: { foo: 1 },
};

local Mixin = {
  local me = self,

  id:: error "must override id",

  ["a%d_ctrl" % (me.id)] +: { bar: true },
};

Base + Mixin { id:: 1 }

Looking at core/static_analysis.cpp:130 or so, it looks like the right thing should be happening, but I don't understand enough as to why the wrong vars are showing up there. (I'm not sure if my employer has a CLA signed, so I don't want to go digging too deeply, since I can't produce a patch anyway 😄.)

Looks like this might also have been tripped on in case_studies/micromanage/lib/mmlib/v0.1.2/service/amazon.libsonnet:256 or so?

@sparkprime
Copy link
Contributor

sparkprime commented Jul 13, 2016

In the case of { [e]: e' } you are "inside" the object in e' but not in e. This means that you don't get self or super or access to object-level locals (although they may bind to levels further out). This is by design. The reason is that the object is not yet fully formed during execution of e and this creates problems. For example, if the semantics were as you were expecting, tell me what these would do:

{ [std.objectFields(self)[0]]: 3 }  # Self-referential field name
{ [if std.length(self) == 0 then "foo"]: 4 }  # Paradox
{ [self.x]: "y", [self.y]: "x" }  # This paradox is much more fun

The design actually does manifest in the spec in a few ways:

  1. Desugaring of object-level locals only puts them into field bodies, not field names.
  2. In the type system rule chk-object the e1..en are not checked in scope of self/super.
  3. In capture-avoiding substitution of keywords (self/super) the field name captures self/super from outside the braces.

However I would say that this definitely needs to be more prominently described in the tutorial because it does affect programmers and otherwise it's not really visible except to language nerds.

It is also the case that you are not inside the object in e'' in the case of {[e]: e' for x in e"} for similar reasons -- you're still deciding how the object is structured (i.e. the set of fields it has).

@sparkprime
Copy link
Contributor

In case_studies/micromanage/lib/mmlib/v0.1.2/service/amazon.libsonnet:256 the local var service is used, which is defined further out and is therefore OK. The line is commented out, but looks like it should work.

@sparkprime
Copy link
Contributor

In static_analysis.cpp

        for (auto &field : ast->fields) {
            append(r, static_analysis(field.name, in_object, vars));
            append(r, static_analysis(field.body, true, vars));
        }

What is happening there is the in_object status is preserved from outside the braces in the case of field names, but overridden to true in the case of field bodies. The same set of vars are bound in both cases, because static analysis occurs after desugaring, and desugaring removes the object-level locals (inlining them into field bodies). So, the same set of variables is visible in both cases.

@jwise
Copy link
Author

jwise commented Jul 13, 2016

Interesting! Ok, yes, that makes sense; it is reasonable, though non-intuitive, and I think that it probably misses some important use cases.

I think there are two ways to make this semantically meaningful. Either:

  1. Field names could have a defined evaluation order, based on the AST. I imagine that this could prove to be disdainful, since as I recall, the language is currently unordered, but it sounds like it would be the easiest thing to implement, anyway.
  2. Or, field names could be evaluated in a "two-phase" mechanism -- first, all "trivial" field names are evaluated (i.e., those that are simple literal expressions) and added to the context, and then in the second phase, all "non-trivial" field names are added, checking against "trivial" field names for context only, but not checking against any other "non-trivial" field names.

I think, as it turns out, both of these would result in the same evaluation for the three examples you suggested before -- { }, { "foo": 4 }, and { }, respectively.

But, irritatingly, I think they would result in the wrong answer for the test that I gave in my original issue! I'm not sure what the interplay here would be with overriding fields, or what phase that's implemented at.

@sparkprime
Copy link
Contributor

Yeah I think the issue is fundamental and any paradox-free design would have more edge cases than the current design.

I think you can do what you want like this though?

local Base = {
  a0_ctrl: { foo: 0 },
  a1_ctrl: { foo: 1 },
};

local Mixin(id) = {
  local me = self,
  ["a%d_ctrl" % id] +: { bar: true },
};

Base + Mixin(id=1)

This static binding of id means you cannot override it later of course, but if you don't need that it should be fine.

@jwise
Copy link
Author

jwise commented Jul 13, 2016

Ok, I spent a bunch of this afternoon converting stuff to that style. It's tough to reduce what I'm trying to do too much without just giving you the entire source; in practice, what doing that forces me to do is to now pass around a 'params' object as a parameter, and do my inheritance like that. It's not the end of the world, but it is a little less clean.

I'm not sure that the other solutions would have more edge cases, but certainly they would have different edge cases -- trading off some unintuitive behavior in the early stage to unintuitive behavior later on.

Either way, thanks for the workaround.

@sparkprime
Copy link
Contributor

If you can share some code that is close to the concrete thing you're doing I would definitely be interested in seeing it. You can obfuscate all the primitive values and field names, because really it is the differences and similarities between different parts of the JSON output that matter (and the kinds of modifications one routinely wants to make to them). The absolute values don't matter at all.

@jwise
Copy link
Author

jwise commented Jul 16, 2016

I'll try and alpha-vary it either this weekend or next week at work.

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

No branches or pull requests

2 participants