search

exercism / python-representer

GNU Affero General Public License v3.0
6 stars 9 forks source link

Representer fails on some solutions #48

Closed ErikSchierboom closed 8 months ago

ErikSchierboom commented 8 months ago

Whilst looking into https://forum.exercism.org/t/solution-isnt-displayed-in-community-solutions/10071, I found that the representer crashes on this solution: https://exercism.org/tracks/python/exercises/sgf-parsing/solutions/Timus

Traceback (most recent call last):
  File "/opt/representer/bin/run.py", line 56, in <module>
    main()
  File "/opt/representer/bin/run.py", line 52, in main
    representer.represent(args.slug, args.input, args.output)
  File "/opt/representer/representer/__init__.py", line 99, in represent
    txt_dst.write_text(representation.dump_code())
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/representer/representer/__init__.py", line 37, in dump_code
    code = utils.to_source(self._tree)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/representer/representer/utils.py", line 98, in to_source
    return astor.to_source(tree)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/astor/code_gen.py", line 62, in to_source
    generator.visit(node)
  File "/usr/local/lib/python3.11/site-packages/astor/node_util.py", line 143, in visit
    return visitor(node)
           ^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/astor/code_gen.py", line 876, in visit_Module
    self.write(*node.body)
  File "/usr/local/lib/python3.11/site-packages/astor/code_gen.py", line 177, in write
    visit(item)
  File "/usr/local/lib/python3.11/site-packages/astor/node_util.py", line 143, in visit
    return visitor(node)
           ^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/astor/code_gen.py", line 334, in visit_FunctionDef
    self.body(node.body)
  File "/usr/local/lib/python3.11/site-packages/astor/code_gen.py", line 225, in body
    self.write(*statements)
  File "/usr/local/lib/python3.11/site-packages/astor/code_gen.py", line 177, in write
    visit(item)
  File "/usr/local/lib/python3.11/site-packages/astor/node_util.py", line 143, in visit
    return visitor(node)
           ^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/astor/node_util.py", line 137, in abort_visit
    raise AttributeError(msg % node.__class__.__name__)
AttributeError: No defined handler for node of type Match
BethanyG commented 8 months ago

The library that was used to turn an AST back into code in the representer (astor) does not appear to be maintained for more recent versions of Python, so it chokes on things like structural pattern matching and the walrus operator (:=).

Fortunately core Python added ast.unparse in Python 3.8, and it was always slightly preferable to use only core Python for the representer anyways. Pull 45 replaces astor with ast.unparse, among other refactorings that fixes this bug.

edited to add: Actually, it does look like the astor folx have tried to bring the library up to date for match statements. However, I don't want to rely on them and have this break every time a new feature is added but the team is behind. I'd rather rely on the core Python team and the built-in AST lib since it is more stable and up-to-date.

Sadly, I've gotten sideswiped by writing a bunch of approaches and reviewing concepts that Colin is writing (and other life things) -- so I never got back to changing the representer to handle multiple student files, and we never merged Pull 45.

While I would love to do that final bit work , it feels as if maybe we merge what we have to fix this and other issues and do the representer re-runs?

I am happy to do the multi-file support, but if we wait on me for that, this bug (at the rate I am currently moving) is likely to go unfixed for another month or more.

BethanyG commented 8 months ago

One note: There aren't specific parsing tests for anything introduced in Python 3.9, Python 3.10, or Python 3.11, but I can add some for features I think might get high use from students. At this time, type annotations are being stripped, so there aren't any test cases needed there.

IIRC, I did make one case with pattern matching and one with a walrus. In any case, I will review and work on adding some of those today and over the weekend, just to cover our bases.

ErikSchierboom commented 8 months ago

While I would love to do that final bit work , it feels as if maybe we merge what we have to fix this and other issues and do the representer re-runs?

I am happy to do the multi-file support, but if we wait on me for that, this bug (at the rate I am currently moving) is likely to go unfixed for another month or more.

As multi-file submissions will be the outlier (I can detect those and selectively re-run them), I'm totally happy with this proposed plan.

BethanyG commented 8 months ago

I added an issue for the multi-file handling, so I don't forget. 😄 I didn't get a chance to add more smoke tests. However, we do have one already for pattern matching, which is the big one here.

I'll make an issue to add more tests for newer features, but since the representer is now using ast.unparse, I think we're largely "safe".

BethanyG commented 8 months ago

Linking PR #55 here, because in the course of writing more smoke tests, @brocla found a bug with unassigned expressions that needed fixing. This is why we test.... 😄

Since that PR is unmerged, I think I will keep this open for a little bit longer.

BethanyG commented 8 months ago

Closing this as fixed, at least until we find another bug. 😄

brocla commented 8 months ago

That is fantastic news. It is great to see success.

I'm going to send you another set of golden tests. This one is for dataclasses. The new features are slots and key-word-only options. All three of the examples come from exercism exercises with solutions that already use dataclasses. I just dressed them up a bit to use the new features.

BTW. I'll be out of town for a week. If there is something that you want to change in these examples, feel free. No need to wait for me.

Regards, Kevin

On Wed, Mar 27, 2024 at 11:22 AM BethanyG @.***> wrote:

Closing this as fixed, at least until we find another bug. 😄

— Reply to this email directly, view it on GitHub https://github.com/exercism/python-representer/issues/48#issuecomment-2023360053, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBULKX3YCN6SYN2PETXBGLY2L55HAVCNFSM6AAAAABEBSJ4NSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRTGM3DAMBVGM . You are receiving this because you were mentioned.Message ID: @.***>