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

Enaml syntaxerror detail #530

Merged
merged 12 commits into from
Sep 8, 2023

Conversation

bburan
Copy link
Contributor

@bburan bburan commented Jun 7, 2023

See #529 for details. Note that the unit tests I added do not actually capture the bug for some reason, but I can verify that in my own code (where I discovered the bug), the changes I made to raising SyntaxError solves the issue.

This does not actually capture the error I am attempting to fix.
@MatthieuDartiailh
Copy link
Member

I am surprised that GHA did not run. Also it looks like your change could break older Python since old syntax error probably won't handle well so many arguments.

@bburan
Copy link
Contributor Author

bburan commented Jun 7, 2023

I tested down to 3.8 and it did not break. I think SyntaxError was designed to accept *args and then it just ignores the extra ones in earlier versions of Python.

@bburan
Copy link
Contributor Author

bburan commented Jun 7, 2023

I see the tests are failing. It's possible I did not test correctly. I think we can just add a check for if PY310 to use the new syntax?

@bburan
Copy link
Contributor Author

bburan commented Jun 7, 2023

FYI, pip install -e .\enaml has not been working for me. I have to do a regular pip install before I can test changes. I'm not sure why.

This also fixes a bug in the unit test (the assert statement
was missing).
@MatthieuDartiailh
Copy link
Member

What error do you see ?

I have not seen such issues but I usually regenerate the parser using the full path to the script, not sure it should make a difference.

@bburan
Copy link
Contributor Author

bburan commented Jun 7, 2023

So, when I pip install -e .\enaml in an environment that already had Enaml installed from PyPI, I get the following error:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "C:\Users\mmm\miniconda3\envs\psi4\Scripts\cfts.exe\__main__.py", line 4, in <module>
  File "C:\Users\mmm\projects\psi4\src\cfts\cfts\app\main.py", line 3, in <module>
    with enaml.imports():
         ^^^^^^^^^^^^^
AttributeError: module 'enaml' has no attribute 'imports'

