Pyflakes (pyls) fails when '%' is present in notebook code

[krassowski]

Description

A weird one. Pyflakes does not work if '%' is present in code (but it is ok in comments).

I would say this is an upstream issue (of pyflakes or pyls) but it does work in plain python files - only not in notebooks.

Reproduce

Context

• Operating System and version: Ubuntu 20.04
• Browser and version: Chrome
• JupyterLab version: 2.1

python-language-server==0.31.7
pyflakes==2.1.1
jupyter-lsp==0.8.0

[krassowski]

To be more clear: pycodestyle works ok...

[bollwyvl]

Dunno... something in the document rewriting? What ends up in your .virtual_document when this goes bad?

[bollwyvl]

Yerp, just ran into this: it's definitely some missing ^ in patterns like this one:
https://github.com/krassowski/jupyterlab-lsp/blob/master/packages/jupyterlab-lsp/src/magics/defaults.ts#L65

[bollwyvl]

ah, i guess % can appear anywhere on a line... and indeed, magics can be registered with any string imaginable, so long as it doesn't start with a whitespace. This is going to be tricky.

perhaps just demanding ^|\s before it would clean up some of these cases...

[krassowski]

We can also access the CodeMirror token which is used for highlighting (string/comment/variable etc), but then we are going for a solution which would only work in some frontends.

I like the proposed regex but ^|\s would fail on x =%ls. So ^|\s|=.

However, parentheses, brackets and +/-/*/\ are not supported: x = (%ls), and x - %ls rise SyntaxError. This could potentially change in the future though... Probably would be good to consult the IPython implementation/ping someone who maintains magics to learn if they wish to keep it simple or expand the capabilities in the future.

This definitely needs some more unit tests.

[bollwyvl]

Yeah, magics are gross. A heavyweight approach might be to cache the actual token names that come back from the kernel completer for %. Since the underlying plumbing is regex based, I don't think they can be truly dynamic. Another thing: even if it is hacked to work, other features (hover) get broken... get_ipython is not defined. So in addition to rewriting the line, a replacer may need to be able to add a set of valid header lines, e.g "from IPython import get_ipython, display" to deal with autoimports, etc. Some of these could just always be done, I suppose, which would be less complex.

[krassowski]

Did some short digging and on the IPython side they do not have a regular expression for that now - they use tokens (not sure which tokenizer, but will definitely differ from CodeMirror) and have a special case for assignments.

I think we can go with the improved regexpr for now (for a release ETA this weekend), and think about a better way of transforming documents afterwards.

[krassowski]

Obviously, even with the improved regex, it will be still possible to break things: list(" %ls") → list(" get_ipython().run_line_magic("ls")", "")