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

Fix Runtime.assert #8742

Merged
merged 6 commits into from
Jan 12, 2024
Merged

Fix Runtime.assert #8742

merged 6 commits into from
Jan 12, 2024

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Jan 11, 2024

Fixes #8741

Pull Request Description

Simplifies implementation of Runtime.assert.

Important Notes

  • Remove Runtime.assert_builtin builtin method, replace it with pure enso code and Runtime.assertions_enabled builtin.

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.

@Akirathan Akirathan self-assigned this Jan 11, 2024
@Akirathan Akirathan linked an issue Jan 11, 2024 that may be closed by this pull request
@Akirathan Akirathan added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Jan 11, 2024
@Akirathan
Copy link
Member Author

Without 798a882, the assertion error in

bar = assert (1 == 3) "Condition failed"
foo = bar
main = foo

is printed as:

Execution finished with an error: Assertion Error: 'Condition failed'
        at <enso> Panic.throw(Internal)
        at <enso> Runtime.assert<arg-2>(/home/pavel/dev/enso-2/built-distribution/enso-engine-0.0.1-dev-linux-amd64/enso-0.0.1-dev/lib/Standard/Base/0.0.1-dev/src/Runtime.enso:70-74)
        at <enso> Runtime.assert<arg-2>(/home/pavel/dev/enso-2/built-distribution/enso-engine-0.0.1-dev-linux-amd64/enso-0.0.1-dev/lib/Standard/Base/0.0.1-dev/src/Runtime.enso:69-74)
        at <enso> Runtime.assert(/home/pavel/dev/enso-2/built-distribution/enso-engine-0.0.1-dev-linux-amd64/enso-0.0.1-dev/lib/Standard/Base/0.0.1-dev/src/Runtime.enso:68-74)
        at <enso> tmp.bar(tmp.enso:12:5-38)
        at <enso> tmp.foo(tmp.enso:14:7-9)
        at <enso> tmp.main(tmp.enso:17:5-7)

With 798a882 the assertion error is printed as:

Execution finished with an error: Assertion Error: 'Condition failed'
        at <enso> tmp.bar(tmp.enso:12:5-38)
        at <enso> tmp.foo(tmp.enso:14:7-9)
        at <enso> tmp.main(tmp.enso:17:5-7)

As a user, I don't want to see internal Runtime.assert stack frames. I tried to create a special exception with dropped stack frames, but failed to do so. At the end, I have provided a special handling of Assertion_Error printing in 798a882. You are welcome to provide better suggestions.

@JaroslavTulach
Copy link
Member

There is InteropLibrary.getExceptionStackTrace method. You might find it useful. I used it in DataflowError speedup.

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.

I don't think such a very specific hack (including logic for just one specific type of exception) is a good idea at all just to remove 4 frames from a specific stack trace.

Is it really such a problem that the stack trace contains additional 4 frames? The assertions are an advanced feature anyway, a user advanced enough to inspect stack traces will quickly see which frames are relevant to their problem.

I think there may be other places as well where we throw the panic from a helper and so it contains frames of that helper. Is it really a problem?

Moreover the hack only applies in one place, ignoring all the other places where we are manipulating stack traces - so the printed stack trace will be inconsistent depending on where we get it from.

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

There is InteropLibrary.getExceptionStackTrace method. You might find it useful. I used it in DataflowError speedup.

@JaroslavTulach Unfortunately no. getExceptionStackTrace is an interop message, which is, unfortunatelly, not used by PolyglotException.getPolyglotStackTrace that we call in our runner's Main.scala

So let's just remove the ugly workaround, and live with a slightly longer stack traces for asserts.

@Akirathan Akirathan force-pushed the wip/akirathan/8741-fix-assert branch from e4d8af8 to 3fc7cc8 Compare January 12, 2024 12:14
@Akirathan
Copy link
Member Author

@radeusgd Just to be sure, I have looked into the Graal graphs by running enso --no-ir-caches --dump-graphs --run tmp.enso (and ensuring assertions are disabled) on:

from Standard.Base import all
import Standard.Base.Runtime.State
from Standard.Base.Runtime import assert

main =
    res = 1.up_to 10000 . map i->
        assert (i == i)
    res.first

And I can confirm that Truffle+Graal compiles assert into just 4 LoadField instructions, which is what I have expected.

Akirathan added a commit that referenced this pull request Jan 12, 2024
@Akirathan Akirathan merged commit 6ae35ab into develop Jan 12, 2024
28 checks passed
@Akirathan Akirathan deleted the wip/akirathan/8741-fix-assert branch January 12, 2024 17:47
@Akirathan
Copy link
Member Author

It appears that I have accidentally used the assert wrongly, so the bug report #8741 might be invalid. But as this PR simplifies the implementation of Runtime.assert and I have even made sure that Runtime.assert is, in fact, a nop in production after PE (See my previous comment), I am merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime.assert does not work when called from lazy atom field
7 participants