However, I just created a fresh environment to test pip install -e .\enaml and it worked fine. So, it is likely a bug with pip. Regardless, this is a separate issue. The main issue is that there's something odd about SyntaxError on Python 3.11 that is preventing it from providing the full details of the error. Without this patch, I just know which file the error is in. With the patch, I know what line the error is on. I pushed a fix to add compatibility for Python 3.8 and 3.9. Not sure why it hasn't shown up yet (it's on the enaml-syntaxerror-detail branch in my repo).

@bburan
Copy link
Contributor Author

bburan commented Jun 9, 2023

Ok. That last commit finally showed up here and the CI checks seem to have passed successfully for all Python versions. I'm working on another computer and ran into this exact same issue where I keep getting a very non-informative SyntaxError message:

Traceback (most recent call last):
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "C:\Users\mmm\miniconda3\envs\psi1\Scripts\noise-exp.exe\__main__.py", line 4, in <module>
  File "C:\Users\mmm\projects\psi1\src\noise-exp\noise_exp\main.py", line 4, in <module>
    from .gui import Main
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\import_hooks.py", line 139, in exec_module
    code, _ = self.get_code()
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\import_hooks.py", line 400, in get_code
    return self.compile_code()
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\import_hooks.py", line 364, in compile_code
    ast = parse(self.read_source(), file_info.src_path)
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\parser\__init__.py", line 66, in parse
    return _parse(
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\parser\__init__.py", line 42, in _parse
    return parser.parse("start")
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\parser\base_python_parser.py", line 79, in parse
    raise SyntaxError("invalid syntax", (self.filename, token.start, 0, token.line))
SyntaxError: invalid syntax (gui.enaml)

Incorporating this patch fixes it, giving me:

Traceback (most recent call last):
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "C:\Users\mmm\miniconda3\envs\psi1\Scripts\noise-exp.exe\__main__.py", line 4, in <module>
  File "C:\Users\mmm\projects\psi1\src\noise-exp\noise_exp\main.py", line 4, in <module>
    from .gui import Main
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\import_hooks.py", line 139, in exec_module
    code, _ = self.get_code()
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\import_hooks.py", line 400, in get_code
    return self.compile_code()
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\import_hooks.py", line 364, in compile_code
    ast = parse(self.read_source(), file_info.src_path)
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\parser\__init__.py", line 66, in parse
    return _parse(
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\parser\__init__.py", line 42, in _parse
    return parser.parse("start")
  File "C:\Users\mmm\miniconda3\envs\psi1\lib\site-packages\enaml\core\parser\base_python_parser.py", line 79, in parse
    raise SyntaxError("invalid syntax", (self.filename, *token.start, token.line, *token.end))
  File "C:\Users\mmm\projects\psi1\src\noise-exp\noise_exp\gui.enaml", line 18
    )
SyntaxError: invalid syntax

This is on Python 3.10.9 (the system I originally discovered this error on is Python 3.11).

@MatthieuDartiailh
Copy link
Member

Could you add an entry to the changelog and commit a regenerated parser ? I think the fact we changed the base but did not update the parser is why appveyor is complaining but I find it a bit weird.

lines = tb.strip().split("\n")
line = '\n'.join(lines[-4:])

expected = 'enamldf CustomLabel(Container):'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in enamldef

@frmdstryr
Copy link
Contributor

frmdstryr commented Jun 15, 2023

I added detail for indentation errors. Could you try it & cherry pick from https://github.com/frmdstryr/enaml/tree/indexerror-detail ?

For some reason the generated parser is formatted weirdly so I didn't commit it.

@MatthieuDartiailh
Copy link
Member

You need to manually run black after generating it.

@MatthieuDartiailh
Copy link
Member

The CIs look unhappy. I should be able to look at this during the week.

@bburan
Copy link
Contributor Author

bburan commented Aug 31, 2023

@MatthieuDartiailh Sorry for the long delay. Finally got around to fixing the unit tests. One is still failing (upload to codecov), but I don't think that's relevant to the changes made in this commit.

@MatthieuDartiailh
Copy link
Member

Thanks ! Could you add a changelog entry and a regenerated parser (formatted with black) ?

@bburan
Copy link
Contributor Author

bburan commented Sep 7, 2023

@frmdstryr The errors above are related to the IndexError tests you added. It seems that Enaml is properly detecting that there is a SyntaxError; however, it's being reported as a general SyntaxError rather than an IndentationError. I'm not sure how to fix this. I also switched to your branch and tested on Python 3.10 and it's not working either. Only two of the tests (childdef-indent-mismatch and class) are being reported as IndentationErrors. All others are SyntaxErrors. Thoughts?

@frmdstryr
Copy link
Contributor

Odd... seems like something with pegen 0.2.0. If you revert to pip install pegen==0.1.0 and regen the parser they all pass.

I guess the parser with the latest pegen is not calling raise_indentation_error.

@bburan
Copy link
Contributor Author

bburan commented Sep 7, 2023

@frmdstryr That explains why the tests passed at one point, but then did not pass. I was doing a line-by-line comparison and was stumped. Thanks for catching that. @MatthieuDartiailh, any thoughts? I'm not familiar with pegen, so I'm not sure where to start and whether this is a bug in Enaml or in Pegen.

@MatthieuDartiailh
Copy link
Member

MatthieuDartiailh commented Sep 7, 2023

Switching to pegen 0.2.0 is somewhere on my list of things to do for 3.12. Right now I have bytecode running and atom is close but CI segfault when exiting. Next I will look into pegen (f-string require some extra work) and finally reach enaml but it will take time. For this PR simply regen with 0.1.0 and commit. You can also pin pegen if I forgot to do it.

Pegen 0.2.0 results in some IndentationError exceptions
being raised as SyntaxError instead.
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #530 (c91b3e7) into main (87e9c31) will increase coverage by 0.08%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head c91b3e7 differs from pull request most recent head 236da37. Consider uploading reports for the commit 236da37 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #530      +/-   ##
==========================================
+ Coverage   72.65%   72.73%   +0.08%     
==========================================
  Files         287      287              
  Lines       24417    24425       +8     
  Branches     4343     4344       +1     
==========================================
+ Hits        17740    17766      +26     
+ Misses       5591     5580      -11     
+ Partials     1086     1079       -7     
📢 Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nucleic).

@bburan
Copy link
Contributor Author

bburan commented Sep 7, 2023

Looks like it passed, finally. 🎉 Thanks @MatthieuDartiailh and @frmdstryr for your help!

releasenotes.rst Outdated Show resolved Hide resolved
@MatthieuDartiailh MatthieuDartiailh merged commit 4639e27 into nucleic:main Sep 8, 2023
31 checks passed
@MatthieuDartiailh
Copy link
Member

Thanks @bburan and @frmdstryr

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 this pull request may close these issues.

3 participants