Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

Refactor cterm_execute #773

Merged
merged 16 commits into from
Dec 12, 2023
Merged

Refactor cterm_execute #773

merged 16 commits into from
Dec 12, 2023

Conversation

tothtamas28
Copy link
Collaborator

@tothtamas28 tothtamas28 commented Dec 11, 2023

  • Extract class CTermExecute to represent the return value
  • Assert some properties on the RPC response and simplify logic

@tothtamas28 tothtamas28 self-assigned this Dec 11, 2023
@tothtamas28 tothtamas28 marked this pull request as ready for review December 11, 2023 16:56
@ehildenb
Copy link
Member

ehildenb commented Dec 11, 2023

What is the purpose of this change?

cterm_execute currently loses a lot of information that the RPC call actually returns, re-inferring it based on things like the number of next states. If anything, we should be propagating more of that logic up, and dealing directly with the ExecuteResult that we already have:

class ExecuteResult(ABC): # noqa: B024

Part of the reason that we should deal with that result directly is because of changes like this, where we want to add more information to the returned response and be able to propagate that up to the KCFGExplore as well.

@tothtamas28
Copy link
Collaborator Author

What is the purpose of this change?

It's a clean-up:

  1. Replaces a five-tuple return value by a class
  2. Eliminates some unreachable branches and asserts the corresponding conditions (tested in evm-semantics and kontrol)

If anything, we should be propagating more of that logic up, and dealing directly with the ExecuteResult

I agree, the more complexity we can eliminate the better, we could replace the return type by a wrapper on ExecuteResult for example. But I don't see how this PR would prevent that, in fact I think we can be more confident in the correctness of such a change after this PR has been merged, as it replaces

if len(next_states) == 1 and len(next_states) < len(_next_states):
    return ...
elif len(next_states) == 1:
    if er.reason == StopReason.CUT_POINT_RULE:
        return ...
    else:
        next_states = []
return ...

by

return ...

@ehildenb
Copy link
Member

Sounds good, thank you for the explanation!

Copy link
Contributor

@geo2a geo2a left a comment

Choose a reason for hiding this comment

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

Great clean-up!

@rv-jenkins rv-jenkins merged commit f9a2da5 into master Dec 12, 2023
10 checks passed
@rv-jenkins rv-jenkins deleted the refactor-cterm-execute branch December 12, 2023 08:43
Baltoli pushed a commit to runtimeverification/k that referenced this pull request Apr 9, 2024
* Extract class `CTermExecute` to represent the return value
* Assert some properties on the RPC response and simplify logic

---------

Co-authored-by: devops <devops@runtimeverification.com>
Baltoli pushed a commit to runtimeverification/k that referenced this pull request Apr 9, 2024
* Extract class `CTermExecute` to represent the return value
* Assert some properties on the RPC response and simplify logic

---------

Co-authored-by: devops <devops@runtimeverification.com>
Baltoli pushed a commit to runtimeverification/k that referenced this pull request Apr 10, 2024
* Extract class `CTermExecute` to represent the return value
* Assert some properties on the RPC response and simplify logic

---------

Co-authored-by: devops <devops@runtimeverification.com>
Baltoli pushed a commit to runtimeverification/k that referenced this pull request Apr 10, 2024
* Extract class `CTermExecute` to represent the return value
* Assert some properties on the RPC response and simplify logic

---------

Co-authored-by: devops <devops@runtimeverification.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants