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

Reverse pyk match operator #2599

Merged
merged 5 commits into from
May 17, 2022
Merged

Reverse pyk match operator #2599

merged 5 commits into from
May 17, 2022

Conversation

ehildenb
Copy link
Member

I was trying to explain the algorithm behind summarization to @xc93 and realized that on this line:

https://github.com/runtimeverification/erc20-verification/blob/master/ksummarize/src/ksummarize/kevm_summarize.py#L211

we are saying term.match(pattern). I would much prefer for us to say pattern.match(term), it matches the same direction as match(pattern, term) that I'm used to for a long time and makes more sense in my head. I can't provide a better justification for it than this, but I did confirm that this is the only use that needs to change.

@ehildenb ehildenb requested a review from tothtamas28 May 17, 2022 00:33
@ehildenb ehildenb self-assigned this May 17, 2022
@ehildenb
Copy link
Member Author

ehildenb commented May 17, 2022

Also, pattern.match(term) matches better the python way of defining match as:

class CTerm:
    def match(self, other):
        return match(self.config, other.config)

Copy link
Contributor

@tothtamas28 tothtamas28 left a comment

Choose a reason for hiding this comment

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

A previous discussion we had on this topic: #2470 (comment).

To sum up. my take on the issue is the following.

  1. Both "pattern matches term" and "term matches pattern" are in use.
  2. Both map(patterm.match, iterable) and map(term.match, iterable) have natural use cases.
  3. term.match(pattern) is not None corresponds to "the set of models of term is a subset of the set of models of pattern". Notice that the order of operands is the same in both the code and its logical interpretation. This is also the direction we draw our abstraction edges in KCFG.

But I do not have a strong opinion on this one, so I'm fine with pattern.match(term).

pyk/src/pyk/tests/test_match.py Outdated Show resolved Hide resolved
pyk/src/pyk/cterm.py Outdated Show resolved Hide resolved
pyk/src/pyk/cterm.py Outdated Show resolved Hide resolved
@ehildenb
Copy link
Member Author

Yeah, I remember being unsure what the correct direction was before, but I think the fact that I spent an hour being baffled at how the summarization algorithm was working was a strong enough indicator of what I naturally expected while working with the library.

@ehildenb ehildenb requested a review from tothtamas28 May 17, 2022 14:09
@rv-jenkins rv-jenkins merged commit cdee73c into master May 17, 2022
@rv-jenkins rv-jenkins deleted the reverse-pyk-match branch May 17, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants