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

Callstack not always being cleared on function exit #92

Closed
faultyserver opened this issue Dec 23, 2017 · 3 comments
Closed

Callstack not always being cleared on function exit #92

faultyserver opened this issue Dec 23, 2017 · 3 comments
Labels
bug.interpreter A bug specifically relating to the interpreter source code bug Generic label for bugs. Every bug should have this tag in addition to the more specific bug tag good first issue An issue that provides a good intro to working with the Myst codebase. Be helpful!
Milestone

Comments

@faultyserver
Copy link
Member

faultyserver commented Dec 23, 2017

This issue has been marked as a "Good First Issue"! If you'd like to tackle this issue as your first contribution to the project, be sure to read the Get Involved section of the README for some help with how to get started.

While working on the Spec library, I encountered an interesting error:

Uncaught Exception: No variable or method `start` for Kernel
  from `start` at /Users/jon/Sites/myst-lang/myst/spec/myst/spec.mt:22:53
  from `-` at /Users/jon/Sites/myst-lang/myst/spec/myst/spec.mt:22:46
  from `puts` at /Users/jon/Sites/myst-lang/myst/spec/myst/spec.mt:22:4
  from `block` at /Users/jon/Sites/myst-lang/myst/stdlib/spec.mt:41:5
  from `describe` at /Users/jon/Sites/myst-lang/myst/spec/myst/time_spec.mt:62:1
  from `run` at /Users/jon/Sites/myst-lang/myst/stdlib/spec.mt:28:10
  from `it` at /Users/jon/Sites/myst-lang/myst/spec/myst/integer_spec.mt:61:3
  from `block` at /Users/jon/Sites/myst-lang/myst/stdlib/spec.mt:28:15
  from `block` at /Users/jon/Sites/myst-lang/myst/stdlib/spec/single_spec.mt:11:7
  from `run` at /Users/jon/Sites/myst-lang/myst/stdlib/spec.mt:28:10
  from `it` at /Users/jon/Sites/myst-lang/myst/spec/myst/integer_spec.mt:56:3
  from `block` at /Users/jon/Sites/myst-lang/myst/stdlib/spec.mt:28:15
  from `block` at /Users/jon/Sites/myst-lang/myst/stdlib/spec/single_spec.mt:11:7
  from `run` at /Users/jon/Sites/myst-lang/myst/stdlib/spec.mt:28:10
  from `it` at /Users/jon/Sites/myst-lang/myst/spec/myst/integer_spec.mt:51:3
  from `run` at /Users/jon/Sites/myst-lang/myst/stdlib/spec.mt:28:10
  from `it` at /Users/jon/Sites/myst-lang/myst/spec/myst/integer_spec.mt:47:3
  from `run` at /Users/jon/Sites/myst-lang/myst/stdlib/spec.mt:28:10
  from `it` at /Users/jon/Sites/myst-lang/myst/spec/myst/integer_spec.mt:43:3
  from `block` at /Users/jon/Sites/myst-lang/myst/stdlib/spec.mt:41:5
  from `describe` at /Users/jon/Sites/myst-lang/myst/spec/myst/integer_spec.mt:3:1

At a glance, it looks fairly simple, but looking at where the error came from shows an issue: the line of code causing the error is at the top-level scope. The error comes from the last line here with IO.puts:

start = Time.now

# TODO: add Dir globbing to automatically detect and require all `*_spec.mt`
# files under this directory.
require "./enumerable_spec.mt"
require "./integer_spec.mt"
require "./list_spec.mt"
require "./map_spec.mt"
require "./string_spec.mt"
require "./unary_ops/not_spec.mt"
require "./type_spec.mt"
require "./time_spec.mt"

finish = Time.now

# The only way to reach this point is if all of the Specs passed. Any failures
# will immediately exit the program, so reaching here implies success.
IO.puts("\nAll in-language specs passed in <(finish-start)> seconds.")

Interestingly, this actually shows two errors with the callstack management:

  1. A function being called is pushed onto the stack before it's arguments are evaluated. This is why puts is the third entry in the list, when the error is actually coming from its argument.
  2. When a function exits, it may not be removed from the callstack.

A similar issue with the selfstack was addressed previously (see #65), but this seems mostly unrelated.

@faultyserver faultyserver added bug Generic label for bugs. Every bug should have this tag in addition to the more specific bug tag bug.interpreter A bug specifically relating to the interpreter source code labels Dec 23, 2017
@faultyserver
Copy link
Member Author

If someone is interested in investigating and potentially resolving this issue, the callstack is managed using the @callstack property of the Interpreter class.

Entries are pushed to the callstack with @callstack.push. Some occurrences of this are in util.cr and call.cr.

Whenever a value is pushed to the callstack, it should have a corresponding place where that value would be _popped. For Calls, this is just a few lines down, but it doesn't seem like other instances of @callstack.push have corresponding pops. That might be a good place to start looking.


Remember to add specs for any changes you make so we can be sure this bug doesn't come back in the future! There actually aren't any specs around the Callstack yet, so that would definitely be a good thing to start! That file should live under spec/interpreter/callstack_spec.cr.

If you have any questions about this issue, feel free to ask either in a comment here, or in the #help channel in our community discord server, or message me directly (faulty#7958 on discord). Good luck!

@faultyserver faultyserver added the good first issue An issue that provides a good intro to working with the Myst codebase. Be helpful! label Dec 23, 2017
@faultyserver faultyserver added this to the Next milestone Dec 28, 2017
@Jens0512
Copy link
Member

Hello to those who did not already see me in Discord. I want to take care of this one. Please leave it to me 😃

@faultyserver
Copy link
Member Author

@bmulvihill bisected this issue, and it seems like it was indirectly resolved by #95.

I'm going to close this for now, but tag with revisit in case it comes up again. Thanks :)

@faultyserver faultyserver added the revisit A tag to revisit this issue sometime in the future. label Jan 12, 2018
@faultyserver faultyserver removed this from the Next milestone Jan 12, 2018
@faultyserver faultyserver added this to the v0.4.0 milestone Feb 3, 2018
@faultyserver faultyserver removed the revisit A tag to revisit this issue sometime in the future. label Feb 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug.interpreter A bug specifically relating to the interpreter source code bug Generic label for bugs. Every bug should have this tag in addition to the more specific bug tag good first issue An issue that provides a good intro to working with the Myst codebase. Be helpful!
Projects
None yet
Development

No branches or pull requests

2 participants