Initialize AtomConstructor's fields via local vars

[hubertp]

Pull Request Description

The mechanism follows a similar approach to what is being in functions
with default arguments.
Additionally since InstantiateAtomNode wasn't a subtype of EnsoRootNode it
couldn't be used in the application, which was the primary reason for
issue #181449213.
Alternatively InstantiateAtomNode could have been enhanced to extend
EnsoRootNode rather than RootNode to carry scope info but the former
seemed simpler.

See test cases for previously crashing and invalid cases.

Important Notes

Checklist

Please include the following checklist in your PR:

• The documentation has been updated if necessary.
• All code conforms to the Scala, Java, and Rust style guides.
• All code has been tested:
  • Unit tests have been written where possible.
  • If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

[kustosz]

LGTM

[hubertp]

@kustosz can you have another look, please? I ended up partially reverting the previous solution to be able to carry information about the already evaluated arguments. It works but I'm not sure this is "the right way" to do it.

[kustosz]

This isn't right. Look at how default arguments are handled inside functions – there's a bunch of assignments of the form

<arg_name> = ReadArgumentNode(arg_pos)

and then anything that wants to get its hands on arg_name just uses the variable, not the argument. It's logically similar to what you've done, but it avoids by-name lookups and other such slow operations. In order for that to work, you'll have to fix the compiler, such that the variables in constructors get bound properly and are not dynamic symbol.

Also, please switch to develop and run sbt runtime/bench. It's our main benchmark suite for the interpreter and it's persistent – when you run it again you'll get a comparison, please make sure to run it on each PR. Just by looking at the changes in DynamicSymbolNode, I'm 99% percent sure some of these benchmarks are at least 10X slower with your changes (anything to do with method dispatch will suffer). My gut instinct is, when you implement this properly, there will be no noticeable performance impact.

[hubertp]

This is now out of the draft state and ready for the final review.

[kustosz]

This is much better, but I still don't like the stylistic choice here, InstantiateNode knows too much, even though there's no reason to change it all. If you just look at InstantiateNode before, what was it saying about the API? All it said was "look, I have a bunch of ExpressionNode children, I don't care what they are, but they are supposed to produce my field values". Now it's a mess of this node knowing about some 2 phases etc. And then this choice comes right back to bite you, when you have to write this:

    for (int i=0; i<args.length; i++) {
      assignments[i] = ConstantObjectNode.build(null);
      reads[i] = ReadArgumentNode.build(i, null);
    }

Clearly there's something wrong if you have to lie like this.

OK, so the alternative:

1. Don't change the structure of InstantiateNode.
2. When building it from code, generate the arg read/assign exprs separately. Then generate an instantiate node with var read children (instead of arg read). Then return a BlockNode of all assignments, returning the instantiate node. So, in pseudo code, the whole thing becomes:

do
  var0 = readArg(0)
  ...
  varN = readArg(N)
  return Instantiate(Constructor, var0, ..., varN)
end

which is much cleaner (I can't even write sensical pseudocode for the other solution).
3. Bonus: when there are no defaulted args (e.g. in builtin constructors), BUT ALSO, as an optimization (premature and probably useless, don't do it!) in actual code paths , the original solution of generating return Instantiate(Constructor, readArg(0), ..., readArg(N)) still works fine, without any nasty hacks.

[kustosz]

This is much better! Approval is conditional on:

1. Fixing the minor doc issue
2. Your confirmation of having run the benchmarks and there not being a slowdown :)

[hubertp]

Benchmarks passed locally. @4e6 @radeusgd @jdunkerley 👍 ?

[4e6]

Looks good! Just a comment about formatting.

[4e6]

please run scalafmt on this file