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:  sage9.3 
Component:  symbolics  Keywords:  callable symbolic expression, backslash, preparser 
Cc:  ghjcamp0x2a, 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: 
Description
As reported in https://groups.google.com/g/sagedevel/c/XD1VtG0TOEk/m/Lo6L8YBGCgAJ, we have currently (Sage 9.2 and 9.3.beta1):
sage: f(x) = x \ ....: + 1 File "<ipythoninput1a5c1e14e19da>", 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
comment:2 Changed 7 months ago by
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
comment:4 Changed 7 months ago by
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 \ <ipythoninput19e03ae3fa6f44>:1: DeprecationWarning: invalid escape sequence \ shell.run_cell('f(x) = x \ + 1')  AttributeError Traceback (most recent call last) <ipythoninput1981598207d80e> in <module> > 1 __tmp__=var("x"); f = symbolic_expression(x * BackslashOperator() * + Integer(1)).function(x) /local/sagegit/sage/local/lib/python3.8/sitepackages/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/sagegit/sage/local/lib/python3.8/sitepackages/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/sagegit/sage/local/lib/python3.8/sitepackages/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/sagegit/sage/local/lib/python3.8/sitepackages/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 followup: ↓ 6 Changed 7 months ago by
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
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
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): 444 444 sage: # instead of ["'''", 'abcInteger(1)Integer(2)', "'''"] 445 445 446 446 """ 447 print('we are now in sage.repl.interpreter.SagePreparseTransformer') 447 448 lines_out = [] 448 449 reset = True 449 450 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, 1745 1745 quote_state = None 1746 1746 1747 1747 L = line.lstrip() 1748 print('result of L = line.lstrip() in sage.repl.preparse') 1749 print(L) 1748 1750 if len(L) > 0 and L[0] in ['#', '!']: 1749 1751 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 "<ipythoninput1a5c1e14e19da>", 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 properlyfunctioning Sage.
comment:8 Changed 7 months ago by
 Cc ghjcamp0x2a slelievre added
comment:9 Changed 7 months ago by
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 "<ipythoninput1bc21d2198ee4>", 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
 Branch set to u/ghjcamp0x2a/30928backslashcontinuation
 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 afterstrip_string_literals
so we don't have to worry about backslash continuations inside strings, and it happens beforepreparse_calculus
so that it can surround the entire thing with asymbolic_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 linebyline. The prompts should've already been removed by theSagePromptTransformer
so I think that's a safe operation. Also, since IPython has already handled the%
magic commands immediately prior to ourSagePreparseTransformer
, I removed the explicit check for them.
 I've done something similar for the
preparse_file
use case with the complication of supportingload
andattach
statements. Basically, just callspreparse
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:
68afcdb  Fix backslash continuation for symbolic expressions

comment:11 Changed 7 months ago by
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
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 followup: ↓ 14 Changed 7 months ago by
Same here with Python 3.8.6
. Bug or feature?
comment:14 in reply to: ↑ 13 ; followup: ↓ 16 Changed 7 months ago by
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
 Commit changed from 68afcdb5b7ebf42e1a5c35e894759978d1bb1391 to e45c71789eb151c5c3c9d15cca17deb9ff3d135c
Branch pushed to git repo; I updated commit sha1. New commits:
e45c717  Explain duplicated print outs in SagePreparseTransformer

comment:16 in reply to: ↑ 14 ; followup: ↓ 17 Changed 7 months ago by
Replying to ghjcamp0x2a:
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 ipython7.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
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 followup: ↓ 20 Changed 7 months ago by
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 followup: ↓ 21 Changed 7 months ago by
Btw, while making tests for this ticket branch, I found another error:
sage: f(x) = (x + ....: 1) File "<ipythoninput10ae23cb631161>", 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
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 ; followup: ↓ 22 Changed 7 months ago by
 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 "<ipythoninput10ae23cb631161>", line 1 __tmp__=var("x"); f = symbolic_expression((x + ).function(x) ^ SyntaxError: invalid syntaxBut 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
Replying to ghjcamp0x2a:
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
 Commit changed from e45c71789eb151c5c3c9d15cca17deb9ff3d135c to 64c5757b4fb67d492f0c64cf7d3bda2d3616b56c
comment:24 followup: ↓ 25 Changed 7 months ago by
 Status changed from needs_work to needs_review
Not trivial. Forgot about nested parentheses/braces/brackets. A regular expression is really illsuited 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
Replying to ghjcamp0x2a:
Not trivial. Forgot about nested parentheses/braces/brackets. A regular expression is really illsuited 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 followup: ↓ 27 Changed 7 months ago by
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
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
 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 followup: ↓ 30 Changed 7 months ago by
 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
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 yearlong goal for 2021, though. :)
comment:31 Changed 6 months ago by
 Branch changed from u/ghjcamp0x2a/30928backslashcontinuation to 64c5757b4fb67d492f0c64cf7d3bda2d3616b56c
 Resolution set to fixed
 Status changed from positive_review to closed
The issue is already there in Sage 9.2.beta14.