Opened 7 months ago

Closed 6 months ago

#30928 closed defect (fixed)

Backslash line continuation broken when defining a callable symbolic expression

Reported by: egourgoulhon Owned by:
Priority: critical Milestone: sage-9.3
Component: symbolics Keywords: callable symbolic expression, backslash, preparser
Cc: gh-jcamp0x2a, slelievre Merged in:
Authors: Joshua Campbell Reviewers: Steven Trogdon, Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 64c5757 (Commits, GitHub, GitLab) Commit: 64c5757b4fb67d492f0c64cf7d3bda2d3616b56c
Dependencies: Stopgaps:

Status badges

Description

As reported in https://groups.google.com/g/sage-devel/c/XD1VtG0TOEk/m/Lo6L8YBGCgAJ, we have currently (Sage 9.2 and 9.3.beta1):

sage: f(x) = x \ 
....:        + 1                                                                                    
  File "<ipython-input-1-a5c1e14e19da>", line 1
    __tmp__=var("x"); f = symbolic_expression(x  * BackslashOperator() * ).function(x)
                                                                         ^
SyntaxError: invalid syntax

There was no such issue with Sage 9.1 and lower.

Change History (31)

comment:1 Changed 7 months ago by egourgoulhon

The issue is already there in Sage 9.2.beta14.

comment:2 Changed 7 months ago by egourgoulhon

FWIW, in Sage 9.3.beta1, we have

sage: preparse(r"f(x) = x \ + 1")                                                                  
'__tmp__=var("x"); f = symbolic_expression(x  * BackslashOperator() * + Integer(1)).function(x)'

which is correct, while in the reported issue the second line is entirely missing after BackslahOperator() *, which triggers the syntax error.

comment:3 Changed 7 months ago by egourgoulhon

The bug could arise from the upgrade to ipython 7 performed in #28197 (merged in Sage 9.2.beta8) or from the follow-up ticket #30417 (merged in Sage 9.2.beta11).

comment:4 Changed 7 months ago by strogdon

I may be off base, but the following may be important

sage: from sage.repl.interpreter import get_test_shell                                                                                                                                               
sage: shell = get_test_shell()                                                                                                                                                                       
sage: shell.run_cell('f(x) = x \ + 1')                                                                                                                                                               
<input>:1: DeprecationWarning: invalid escape sequence \ 
<>:1: DeprecationWarning: invalid escape sequence \ 
<ipython-input-19-e03ae3fa6f44>:1: DeprecationWarning: invalid escape sequence \ 
  shell.run_cell('f(x) = x \ + 1')
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-19-81598207d80e> in <module>
----> 1 __tmp__=var("x"); f = symbolic_expression(x  * BackslashOperator() * + Integer(1)).function(x)

/local/sage-git/sage/local/lib/python3.8/site-packages/sage/misc/misc.py in __mul__(self, right)
    829             (0.0, 0.5, 1.0, 1.5, 2.0)
    830         """
--> 831         return self.left._backslash_(right)
    832 
    833 

/local/sage-git/sage/local/lib/python3.8/site-packages/sage/structure/element.pyx in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4701)()
    491             AttributeError: 'LeftZeroSemigroup_with_category.element_class' object has no attribute 'blah_blah'
    492         """
--> 493         return self.getattr_from_category(name)
    494 
    495     cdef getattr_from_category(self, name):

/local/sage-git/sage/local/lib/python3.8/site-packages/sage/structure/element.pyx in sage.structure.element.Element.getattr_from_category (build/cythonized/sage/structure/element.c:4813)()
    504         else:
    505             cls = P._abstract_element_class
--> 506         return getattr_from_other_class(self, cls, name)
    507 
    508     def __dir__(self):

/local/sage-git/sage/local/lib/python3.8/site-packages/sage/cpython/getattr.pyx in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:2620)()
    370         dummy_error_message.cls = type(self)
    371         dummy_error_message.name = name
--> 372         raise AttributeError(dummy_error_message)
    373     attribute = <object>attr
    374     # Check for a descriptor (__get__ in Python)

AttributeError: 'sage.symbolic.expression.Expression' object has no attribute '_backslash_'
<ExecutionResult object at 7f213d0cd130, execution_count=None error_before_exec=None error_in_exec='sage.symbolic.expression.Expression' object has no attribute '_backslash_' info=<ExecutionInfo object at 7f213d09fc40, raw_cell="f(x) = x \ + 1" store_history=False silent=False shell_futures=True> result=None>

comment:5 follow-up: Changed 7 months ago by strogdon

I get the same if f(x) = x \ + 1 is typed at the sage prompt. So probably not the correct thing to try.

