sagemathinc / cocalc

CoCalc: Collaborative Calculation in the Cloud
https://CoCalc.com
Other
1.16k stars 211 forks source link

sage worksheet bug -- ending a block of code with ? shouldn't always submit introspection request #1835

Open williamstein opened 7 years ago

williamstein commented 7 years ago
  1. Put this code in a worksheet:
    a = 5 # what?
  2. Hit shift enter and get introspection.
screen shot 2017-04-04 at 10 07 49 am
williamstein commented 7 years ago

Here's how jupyter does it -- they make a custom python mode that treats ? in a special way. I don't like this approach at all.


Comment I put on a PR

Sorry, I don't think this will help much with #1835 because: (1) that needs to run on the frontend, which is when doing introspection is determined, (2) that also needs to support all languages, not just python. What you've done here must be only python.

The right solution for #1835 is to use Codemirror's own parser, and run CodeMirror.runMode over the code to be submitted first. See, e.g., https://github.com/sagemathinc/smc/blob/jupyter2/src/smc-webapp/jupyter/parsing.coffee#L7

That's not to say that we don't want this on the backend like you have it. It's just not going to solve #1835.

williamstein commented 7 years ago

This is easy now, since parsing.coffee is in master.

DrXyzzy commented 7 years ago

The approach in parsing.coffee will work for cell input ending with a comment ending with one or two question marks, but I don't see a simple solution for dealing with this input, which also evokes the error:

a = 5 # what?
b = 6

I've looked briefly at the python parser in codemirror at src/smc-webapp/codemirror/mode/python.js, and don't see a simple way of extracting individual blocks, as is done in src/smc_sagews/smc_sagews/sage_parsing.py

Am I missing something?

williamstein commented 7 years ago

Am I missing something?

No, I forgot that the block parser is only implemented in the backend. Dang.

DrXyzzy commented 7 years ago

There is a fix for this bug that is commented out... Can we restore that fix and close this bug? @haraldschilly, in view of @williamstein last comment above about the block parser only implemented in the back end, does it make sense to restore the 7-line ushlex fix to sage_server.py around here?

I have verified that using the uncommented code passes the pytest suite for sagews*, including lexer tests that we have been skipping pending resolution of this bug.

*there is one test that fails that i will delete shortly because it is no longer relevant: the test for sagemath kernel in jupyter bridge.

haraldschilly commented 7 years ago

Well, back then I found cases where ushlex trips over the input and causes problems. (I tried to guard it in a try-block, but that didn't really help here). Hence unfortunately we can't use this and need a better solution.

There are somewhere python language parsers in the python core library, but no idea if they could help us either, because in essence, we want to parse invalid Python code in a meaningful way.

My general feeling is we should write our own parser (I have the feeling it's just 10 to 20 lines for a simple heuristic) and try to aim for getting it to work in as many cases as possible and graceful degradation upon parsing errors.

DrXyzzy commented 7 years ago

@haraldschilly, thank you.

Do you have examples where ushlex did not work? I would like to add them to the test suite.

I'll look into parsing alternatives.

haraldschilly commented 7 years ago

@DrXyzzy I don't remember exactly, but try this for a start (in a sagews worksheet)

def parse(b):
    from ushlex import shlex
    try:
        s = shlex(b)
        s.commenters = '#'
        s.quotes = '"\''
        b = ''.join(s)
        return b
    except Exception as e:
        return 'ERROR: ' + str(e)
parse(u"x = 1 # ?")  # good
parse(u"x = 1; x?")  # good
parse(u"test('x # test?') # ?") # good
parse(u"test('x # test?') + foo? # baz?") # good
parse(u"'") # bad
parse(u'"') # bad
parse(u"foo '") # bad
parse(u"bar(' # baz) # foo") # bad
parse(u'bar("""# baz"") # foo') # bad
parse(u'%foo bar(9)') # bad, concatenates foo and bar
parse(u'%foo bar?') # bad, concatenates foo and bar

note, that the last two don't produce an error, but they concatenate the string.

haraldschilly commented 7 years ago

ok, I found the "original" error related to it. So, please also add this:

parse(u"var('x',latex_name=r'(x)')") # bad

there, the quotes are fine, but the "r" confuses it. i.e. this is a deeper issue than just looking for balanced quotes.

DrXyzzy commented 7 years ago

Re-opening because bug is still there. PR #2227 only cleaned up related tests in sagews pytest suite.

haraldschilly commented 5 years ago

Was reported again. Example below. I remember that I wasn't able to figure out how to fix the parser. It's harder than I hoped for :frowning_face:

screenshot from 2019-01-18 18-48-01

williamstein commented 2 weeks ago

Tagging this as wontfix, because it will be fixed indirectly by creating a sage worksheet style mode (and kernel) for Jupyter notebooks. That will be https://github.com/sagemathinc/cocalc/issues/6374.

In fact, I fixed exactly this issue recently in our Jupyter implementation.