Fail early on interpolation to self or to ancestor node

[Jasha10]

Using the interpolation ${} should be illegal because it necessarily causes cycles in the config object.

For example, the following config is pathological: {"x": "${}"}.

Based on a comment originally posted by @omry in #661 (comment)

[omry]

Okay, so now I get what ${} is.
It's an interpolation to the empty string, which we decided should represent the root node.
This is more of a feature. The intention is for it to be used inside custom resolvers to access the root:

a: 10
b: ${foo:${}}

We have since added the ability to access the config root when defining custom resolvers by passing _root_.
@odelalleau, do you remember the context for this? I wonder if we still need to support it now that custom resolvers takes _root_.

[odelalleau]

@odelalleau, do you remember the context for this? I wonder if we still need to support it now that custom resolvers takes _root_.

It looks like it was added as part of #597.
I don't feel strongly about it, but the new _root_ resolver argument doesn't fully replace ${} since someone might want to use the root config as argument for a resolver that may take any DictConfig as input. Something like:

a: 0
b: 1
c: 2
_meta:
  cfg_keys: ${oc.dict.keys:${}}

[omry]

Yes, there are cases where it can be useful. but I am not sure it's worth the added risk.

Since most of the badness begin when a node interpolation resolves to a parent of that node in general (not just in the case of the root node), we may just prevent such interpolations with an early error.

a:
  # absolute
  a: ${}
  b: ${a}
  c: ${a.c}
  # relative
  d: ${.}
  e: ${..}
  f: ${..a}
  g: ${..a.g}

If think it can be pretty straight forward to detect the condition when a interpolation is pointing to a node that is either the declaring node or a parent of it.
This will require undoing the support for ${}, but as the initial motivation it was fully addressed by introducing _root_ I am fine with removing that support.

Arguably, we can still support it if we do not perform the validation when resolving an interpolation inside a custom resolver, but I would lean toward simpler behavior until we see a really strong use case that would justify an exception.

[omry]

I took a quick look.
One approach to make this work is to perform interpolation validation during config creation and mutation.
Another is to detect it on access.
I experimented with both approaches a bit.

I got something basic that works, but it triggers on custom resolvers.
I don't think we should be calling custom resolvers on config initialization but right now I think there is no way to differentiate between custom resolvers and regular interpolations (this would potentially not be an issue if we perform the detection during interpolation resolution.

I got some initial success detecting that a node resolves to a parent of itself.
That approach is obviously also very partial.
it will not catch this:

a: ${b}
b: ${a}

(although I think we can detect it while resolving the interpolation).

[odelalleau]

I don't think we should be calling custom resolvers on config initialization but right now I think there is no way to differentiate between custom resolvers and regular interpolations (this would potentially not be an issue if we perform the detection during interpolation resolution.

It's also possible that:

• a resolver may return a parent node (ex: an upcoming oc.select)
• changes to some config options may impact the validity of arbitrary nodes

==> to me performing the dection during resolution makes more sense

Here's an example of the second case:

data:
  target: .a
  a: 0
  x: ${${.target}}

If you now set cfg.data.target = "data" then suddenly cfg.data.x points to its parent, but it would be really hard to detect it efficiently when this assignment is made.

[omry]

I think airtight detection will be extremely difficult wit custom resolvers and nested interpolations and I am okay with not detecting all cases. if we can catch the common cases it's already much better.

[odelalleau]

I think airtight detection will be extremely difficult wit custom resolvers and nested interpolations and I am okay with not detecting all cases. if we can catch the common cases it's already much better.

Ok -- the other potential issue I can see with doing it on creation / mutation and not on access is that a config may go through invalid states while doing a sequence of operations on it, and if we crash as soon as we detect a problem there may be false positives (if subsequent operations were meant to bring the config in a valid state). I'm not sure to which extent this could be a major issue though -- just flagging it as an FYI.