comment:6 in reply to: ↑ 5 Changed 7 months ago by egourgoulhon

Replying to strogdon:

I get the same if f(x) = x \ + 1 is typed at the sage prompt. So probably not the correct thing to try.

Yes probably; moreover shell.run_cell(r'f(x) = x \ + 1') yields the same AttributeError in Sage 9.1, where everything is fine with backslash line continuation.

comment:7 Changed 7 months ago by strogdon

Some data points. I think sage/repl/preparser.py and perhaps sage/repl/interpreter.py need significant changes. Adding print statements as

  • src/sage/repl/interpreter.py

    diff --git a/src/sage/repl/interpreter.py b/src/sage/repl/interpreter.py
    index b70345f940..206041e4a1 100644
    a b def SagePreparseTransformer(lines): 
    444444        sage: # instead of ["'''", 'abc-Integer(1)-Integer(2)', "'''"]
    445445
    446446    """
     447    print('we are now in sage.repl.interpreter.SagePreparseTransformer')
    447448    lines_out = []
    448449    reset = True
    449450    for line in lines:
  • src/sage/repl/preparse.py

    diff --git a/src/sage/repl/preparse.py b/src/sage/repl/preparse.py
    index 3619fd9e54..2073caa855 100644
    a b def preparse(line, reset=True, do_time=False, ignore_prompts=False, 
    17451745        quote_state = None
    17461746
    17471747    L = line.lstrip()
     1748    print('result of L = line.lstrip() in sage.repl.preparse')
     1749    print(L)
    17481750    if len(L) > 0 and L[0] in ['#', '!']:
    17491751        return line

I see:

sage: f(x) = x \ 
....:        + 1                                                                
we are now in sage.repl.interpreter.SagePreparseTransformer
result of L = line.lstrip() in sage.repl.preparse
f(x) = x \

result of L = line.lstrip() in sage.repl.preparse
+ 1

we are now in sage.repl.interpreter.SagePreparseTransformer
result of L = line.lstrip() in sage.repl.preparse
f(x) = x \

result of L = line.lstrip() in sage.repl.preparse
+ 1

  File "<ipython-input-1-a5c1e14e19da>", line 1
    __tmp__=var("x"); f = symbolic_expression(x  * BackslashOperator() * ).function(x)
                                                                         ^
SyntaxError: invalid syntax

Note that preparse is called from interpreter and it is called twice! I don't believe it should be called twice. Furthermore preparse treats the input as two separate lines instead of one line separated by the \ continuation character. This results in \ being interpreted as the * BackslashOperator() *. I'm unable to test for a properly-functioning Sage.

comment:8 Changed 7 months ago by slelievre

  • Cc gh-jcamp0x2a slelievre added

comment:9 Changed 7 months ago by gh-jcamp0x2a

I think this may have been introduced by the transition to IPython 7 and how its new input transformer API deals with lines. For example, commenting out the backslash substitution altogether in preparse.py still yields:

sage: f(x) = x \
....:        + 1
  File "<ipython-input-1-bc21d2198ee4>", line 1
    __tmp__=var("x"); f = symbolic_expression(x \).function(x)
                                                              ^
SyntaxError: unexpected character after line continuation character

In the old IPython input transformer docs, it appears there were three types of transformer: physical line, logical line, and python line. In the same docs for IPython 7, there now only appears to be two: cleanup and post.

Judging by the change to ipython_extension.py for #28197, it looks like the SagePromptTransformer was moved from "physical line" to "cleanup" and SagePreparseTransformer from "python line" to "post".

I'm wondering if that "post" is not quite the same as the old "python line" however and functions more like the old "logical line". That could explain why #30417 that I fixed arose in the first place.

I have a couple ideas for how to patch this in the short term, but neither is likely to be particularly elegant. I'll play around with them and see if I can get something pushed in the next couple days.

I agree with @strogdon that some significant changes/cleanup are needed in the preparser and interpreter in order to fix this properly. I know there was talk in #28974 about building a proper grammar and parser for Sage to remove the need for all these regex kludges. Would be a large undertaking though.

comment:10 Changed 7 months ago by gh-jcamp0x2a

  • Authors set to Joshua Campbell
  • Branch set to u/gh-jcamp0x2a/30928-backslash-continuation
  • Commit set to 68afcdb5b7ebf42e1a5c35e894759978d1bb1391
  • Status changed from new to needs_review

I've pushed a potential fix.

  • preparse removes backslash+newline combinations, putting such constructions onto the same line. This happens after strip_string_literals so we don't have to worry about backslash continuations inside strings, and it happens before preparse_calculus so that it can surround the entire thing with a symbolic_expression call.
  • For the IPython use case, I've just squashed the list of lines into a single string and passed that to preparse instead of going line-by-line. The prompts should've already been removed by the SagePromptTransformer so I think that's a safe operation. Also, since IPython has already handled the % magic commands immediately prior to our SagePreparseTransformer, I removed the explicit check for them.
  • I've done something similar for the preparse_file use case with the complication of supporting load and attach statements. Basically, just calls preparse on batches between such statements and between them and the beginning/end of file.

As predicted: not the most elegant fix, and there's a fair chance I broke another edge case somewhere in the process. I'm not sure how else to fix it without tearing up a bunch of the existing preparser code. Input most welcome! :)


New commits:

68afcdbFix backslash continuation for symbolic expressions

comment:11 Changed 7 months ago by strogdon

Initial testing indicates that line continuation is now working. However, regardless the input after the sage: prompt, that input is parsed twice. Perhaps this is a separate issue or maybe not an issue?

Example:

sage: f(x) = x \ 
....:        + 1 \ 
....:        + x^2                                                                                                                                                                                   
we are now in sage.repl.interpreter.SagePreparseTransformer
result of L = line.lstrip() in sage.repl.preparse
f(x) = x \
       + 1 \
       + x^2

we are now in sage.repl.interpreter.SagePreparseTransformer
result of L = line.lstrip() in sage.repl.preparse
f(x) = x \
       + 1 \
       + x^2

sage: f(x)                                                                                                                                                                                           
we are now in sage.repl.interpreter.SagePreparseTransformer
result of L = line.lstrip() in sage.repl.preparse
f(x)

we are now in sage.repl.interpreter.SagePreparseTransformer
result of L = line.lstrip() in sage.repl.preparse
f(x)

x^2 + x + 1

comment:12 Changed 7 months ago by gh-jcamp0x2a

Hmm... perhaps this is an IPython quirk? For example, running an IPython installed completely independently of Sage:

$ ~/.local/bin/ipython3
Python 3.6.9 (default, Oct  8 2020, 12:12:24)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.16.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: def transform(lines):
   ...:     print(lines)
   ...:     return lines
   ...:

In [2]: get_ipython().input_transformers_post.append(transform)

In [3]: 2 + 2
['2 + 2\n']
['2 + 2\n']
Out[3]: 4

comment:13 follow-up: Changed 7 months ago by strogdon

Same here with Python 3.8.6. Bug or feature?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 7 months ago by gh-jcamp0x2a

Replying to strogdon:

Same here with Python 3.8.6. Bug or feature?

Looks like intended behavior:

  • Issue: 11714 "Arrow printed twice for autocall"
  • Pull request: 12456 "Allow to mark transformers as having side effects"

The new has_side_effects attribute would only work in IPython >= 7.17.0, and Sage only requires 7.13.0. Besides, the only side effects here should be our debugging print outs, so I vote to just let IPython do its thing.

I'll add a bit about that attribute in SagePreparseTransformer's docstring for those that stumble across this in the future.

comment:15 Changed 7 months ago by git

  • Commit changed from 68afcdb5b7ebf42e1a5c35e894759978d1bb1391 to e45c71789eb151c5c3c9d15cca17deb9ff3d135c

Branch pushed to git repo; I updated commit sha1. New commits:

e45c717Explain duplicated print outs in SagePreparseTransformer

comment:16 in reply to: ↑ 14 ; follow-up: Changed 7 months ago by strogdon

Replying to gh-jcamp0x2a:

Replying to strogdon:

Same here with Python 3.8.6. Bug or feature?

Looks like intended behavior:

  • Issue: 11714 "Arrow printed twice for autocall"
  • Pull request: 12456 "Allow to mark transformers as having side effects"

Good spotting this. I tried to search the ipython git repo bugs for this without success. I never considered that it was related to a closed issue. I have ipython-7.18.1 here on my Gentoo laptop. Your above ipython example, after importing inputtransformer2 from IPython.core and then setting inputtransformer2.has_side_effects = True, does work

In [5]: 2 + 2
['2 + 2\n']
Out[5]: 4

I'm satisfied with this, but will wait to see if @egourgoulhon has any comments.

comment:17 in reply to: ↑ 16 Changed 7 months ago by strogdon

Replying to strogdon:

I'm satisfied with this, but will wait to see if @egourgoulhon has any comments.

Or if anyone else has comments.

comment:18 follow-up: Changed 7 months ago by egourgoulhon

Thanks for the fix!

I made a few tests and everything is OK. So I am +1 for a positive review, once the pyflake error reported by the patchbot error has been fixed.

comment:19 follow-up: Changed 7 months ago by egourgoulhon

Btw, while making tests for this ticket branch, I found another error:

sage: f(x) = (x +  
....:         1)                                                                                    
  File "<ipython-input-10-ae23cb631161>", line 1
    __tmp__=var("x"); f = symbolic_expression((x + ).function(x)
                                                   ^
SyntaxError: invalid syntax

But this does not pertain to the current ticket: it is already here in Sage 9.1.

comment:20 in reply to: ↑ 18 Changed 7 months ago by gh-jcamp0x2a

Replying to egourgoulhon:

Thanks for the fix!

I made a few tests and everything is OK. So I am +1 for a positive review, once the pyflake error reported by the patchbot error has been fixed.

I think this is the same false positive that was identified in comment:8:ticket:30417. Trying to ignore it with # noqa didn't work, so I will use an assertion to silence pyflakes for good.

comment:21 in reply to: ↑ 19 ; follow-up: Changed 7 months ago by gh-jcamp0x2a

  • Status changed from needs_review to needs_work

Replying to egourgoulhon:

Btw, while making tests for this ticket branch, I found another error:

sage: f(x) = (x +  
....:         1)                                                                                    
  File "<ipython-input-10-ae23cb631161>", line 1
    __tmp__=var("x"); f = symbolic_expression((x + ).function(x)
                                                   ^
SyntaxError: invalid syntax

But this does not pertain to the current ticket: it is already here in Sage 9.1.

The code is still fresh in my mind, and this seems like a trivial fix, so I'm going to try to knock it out here. If it's not as trivial as I thought, though, I'll spin off a new ticket and put this back into review.

comment:22 in reply to: ↑ 21 Changed 7 months ago by egourgoulhon

Replying to gh-jcamp0x2a:

The code is still fresh in my mind, and this seems like a trivial fix, so I'm going to try to knock it out here. If it's not as trivial as I thought, though, I'll spin off a new ticket and put this back into review.

OK very good!

comment:23 Changed 7 months ago by git

  • Commit changed from e45c71789eb151c5c3c9d15cca17deb9ff3d135c to 64c5757b4fb67d492f0c64cf7d3bda2d3616b56c

Branch pushed to git repo; I updated commit sha1. New commits:

09b499cSilence false positive from pyflakes
64c5757preparse_calculus: allow vars to span mulitple lines

comment:24 follow-up: Changed 7 months ago by gh-jcamp0x2a

  • Status changed from needs_work to needs_review

Not trivial. Forgot about nested parentheses/braces/brackets. A regular expression is really ill-suited to handling that, so preparse_calculus is going to need some work. Can use backslash continuations as a workaround for now.

On the bright side, I was able to quickly add support for breaking the parameter list onto multiple lines like so:

f(a,
  b,
  c,
  d) = a + b*2 + c*3 + d*4

...and we'll see what pyflakes has to say now. :)

comment:25 in reply to: ↑ 24 Changed 7 months ago by gh-jcamp0x2a

Replying to gh-jcamp0x2a:

Not trivial. Forgot about nested parentheses/braces/brackets. A regular expression is really ill-suited to handling that, so preparse_calculus is going to need some work. Can use backslash continuations as a workaround for now.

Created #30953 to track this.

comment:26 follow-up: Changed 7 months ago by strogdon

Not sure what the source of the Darwin failures is. Every thing is fine here with beta2. I believe I've seen other false negative failures on Darwin.

comment:27 in reply to: ↑ 26 Changed 7 months ago by gh-jcamp0x2a

Replying to strogdon:

Not sure what the source of the Darwin failures is. Every thing is fine here with beta2. I believe I've seen other false negative failures on Darwin.

Yea, doesn't look like a very happy patchbot. :) Looking through it's history, I don't see a single green TestsPassed result, so I don't think it's due to anything introduced here.

comment:28 Changed 7 months ago by strogdon

  • Reviewers set to Steven Trogdon
  • Status changed from needs_review to positive_review

Eric please add your name to reviewers if you agree.

comment:29 follow-up: Changed 7 months ago by egourgoulhon

  • Reviewers changed from Steven Trogdon to Steven Trogdon, Eric Gourgoulhon

Thank you very much for having fixed this!

comment:30 in reply to: ↑ 29 Changed 7 months ago by gh-jcamp0x2a

Replying to egourgoulhon:

Thank you very much for having fixed this!

You are most welcome!

I'm going to spend some time brainstorming ways to make the preparser more robust, potentially including building that proper grammar/parser I mentioned earlier. Might have to be a year-long goal for 2021, though. :)

comment:31 Changed 6 months ago by vbraun

  • Branch changed from u/gh-jcamp0x2a/30928-backslash-continuation to 64c5757b4fb67d492f0c64cf7d3bda2d3616b56c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.