Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should invalid code be representable? #287

Closed
Zac-HD opened this issue Apr 23, 2020 · 2 comments · Fixed by #340
Closed

Should invalid code be representable? #287

Zac-HD opened this issue Apr 23, 2020 · 2 comments · Fixed by #340

Comments

@Zac-HD
Copy link
Contributor

Zac-HD commented Apr 23, 2020

Hi there! I've been working on Zac-HD/hypothesmith#2, generating random CSTs in order to turn them back in to Python source code and ruin someone's day help test parsers, autoformatters, and so on. LibCST has been fantastic for this - thanks so much!

In the process, I've encountered two cases where Hypothesmith generated a valid CST and could convert this to source code, but calling compile on the result gave a SyntaxError:

from libcst import *

import_node = Import(
    names=[ImportAlias(name=Attribute(value=Float(value="0."), attr=Name(value="A")))],
)
print(Module([import_node]).code)  # `import 0..A` is invalid syntax

try_node = Try(
    body=SimpleStatementSuite(body=[]),
    handlers=[
        ExceptHandler(body=SimpleStatementSuite(body=[])),
        ExceptHandler(body=SimpleStatementSuite(body=[])),
    ])
print(Module([try_node]).code)  # two `except:` clauses is invalid syntax

In the first case, the float 0. needs to be in parens. For the second, Try._validate would need to reject multiple except: handlers, or handlers where except: is present but not the last handler listed.

I don't know whether these odd behaviour will be considered a bug, since my use-case is quite unusual and I can work around them fairly easily, but it seemed worth reporting. If you do want to have _validate reject all invalid code though, I can definitely help find those cases 😅

@zsol
Copy link
Member

zsol commented Apr 23, 2020

As one of the main usecases of LibCST is to be able to construct source code, I think it should catch these kinds of errors. Bug or not, IMO it's worth addressing this :)

@jimmylai
Copy link
Contributor

Those cases can be rejected by updating corresponding _validate functions and include the examples as test cases. We can use this thread to collect those cases and anyone who is interested in fixing can submit PRs.

sk- added a commit to sk-/LibCST that referenced this issue Jul 12, 2020
For `Try` statements we ensure that the bare except, if present, is at the last position.

For ImportAlias we ensure that the imported name is valid.

Fixes Instagram#287
jimmylai added a commit that referenced this issue Jul 14, 2020
* fix: improve validation for ImportAlias and Try statements

For `Try` statements we ensure that the bare except, if present, is at the last position.

For ImportAlias we ensure that the imported name is valid.

Fixes #287

* Apply suggestions from code review

Add missing periods.

* Apply suggestions from code review

Add missing periods.

* Update libcst/_nodes/tests/test_import.py

Co-authored-by: jimmylai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants