Opened 21 months ago

Closed 21 months ago

Last modified 18 months ago

#30417 closed defect (fixed)

preparsing multi-line strings is broken

Reported by: gh-mwageringel Owned by:
Priority: blocker Milestone: sage-9.2
Component: misc Keywords:
Cc: arojas, jhpalmieri, gh-kliem, ​novoselt Merged in:
Authors: Joshua Campbell Reviewers: Jonathan Kliem
Report Upstream: N/A Work issues:
Branch: 282fda9 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

As of the IPython 7 upgrade #28197, preparsing multiline strings does not work anymore. The preparser alters multiline strings, even though it should not:

sage: """
....: abc-1-2
....: """
'\nabc-Integer(1)-Integer(2)\n'    # expected: '\nabc-1-2\n'

Reported on devel.

Change History (13)

comment:1 Changed 21 months ago by novoselt

  • Priority changed from critical to blocker

This makes pretty much all my interacts unusable with no sensible workaround to fix it. I doubt I am the only user with this issue.

comment:2 follow-up: Changed 21 months ago by gh-kliem

Note that this only happens in the sage shell not during doctesting. If you run this during a doctest, you get the following:

File "src/sage/__init__.py", line 54, in sage.isfunction
Failed example:
    '''
    abc-1-2
    '''
Expected nothing
Got:
    '\nabc-1-2\n'

So this is exactly what we want. #29139 has a doctest that works find during doctesting but not in real life.

comment:3 in reply to: ↑ 2 Changed 21 months ago by gh-mwageringel

Replying to gh-kliem:

Note that this only happens in the sage shell not during doctesting.

I assume that is because BackendDoctest does not inherit from BackendIPython, so doctesting alone will not in general detect this kind of IPython issues.

comment:4 Changed 21 months ago by gh-jcamp0x2a

  • Authors set to Joshua Campbell
  • Branch set to u/gh-jcamp0x2a/30417
  • Commit set to 282fda95b6baf610dae1f3ee9c694fe74b824eb6
  • Status changed from new to needs_review

I think I was able to track this down and have pushed a branch with an attempt at a fix. The state of whether/which set of quotes we were in wasn't being carried over across lines of input. This fixes the example code in the description and a few other trivial multi-line strings I was able to come up with off the top of my head, but would appreciate if someone could throw it at some real world examples.

comment:5 follow-up: Changed 21 months ago by gh-kliem

Apparently this does the job. Looks fine to me.

We could remove an unused import, while we are touching the file anyway:

+src/sage/repl/interpreter.py:780:13 'line_profiler' imported but unused

comment:6 Changed 21 months ago by gh-kliem

Thanks for figuring this out.

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

Replying to gh-kliem:

We could remove an unused import, while we are touching the file anyway:

+src/sage/repl/interpreter.py:780:13 'line_profiler' imported but unused

I believe that warning is a false positive. Removing the import breaks over 80% of the doctests in interpreter.py. The import is surrounded by a try-catch, so I think the intent is to check that the import could be done without actually needing it at the moment.

Thanks for taking a look at it!

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

Replying to gh-jcamp0x2a:

I believe that warning is a false positive. Removing the import breaks over 80% of the doctests in interpreter.py. The import is surrounded by a try-catch, so I think the intent is to check that the import could be done without actually needing it at the moment.

Sorry, sorry. When I commented out the import, I forgot that in Python you need a pass statement in an empty block. That's what broke the doctests, not removing the import.

That said, I think my understanding of the import's intent is still correct, and thus it shouldn't be removed. Here's the full block of code for context:

# Load the %lprun extension if available
try:
    import line_profiler
except ImportError:
    pass
else:
    self.extensions.append('line_profiler')

comment:9 in reply to: ↑ 8 Changed 21 months ago by mkoeppe

Replying to gh-jcamp0x2a:

I think my understanding of the import's intent is still correct, and thus it shouldn't be removed.

I agree

comment:10 Changed 21 months ago by gh-kliem

Sorry for keeping you busy. I usually trust those pyflakles warnings blindly, because usually it is right. But in this case this seems to be not the case.

comment:11 Changed 21 months ago by gh-kliem

  • Reviewers set to Jonathan Kliem
  • Status changed from needs_review to positive_review

Ok. Looks good to me.

comment:12 Changed 21 months ago by vbraun

  • Branch changed from u/gh-jcamp0x2a/30417 to 282fda95b6baf610dae1f3ee9c694fe74b824eb6
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 18 months ago by egourgoulhon

  • Commit 282fda95b6baf610dae1f3ee9c694fe74b824eb6 deleted

See #30928 for a possible follow-up.

Note: See TracTickets for help on using tickets.