Automatically apply from conversion when runtime argument check fails

[JaroslavTulach]

Pull Request Description

Enhancing ReadArgumentNode to apply from conversions when the argument type check fails. For example there is no need to type 42.to_text when requiring a Text argument. The conversion will happen automatically.

Details

#6790 has introduced ability to opt-in and check type of arguments where needed (from a standard library perspective). These checks are slowly appearing in the Enso code base. They help us catch typing errors as soon as possible and report errors to users in a meaningful way. User can then change their code and apply necessary conversions themselves. However there is no need for such manual conversions!

We can automatically apply from conversion when the argument type doesn't fit the ascribed type and such a conversion is available in the module scope. We can do such conversions effectively - almost at no performance cost as benchmarks added by this PR demonstrate.

Non-goal

Changing the way conversions are imported into scope of a module isn't a goal for this PR. The way conversions are found remains unchanged by this PR. If a from conversion was found "manually" before, it is going to be found "automatically" now. Designing new ways for conversions to be located is out of scope of this PR. However as #7271 demonstrates, the current behavior may actually be good enough.

Future Work

These automatic conversions work great with runtime type classes. Implementing such runtime type classes with automatic conversions from any value is going to be possible when this PR is integrated.

It is (almost certainly) possible to add support for & operator without any performance penalty - e.g. one could write (a: Ord & Eq) and the type system would verify that the argument a supports all the required conversions - to Ord as well as to Eq. Implementation of this is out of scope of this PR.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

• The documentation has been updated, if necessary.
• All code follows the
  Scala,
  Java,
• All code has been tested:
  • Unit tests have been written where possible.
  • Ensure no regression in ListBenchmarks.mapOverList benchmark

[GregoryTravis]

I am concerned that this might result in hard-to-find bugs. If you write a bad method call that should fail, and it succeeds because of a conversion you weren't aware about, you'll get unexpected behavior that will be tricky to find, because it is made possible by a call to from that you did not explicitly make.

On the other hand, I am not familiar with the from conversions that exist in the code, so I don't know how much of a problem this will actually be in practice.

[xvcgreg]

@JaroslavTulach what is the status of this issue?

[JaroslavTulach]

@JaroslavTulach what is the status of this issue?

It is a draft.

[JaroslavTulach]

Running benchmarks to get official results for the new ListBenchmarks. The results are as expected:

        <case>
            <label>org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapAnyOverList</label>
            <scores>
                <score>147.31108088695652</score>
            </scores>
        </case>
        <case>
            <label>org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapConvOverList</label>
            <scores>
                <score>84287.3115472</score>
            </scores>
        </case>
        <case>
            <label>org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapIntegerOverList</label>
            <scores>
                <score>145.74264644395203</score>
            </scores>
        </case>
        <case>
            <label>org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapMultiOverList</label>
            <scores>
                <score>144.37380159563025</score>
            </scores>
        </case>
        <case>
            <label>org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapOverList</label>
            <scores>
                <score>141.27027240281691</score>
            </scores>
        </case>
        <case>
            <label>org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapVOverList</label>
            <scores>
                <score>141.1073415324603</score>
            </scores>
        </case>

all relevant benchmarks need 140-150ms per operation, but mapConvOverList which needs more than a minute (84287ms).

[JaroslavTulach]

The case with conversion shall now be faster since ec5a3b3 - running benchmarks to verify. mapConvOverList is now as fast as regular runs without any conversions:

        <case>
            <label>org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapConvOverList</label>
            <scores>
                <score>144.07712597041146</score>
            </scores>
        </case>

there seems to be a slowdown in plain

        <case>
            <label>org.enso.interpreter.bench.benchmarks.semantic.ListBenchmarks.mapOverList</label>
            <scores>
                <score>150.43897446282224</score>
            </scores>
        </case>

and it needs to be investigated.

[JaroslavTulach]

Benchmarking most recent changes at benchmark run.... existing benchmarks seem to be fine...

[wdanilo]

This is absolutely AMAZING! It also means that we are closer and closer to the .to method, right? Also, would it be possible to notify GUI where such conversion was applied? We would then display that using a special connection style (e.g. dashed one).

@GregoryTravis your concerns are very valid. Every implicit thing has the downside of code being more complex to trace. That's why it's important to visually indicate all such places - both on the graph and in the text editor. Anyway, such auto-conversions will help make the graph clean and fast to work with and I believe they can become one of the most powerful features of Enso.

[JaroslavTulach]

@GregoryTravis your concerns are very valid.
Every implicit thing has the downside of code being more complex to trace.
That's why it's important to visually indicate all such places ... on the graph

+1

Also, would it be possible to notify GUI where such conversion was applied?

Yes, we are able to trace when a conversion happens using GraalVM Insight (a toy I love). Imagine following convert.enso program:

from Standard.Base import all

meaning (v : Text) = "Meaning of world " + v

main = meaning 42

Text.from (that : Number) = that.to_text

which will do the automatic conversion when passing number 42 to the meaning function as it requires Text. You can run it with this PR as:

enso$ ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --run convert.enso 
'Meaning of world 42'

Let's now use the GraalVM's Insight capabilities by creating a tracing script to print out a message whenever a from function is invoked. Create trace.js:

insight.on('return', (ctx, frame) => {
    let that = frame.that;
    let to = ctx.returnValue(frame);
    print(`Converting via ${ctx.name} at ${ctx.source.name}:${ctx.line}`
       + ` from ${that}:${typeof that} to ${to}:${typeof to}`);
}, {
    rootNameFilter : '.*from',
    roots : true
});

and then execute with additional JAVA_OPTS parameters:

enso$ JAVA_OPTS=-Dpolyglot.insight=trace.js ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --run convert.enso 
Converting via convert::Standard.Base.Data.Text.Text::from at convert.enso:7 from 42:number to 42:string
'Meaning of world 42'

and we can see that the from conversion has been observed. As GraalVM Insight offers similar capabilities as Enso instrumentation I am pretty sure we can trace automatic conversions at runtime...

We would then display that using a special connection style (e.g. dashed one).

...we just need to design a way to provide such information to the IDE and then display it. That will need some future work, CCing @4e6.

That's why it's important to visually indicate all such places - both on the graph and in the text editor.

I am sure we can do it in the graph - e.g. when the program is running - easily. Alas, I don't know how to do the same in the text editor - e.g. just by static analysis.

[JaroslavTulach]

It also means that we are closer and closer to the .to method, right?

This PR proves there is no overhead when applying the conversions - e.g. that the .to method can be implemented effectively. On the other hand, this PR also removes the need to use the .to conversion in certain situations. Rather than writing in the previous example:

meaning (42.to Text)

one can simply call meaning 42 and benefit from the automatic conversion of ascribed runtime arguments.

[hubertp]

nits

[hubertp]

That's a bit unusual. IIRC for all nodes we assume that build will return the node, no exceptions?

[hubertp]

speaking of DataflowError logic being distributed ;)

[hubertp]

from can be null I think, but I don't think we handle that nicely in findConversion, do we?

[radeusgd]

We would then display that using a special connection style (e.g. dashed one).

...we just need to design a way to provide such information to the IDE and then display it. That will need some future work

Just please note that in many cases we put parameters directly on a node, and this conversion may also happen directly there and there will be no connection to stylize.

i.e. if we have

y = my_operation 42

instead of

x = 42
y = my_operation x

there is no connection between 42 and my_operation to display in the first case as the parameter 42 is directly in the node.

Maybe we should consider to then add some indication for these cases as well? Possibly some dashed frame over the parameter, which when hovered says which conversion has been applied?