Do lazy evaluation in list/map index defaults

[lslezak]

The defaults for accessing YCPList and YCPMap values need to evaluated only when the requested index/key is missing.

If a function with side effect is used for returning the default then the Ruby code behaves differently to the original YCP.

YCP Example:

{
  boolean flag = false;

  define string Foo()
  {
    flag = true;
    return "foo";
  }

  map<string,string> m = $["a" : "b"];

  string str = m["a"]:Foo();
  y2internal("flag: %1", flag);

  str = m["missing"]:Foo();
  y2internal("flag: %1", flag);
}

This logs:

flag: false
flag: true

The converted ruby code contains this

 @str = Ops.index(@m, ["a"], Foo())

which causes the Foo() function evaluation before accessing the map and thus logs:

flag: true
flag: true

This breaks testsuite in yast2 in PortAliases module, see lines https://github.com/yast/yast-yast2/blob/master/library/network/src/PortAliases.ycp#L216 and https://github.com/yast/yast-yast2/blob/master/library/network/src/PortAliases.ycp#L167 where the default handler function modifies the accessed map (yeah!).

[lslezak]

I have added a workaround in yast/ycp-killer#461, it should be removed after fixing this issue.

[dmajda]

@jreidinger Could you make Ops.index to accept an additional way to specify the default value — via a block? The block would be called only when the default is really needed (the looked-up element is missing). If no block is passed, the 3^rd argument would be used as default, as it is now.

With that implemented, I could translate this code:

string str = m["a"]:Foo();

as:

@str = Ops.index(@m, ["a"]) { Foo() }

This would ensure lazy evaluation of the default value. Translation of default values that are not functions would stay the same (they would be passed as the 3^rd argument).

[jreidinger]

yes, WIP, not sure if I finish it today

[jreidinger]

Implemented - yast/yast-ruby-bindings@0dfcb4d

[dmajda]

One note to the fix: It only fixes the case when the default value is a call. All other cases are still evaluated eagerly. This means that e.g. complex default values including calls (e.g. a list or map) may still behave incorrectly.

I don't look if such values are even allowed in YCP, and how they behave if so. Moreover, I didn't see any such default value in the code so far. As a result, I think this case is most likely irrelevant.

[mvidner]

Sorry, that argument is wrong. Yes, the majority of cases are simple values, but fixing only one specific case of evaluable values can result in subtle, well hidden bugs. It should be safe and equally simple to replace "use block if default is a call" with "use block if default is not a literal".

To clarify, $[] is a literal, $["foo": "bar"] is still a literal, and $["foo": Bar()] is not. But it would be OK to treat only empty maps as "simple" defaults.

[dmajda]

@mvidner I added lazy evaluation for non-empty lists, maps and terms. It seems a reasonable compromise between generating too many blocks and too complex code. We can improve this further later if we'll have time.

BTW, this change added 145 blocks to the translated YaST code, most of them containing UI terms (e.g. VSpacing(1)). This seems acceptable to me.

[dmajda]

BTW, this change added 145 blocks to the translated YaST code, most of them containing UI terms (e.g. VSpacing(1)).

Correction: There were 145 + lines in the diff. That doesn't mean 145 blocks, because some added blocks were multi-line.