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

Self references to fields within maps #360

Open
iFrozenPhoenix opened this issue Feb 5, 2022 · 11 comments
Open

Self references to fields within maps #360

iFrozenPhoenix opened this issue Feb 5, 2022 · 11 comments

Comments

@iFrozenPhoenix
Copy link

Currently it is not possible to reference a field within a map to another field within the same map, it leads to a nil pointer reference.
Example:

testmap := {
  metadata: {
    name: "testapp",
    labels: {
      labela: "yxc",
      labelb: "qwe"
    }
  },
  spec: testmap.metadata.name
}

I think it would make sense to support self references within maps as this would enable many use cases as a configuration language.
I've seen a pull request for self references but unfortunately I cannot find it anymore.

In my opinion it would make sense to reference to a field within the same map with a "$" instead of self as this would enable to capability to use existing JSONNET references. What do you think?

If we could agree on a syntax and if this should be allowed within the lang I would be happy to work on it and send PR.

@geseq
Copy link
Collaborator

geseq commented Feb 5, 2022

I don’t agree with this. Right now we have a clear model of variable definition and assignment and a feature like this would blur the lines and complicate it possibly leading to edge cases that would add significant maintenance overhead.

@iFrozenPhoenix
Copy link
Author

I am only talking about referencing a field of a map within the same map, so the overall effect should be limited to a map field. For sure, new features, in most cases, lead possibly to more complications because there are more variables. But this would be an optional feature, i.e. if you don't use the keyword that references to the map itself, the behavior is like at the moment.

This feature could remove a lot of boilerplate in configuration files and also reduce errors in the resulting configurations.

@geseq
Copy link
Collaborator

geseq commented Feb 5, 2022

Feel free to create a PR if you can manage one in a way that doesn’t add excessive maintenance cost.

@iFrozenPhoenix
Copy link
Author

@geseq thanks.
Would you prefer $ or self.
Could you eventually point me to the relevant files?

@geseq
Copy link
Collaborator

geseq commented Feb 5, 2022

Tbh I’m not sure about either. Is your use of $ coming from another language? I think it’s also good to get thoughts from @d5 on his opinion about this as well.

@geseq
Copy link
Collaborator

geseq commented Feb 5, 2022

Also to be entirely clear I don’t think this is achievable in as simple a way as you envision it to be and I wouldn’t be amenable to merging something that makes major changes that I think it’ll take.

That said you should look in the parser, specifically scanner.go and follow the scanning for Define token

@geseq
Copy link
Collaborator

geseq commented Feb 5, 2022

On second thought maybe you can bypass the parser entirely and do it from

tengo/vm.go

Line 293 in 1bcc189

case parser.OpMap:

EDIT: this will likely be a crude solution

@iFrozenPhoenix
Copy link
Author

I'll take a look at the two entry points.

@d5
Copy link
Owner

d5 commented Feb 5, 2022

Hi @iFrozenPhoenix.

We can certainly look into this if you open a PR. But, please understand that we may not accept it if it would involve a significant maintenance cost like @geseq said. Tengo has been very stable (for a 2-person maintained open source project) and fast, and we want to keep it that way if possible.

That being said, last time we looked into this, one particular challenge I remember is how we should define this 'self' in different scenarios. For example, in your example code, what should happen if I do

testmap := {
  metadata: {
    name: "testapp",
    labels: {
      labela: "yxc",
      labelb: "qwe"
    }
  },
  spec: testmap.metadata.name
}

apple := testmap.spec

Does this mean apple should still reference testmap.metadata.name? What if testmap.metadata or test.metadata.name is mutated or deleted? Or, can I mutate testmap.metadata.name through apple?

In a different example (assuming we introduce self):

a := {
  b: "foo",
  c: self.b
}

d := {
  b: "bar",
  e: a
}

Is a.c still referencing a.b or should it reference d.b?

@iFrozenPhoenix
Copy link
Author

You have valid arguments. I think the simplest way would be to just copy the values, i.e. that references are not considered as generally in tengo lang.

That means in your first example apple has still the value of testmap.spec if one of the previous values alre deleted or mutated.
In my opinion to also introduce unmutatable refrences would really overcomplicate the things.

I.e. the reference should be just a copy operation of the original value.

I think this would also reflect the current behavior of the language.

@iFrozenPhoenix
Copy link
Author

@d5 @geseq I have made initial progress on this. But it is not that easy as I've thought...

In scanner it is hard to access the complete structure in an easy way. In vm it is too late as the compiler already has thrown a unresolvable reference.

https://github.com/iFrozenPhoenix/tengo/blob/3dd4cce41bbd6a97ee17cd3ce8718f20fc2177c1/parser/parser.go#L1067
https://github.com/iFrozenPhoenix/tengo/blob/3dd4cce41bbd6a97ee17cd3ce8718f20fc2177c1/parser/parser_self.go
I implemented it in parseMapLit. After the population of the elements slice I create a temporary flattened map, resolve the references and write the values in the original elements slice where the reference "self." is set.

May you could take a look?

It is working so far for simple references, but if the reference is an object key the refill of the original elements slice makes problems because the intermediate MapLit is missing.

working_map := {
  a: "vala",
  b: "valb",
  c: self.a,
  d: {
    nesteda: "valx"
  },
  e: self.d.nesteda
}

not_working_map := {
  a: "vala",
  b: "valb",
  c: self.a,
  d: {
    nesteda: "valx"
  },
  e: self.d (This doesn't get refilled)
}

Do you have an idea to work around this?

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

3 participants