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

TCO is not properly applied when function encounters a value with warnings attached #7093

Closed
radeusgd opened this issue Jun 20, 2023 · 3 comments · Fixed by #7116
Closed
Assignees
Labels
--bug Type: bug --regression Important: regression -compiler

Comments

@radeusgd
Copy link
Member

It is possible to trigger a stack-overflow in methods which rely on @Tail_Call to avoid that exact issue and work for larger datasets.

The Vector.contains can be tripped and trigger a stack overflow on a vector with as little as 1000 elements.

Run the following program:

from Standard.Base import all

main =
    v = Vector.new 1000 (i-> i+1)
    elem = Warning.attach "WARNING" 998
    IO.println (v.contains 998)
    IO.println (v.contains elem)

Expected behaviour

True
True

Actual behaviour

It fails with stack overflow:

True
Execution finished with an error: Resource exhausted: Stack overflow
        at <enso> go(Internal)
        at <enso> go<arg-2>(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:486-487)
...
        at <enso> go<arg-2>(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:486-487)
        at <enso> go(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:485-487)
        at <enso> Range.find_internal(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:488:5-24)
        at <enso> <anonymous>(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:338:22-60)
        at <enso> Range.check_start_valid<arg-2>(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:477:13-31)
        at <enso> Range.check_start_valid<arg-2>(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:476-477)
        at <enso> Range.check_start_valid(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:472-477)
        at <enso> Range.find(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:337-339)
        at <enso> Range.any<arg-0>(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:318:26-44)
        at <enso> Range.any<arg-0>(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:318:26-57)
        at <enso> Range.any(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Range.enso:318:26-63)
        at <enso> Vector.any(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Vector.enso:368:9-68)
        at <enso> Vector.contains(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Vector.enso:394:26-49)
        at <enso> stackoverflow1.main<arg-1>(stackoverflow1.enso:7:17-31)
        at <enso> stackoverflow1.main(stackoverflow1.enso:7:5-32)
radeusgd added a commit that referenced this issue Jun 20, 2023
@hubertp hubertp removed the triage label Jun 21, 2023
@hubertp hubertp moved this from ❓New to 📤 Backlog in Issues Board Jun 21, 2023
radeusgd added a commit that referenced this issue Jun 21, 2023
@hubertp hubertp moved this from 📤 Backlog to 🔧 Implementation in Issues Board Jun 22, 2023
@hubertp hubertp added the --regression Important: regression label Jun 22, 2023
@hubertp
Copy link
Collaborator

hubertp commented Jun 22, 2023

This has regressed in #6849. And I'm not surprised as I wasn't entirely convinced by that change.

radeusgd added a commit that referenced this issue Jun 23, 2023
@hubertp hubertp moved this from 🔧 Implementation to 👁️ Code review in Issues Board Jun 23, 2023
radeusgd added a commit that referenced this issue Jun 23, 2023
radeusgd added a commit that referenced this issue Jun 26, 2023
@mergify mergify bot closed this as completed in #7116 Jun 26, 2023
mergify bot pushed a commit that referenced this issue Jun 26, 2023
Partially revert #6849, which introduced a regression in TCO in the presence of warnings. Rather than modifying the tail call status, `TailCallException` now propagates the extracted warnings and appends them to the final result.

Closes #7093

# Important Notes
Compared to the previous attempt we don't pay the penalty of adding the warnings or even checking for them because it is being dealt in a separate specialization.
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Jun 26, 2023
radeusgd added a commit that referenced this issue Jun 26, 2023
@enso-bot
Copy link

enso-bot bot commented Jun 26, 2023

Hubert Plociniczak reports a new STANDUP for the provided date (2023-06-23):

Progress: Preparing a PR with a solution. Seems to cover the problem without any perf impact. Started looking into #7002. Reviewing changes to Any. It should be finished by 2023-06-23.

Next Day: Next day I will be working on the #7002 task. Provide a PR with a change.

radeusgd added a commit that referenced this issue Jun 27, 2023
@enso-bot
Copy link

enso-bot bot commented Jun 27, 2023

Hubert Plociniczak reports a new STANDUP for the provided date (2023-06-22):

Progress: Finding regression for #7098 (aka #6994). Investigating #7093 - I think I found an elegant solution with a negligible perf impact. It should be finished by 2023-06-23.

Next Day: Next day I will be working on the #7093 task. Provide a PR with a fix for #7093.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug --regression Important: regression -compiler
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants