Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print out warnings associated with local variables #10842

Merged
merged 30 commits into from
Aug 21, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Aug 17, 2024

Pull Request Description

Fixes #9749 by:

  • Adding fn option to enso-debug-server instrument - eb3b76e
  • Print warnings (if any) to stderr - 4fda04b
  • Improving output of :list to print out warnings - dbe3c45
  • Print errors to stderr - 1312546
  • Exiting on DataflowError - 2cc7ef5 and e6fbf73
  • Using all of that inside of runner/*Main - 7df58ef

The core of the change is in instrumentation that wraps the main method and at its end checks for warnings or errors among local variables. When an error is found, it wraps the original return value of main with a proxy that delegates to the original value, but also pretends to be exit exception with exit code 173. That one is detected in Main launcher to exit the process with exit code 173.

Important Notes

As a side-effect of this change, one can request an invocation of REPL at the end of any method just by providing a property to the VM:

$ enso --vm.D=polyglot.enso-debug-server.method-break-point=err_test.main --run err_test.enso --repl

stops at the end of main method of err_test.enso file.

Checklist

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

  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@JaroslavTulach
Copy link
Member Author

After 63d1560 one can create following warn_test.enso file:

from Standard.Base import all

main =
    j = 1
    d = Warning.attach "Doubled number" 2
    t = j + d
    v = [j, d, t]
    v

and then execute:

$ ./built-distribution/enso-engine-*/enso-*/bin/enso \
  --run warn_test.enso \
  -vm.D=polyglot.enso-debug-server.fn=warn_test.main \
  --repl
> :list
d = WithWarnings{2 has 1 warnings}
t = WithWarnings{3 has 1 warnings}
v = [1, 2, 3]
j = 1

e.g. by using the polyglot.enso-debug-server.fn one can trigger the standard REPL without having to specify Debug.breakpoint in the source code and some of the warnings are somehow displayed.

@JaroslavTulach
Copy link
Member Author

With dbe3c45 the output of the :list command for the previously shown warn_test.enso program is:

> :list
d = 2
  ! Doubled number
t = 3
  ! Doubled number
v = [1, 2, 3]
  ! Error 1 Doubled number
  ! Error 2 Doubled number
j = 1
> 

OK, @jdunkerley?

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Aug 19, 2024

The last improvement to startup we got was because of disabling the debug server by default:

needs --repl

this PR basically enables the same debug server instrument (but without the message transfer), so let's check what is the affect on startup with followin bench run. Probably there is some slowdown in _Startup_ benchmark - verifying with another run.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warnings printed upon :list look great, I think that will be quite helpful.

In the REPL, we also print the value returned from a function. Will the warnings show up there as well??? If not, could we please make them?

E.g.

> Warning.attach 2+2 3+3
>>> 6
    ! 4

That would be super helpful and would give immediate feedback for the warnings in REPL without having to check :list every time.


I'm a bit concerned about the changes to semantics of error propagation - I need to understand it further.

@JaroslavTulach
Copy link
Member Author

APIs being introduced by this PR:

  • exit code 173
  • property to select method to stop at
  • format of the printed (to stderr) out warnings and error

Changing these APIs will have a visible effect on users of Enso. Please review with care.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for addressing the comments

@radeusgd
Copy link
Member

In the REPL, we also print the value returned from a function. Will the warnings show up there as well??? If not, could we please make them?

E.g.

> Warning.attach 2+2 3+3
>>> 6
    ! 4

That would be super helpful and would give immediate feedback for the warnings in REPL without having to check :list every time.

Just checked and the warnings are not printed on REPL values. Any chance we could get that? Not necessarily as part of this PR. Shall I create a ticket?

@radeusgd
Copy link
Member

Just checked and the warnings are not printed on REPL values. Any chance we could get that? Not necessarily as part of this PR. Shall I create a ticket?

Ah, I guess the problem is that = actually return Nothing and not the assigned value, and the warning is associated with that value. So hmm, maybe it's more problematic than I thought. So maybe ignore my suggestion for now, as the REPL isn't a big priority anywhere.

@JaroslavTulach
Copy link
Member Author

In the REPL, we also print the value returned from a function. Will the warnings show up there as well??? If not, could we please make them?
E.g.

> Warning.attach 2+2 3+3
>>> 6
    ! 4

Just checked and the warnings are not printed on REPL values. Any chance we could get that? Not necessarily as part of this PR. Shall I create a ticket?

I am just too sloooow this afternoon. Done in 49d535b

Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks alright but please rename FN

engine/runner/src/main/java/org/enso/runner/Main.java Outdated Show resolved Hide resolved

mainFile = pkg.mainFile();
if (!mainFile.exists()) {
println("Main file does not exist.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this print to stderr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. However the code has been there before this PR - so the answer doesn't need to be provided by this PR.

engine/runner/src/main/java/org/enso/runner/Main.java Outdated Show resolved Hide resolved
@JaroslavTulach
Copy link
Member Author

Looks alright but please rename FN

Renamed to METHOD_BREAKPOINT.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Aug 20, 2024
@mergify mergify bot merged commit e5f865f into develop Aug 21, 2024
41 checks passed
@mergify mergify bot deleted the wip/jtulach/PrintWarningsOnExit branch August 21, 2024 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report Warning in the CLI Execution
4 participants