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: unused step and step_end results #800

Closed
wants to merge 1 commit into from
Closed

fix: unused step and step_end results #800

wants to merge 1 commit into from

Conversation

makcandrov
Copy link
Contributor

This PR fixes a bug introduced by #759:

  • The result of step is read but not written to the interpreter result. If the function returns something other than Continue, it will lead to an infinite loop in the interpreter.
  • The result of step_end is not being read at all, which means the execution cannot be halted by the inspector from this function.

@rakita
Copy link
Member

rakita commented Oct 16, 2023

Hey, I wanted to remove the return type from step and step_end as you can set that value directly inside Inspector interpreter.instruction_result = result.

Would be interested to get your feedback on that approach?

@makcandrov
Copy link
Contributor Author

Yes, I thought about it too - it definitely makes sense.

However, the current implementation is a confusing mix between the two and silently breaks some inspectors. That's why I suggested reverting to the original behavior.

If you want to take a look, I tried implementing the new behavior here (I wanted to open the PR in my fork it ended up in the main repo - sorry)

@rakita
Copy link
Member

rakita commented Oct 16, 2023

Yes, I thought about it too - it definitely makes sense.

However, the current implementation is a confusing mix between the two and silently breaks some inspectors. That's why I suggested reverting to the original behavior.

If you want to take a look, I tried implementing the new behavior here (I wanted to open the PR in my fork it ended up in the main repo - sorry)

Yeah, the current main approach is definitely a footgun.

I like this other PR more: #804 as it does not reuse the InstructionResult as returns (that felt bad in past, it should have been enum of some kind) and it is more straightforward on what changes inside aka interpreter.instruction_result.

@makcandrov

This comment was marked as outdated.

@makcandrov
Copy link
Contributor Author

Closing in favor of #804

@makcandrov makcandrov closed this Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants