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 a crash produced when a pattern contains a BoxExpression as an element. #626

Closed
wants to merge 5 commits into from

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Nov 16, 2022

This PR fixes a crash produced when a pattern contains a BoxExpression. Suppose for example that
we want to evaluate the expression

expr/.F[RowBox[list___]]->G[list] 

The pattern in the rule contains a RowBox, so in the evaluation, F[RowBox[list___]] is evaluated. Then, the RowBox[] expression is replaced by a RowBox object. Then, when Rule is evaluated, Pattern.create is called, with Expression(Symbol("Global`F"), RowBox(...)) as a parameter. It is not an Expression, so Patter.create fails raising an exception.

In this PR this behavior is fixed by catching this case and converting RowBox into an Expression before creating the pattern.

  • Fix Pattern.create in a way that handles input expr that are not Atom or Expression, but have a to_expression method.
  • Fix BoxExpression.sameQ to avoid infinite loops.
  • adds the pattern_sort parameter to the BuiltinExpression.get_sort_key method.

…om or Expression, but have a to_expression method.

* Fix BoxExpression.sameQ to avoid infinite loops.
* adds the pattern_sort parameter to the ``BuiltinExpression.get_sort_key`` method.
Copy link
Contributor

@TiagoCavalcante TiagoCavalcante left a comment

Choose a reason for hiding this comment

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

LGTM

@rocky
Copy link
Member

rocky commented Nov 28, 2022

LGTM but a couple of comments. First it would be good to add a an example involving

expr/.F[RowBox[list___]]->G[list] 

so that it is clearer what is getting fixed.

Second there are these places where we don't have a pattern_sort parameter. Will any of these be a problem?

mathics/builtin/patterns.py:1670:    def get_sort_key(self) -> tuple:
mathics/core/element.py:150:    def get_sort_key(self) -> list:
mathics/core/evaluation.py:160:    def get_sort_key(self) -> Tuple[bool, bool, str]:
mathics/core/rules.py:110:    def get_sort_key(self) -> tuple:

@mmatera
Copy link
Contributor Author

mmatera commented Aug 3, 2024

This seems to have been fixed in some other way, so let's close this.

@mmatera mmatera closed this Aug 3, 2024
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.

3 participants