Print values even if their `to_text` returns a non-text method, to aid with debugging. #6154

radeusgd · 2023-03-30T16:03:55Z

Pull Request Description

Important Notes

Checklist

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

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

radeusgd · 2023-03-30T16:07:01Z

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/io/PrintlnNode.java

-      @Cached("buildInvokeCallableNode()") InvokeCallableNode invokeCallableNode,
-      @Cached ExpectStringNode expectStringNode) {
+      @Cached("buildInvokeCallableNode()") InvokeCallableNode invokeCallableNode) {
    var str = invokeCallableNode.execute(symbol, frame, state, new Object[] {message});
    EnsoContext ctx = EnsoContext.get(this);
-    print(ctx.getOut(), expectStringNode.execute(str));
+    print(ctx.getOut(), str);


The ExpectStringNode was also dealing with stripping warnings from this value.

Now, if I understand it correctly, the warnings will be printed if present. Although tbh I did not test it yet - probably worth doing first.

Still, I know there were different opinions on this - are we ok with printing warnings, do we want to keep dropping them or do we want to print them to stderr?

cc: @jdunkerley @JaroslavTulach

I like printing them like currently, we can always clear them if we do not want them in the output.

I'm not a fan of the stderr idea, because it may be unexpected (we should have a separate method to print to stderr) and it may be hard to keep track which warnings were associated with which value if many values are printed.

Maybe the best solution will be to duplicate println into a IO.debug_print node?

Then we could have println keep the existing behaviour, always expecting text returned from to_text and failing with type error if that invariant is broken. And debug_print would be more lenient about types and also would print warnings?

Based on Rust print vs. debug print:

Maybe the best solution will be to duplicate println into a IO.debug_print node?

IO.println warnings=True "Hi with warnings"

I think having the control to print warnings would be good - my feeling is we should print the value without warnings by default ad then have the option to show warnings.

This would be consistent with our visualizations in the IDE as well.

Ok, that sounds good. I guess we need to add an IO.println_builtin to be able to add a default argument (since a builtin cannot directly have default arguments, right? cc: @JaroslavTulach or was this limitation lifted?) and I will make warnings=False by default.

I'll update this PR once I have some more free time, probably after I finish the full join PR.

we need to add an IO.println_builtin to be able to add a default argument (since a builtin cannot directly have default arguments, right? cc: @JaroslavTulach or was this limitation lifted?)

Yes, separate builtin method is needed. The limitation is still there:

Builtin methods should support default parameters #5312

JaroslavTulach

We certainly want to print something every time for every value and avoid throwing errors.

JaroslavTulach · 2023-03-31T05:20:39Z

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/io/PrintlnNode.java

-      @Cached("buildInvokeCallableNode()") InvokeCallableNode invokeCallableNode,
-      @Cached ExpectStringNode expectStringNode) {
+      @Cached("buildInvokeCallableNode()") InvokeCallableNode invokeCallableNode) {
    var str = invokeCallableNode.execute(symbol, frame, state, new Object[] {message});
    EnsoContext ctx = EnsoContext.get(this);
-    print(ctx.getOut(), expectStringNode.execute(str));
+    print(ctx.getOut(), str);


Based on Rust print vs. debug print:

Maybe the best solution will be to duplicate println into a IO.debug_print node?

IO.println warnings=True "Hi with warnings"

jdunkerley · 2023-04-05T15:48:24Z

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/io/PrintlnNode.java

-      @Cached("buildInvokeCallableNode()") InvokeCallableNode invokeCallableNode,
-      @Cached ExpectStringNode expectStringNode) {
+      @Cached("buildInvokeCallableNode()") InvokeCallableNode invokeCallableNode) {
    var str = invokeCallableNode.execute(symbol, frame, state, new Object[] {message});
    EnsoContext ctx = EnsoContext.get(this);
-    print(ctx.getOut(), expectStringNode.execute(str));
+    print(ctx.getOut(), str);


I think having the control to print warnings would be good - my feeling is we should print the value without warnings by default ad then have the option to show warnings.

This would be consistent with our visualizations in the IDE as well.

…debugging easier. Also print if warnings are attached.

radeusgd · 2023-04-07T09:19:20Z

I got piled up in a bit of a rabbit hole trying to add a remove_warnings default implementation.

Regardless - the handling of warnings makes the prinltn node, a rather primitive operation pretty complicated, as you can see by now having 4 specializations.

I'm not sure what format do we expect either? I tried to do <Value> (<n> warnings attached) if n > 0, but I'm not sure if that's what we want. IMO it made sense to display the warnings if it were to be turned by default - as a cautionary safety check for not having them (similarly like our write operations would not work if there are warnings attached by default, but here of course not printing is not the good solution, so printing the warnings IMO was).

If we add the warnings as a non-default, I don't think it makes as much sense and we can always implement it as something like following in Enso stdlib:

print_with_warnings obj =
    count = Warning.get_all obj . length
    IO.println obj
	if count > 0 then IO.println "("+count.to_text+" warnings attached)"

If you think the warnings should be handled by the builtin, I can reopen this, but IMO it's not as needed.

Instead I opened a PR that just handles the fix for #5961:
#6223

radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 30, 2023

radeusgd requested a review from 4e6 as a code owner March 30, 2023 16:03

radeusgd self-assigned this Mar 30, 2023

radeusgd requested review from JaroslavTulach and hubertp as code owners March 30, 2023 16:03

radeusgd commented Mar 30, 2023

View reviewed changes

JaroslavTulach approved these changes Mar 31, 2023

View reviewed changes

JaroslavTulach linked an issue Mar 31, 2023 that may be closed by this pull request

IO.println: Type error: expected str to be Text, but got Function #5961

Closed

radeusgd force-pushed the wip/radeusgd/print-problematic-to-text branch from 8e2b8c9 to a25523e Compare March 31, 2023 16:21

enso-bot bot mentioned this pull request Apr 1, 2023

21 compiler tests that relied on the old parser disabled #5894

Closed

jdunkerley approved these changes Apr 5, 2023

View reviewed changes

radeusgd added 3 commits April 6, 2023 20:42

Print values even if their to_text returns a non-text method, making …

3c2fbd2

…debugging easier. Also print if warnings are attached.

Add warnings switch

c3ded1f

"fixes"

fddb05c

radeusgd force-pushed the wip/radeusgd/print-problematic-to-text branch from a25523e to fddb05c Compare April 6, 2023 23:31

radeusgd closed this Apr 7, 2023

radeusgd deleted the wip/radeusgd/print-problematic-to-text branch December 7, 2023 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print values even if their `to_text` returns a non-text method, to aid with debugging. #6154

Print values even if their `to_text` returns a non-text method, to aid with debugging. #6154

radeusgd commented Mar 30, 2023

radeusgd Mar 30, 2023

JaroslavTulach Mar 31, 2023

jdunkerley Apr 5, 2023

radeusgd Apr 5, 2023

JaroslavTulach Apr 6, 2023

JaroslavTulach left a comment

JaroslavTulach Mar 31, 2023

jdunkerley Apr 5, 2023

radeusgd commented Apr 7, 2023

Print values even if their to_text returns a non-text method, to aid with debugging. #6154

Print values even if their to_text returns a non-text method, to aid with debugging. #6154

Conversation

radeusgd commented Mar 30, 2023

Pull Request Description

Important Notes

Checklist

radeusgd Mar 30, 2023

Choose a reason for hiding this comment

JaroslavTulach Mar 31, 2023

Choose a reason for hiding this comment

jdunkerley Apr 5, 2023

Choose a reason for hiding this comment

radeusgd Apr 5, 2023

Choose a reason for hiding this comment

JaroslavTulach Apr 6, 2023

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Mar 31, 2023

Choose a reason for hiding this comment

jdunkerley Apr 5, 2023

Choose a reason for hiding this comment

radeusgd commented Apr 7, 2023

Print values even if their `to_text` returns a non-text method, to aid with debugging. #6154

Print values even if their `to_text` returns a non-text method, to aid with debugging. #6154