Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a3675bcdd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4b66767bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b07791338
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # https://docs.python.org/3/library/exceptions.html#SyntaxError.offset) | ||
| offset = e.offset | ||
| if offset is not None: | ||
| offset -= 1 |
There was a problem hiding this comment.
For IndentationError/TabError raised by tokenize, e.offset is the 1-based position after the offending logical line, not the indentation column; on the Python 3.12 CI workflow, the added return case reports offset 10, so this stores col 9 instead of the expected col 2. This makes the new indentation tests fail and points users at the end of the line rather than the bad indent; derive the indentation width from the error text/args for tokenizer indentation errors instead of only subtracting one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
In Python 3.12 when adding support for PEP 701, tokenize was re-implemented in C (before, after).
One regression of this process was that now IndentationErrors have their e.offset set to end of line + 1
This remains the case all the way to the previews for 3.15
See python/cpython#153837
As a consequence, we cannot give accurate positions for these kinds of errors (unless we manually re-compute the offset)
There was a problem hiding this comment.
Fixed: 568464c (by manually re-computing the offset)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37e5ff8da0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
charles-cooper
changed the title
CompilerPanic
| offset -= 1 | ||
| raise SyntaxException(e.msg, code, e.lineno, offset) from e | ||
| except UnicodeEncodeError as e: | ||
| prefix = code[: e.start] |
There was a problem hiding this comment.
explanation in this branch please
There was a problem hiding this comment.
See test_unpaired_surrogate_raises_syntax_exception
This seems like a super edge-case, but Codex suggested it, and it was fairly simple to implement.
Oh and every exception type we do not catch here is reported to the user as-is
There was a problem hiding this comment.
i meant an explanation in the code as a comment
There was a problem hiding this comment.
I wasn't sure what kind of explanation you wanted, let me know what you think
There was a problem hiding this comment.
i don't think it's worth special handling tbh? or like the issue is not wrapping exceptions properly right? if we really care about that why don't we have a fallback?
There was a problem hiding this comment.
With this PR we do this:
except _BaseVyperException:
raise
except Exception as e:
raise CompilerPanic(f"unhandled exception during parsing: {type(e).__name__}: {e}") from e
And so a UnicodeEncodeError bubbles up to the user as a CompilerPanic, even though that is not an issue of the compiler, but of the input file.
We could do the opposite assumption, and do something like:
...
except Exception as e:
raise ParserException(f"{type(e).__name__}: {e}")
But this risks showing internal mistakes as ParserExceptions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54e5650e32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
approved but i would recommend splitting out the change implied by the PR title from the other changes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f198341816
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9af7378ce6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e119edcc2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| e.resolved_path = resolved_path | ||
| raise e | ||
| except Exception as e: | ||
| raise CompilerPanic(f"unhandled exception during parsing: {type(e).__name__}: {e}") from e |
There was a problem hiding this comment.
i think dumping e as a string and also its type is redundant given the exception chain (from e)
2 participants
What I did
Catch unexpected exceptions raised during parsing (e.g.
RecursionErrorfromdeeply nested types) and surface them as a
CompilerPanicwith a helpfulmessage, instead of letting them bubble up as raw Python tracebacks.
Fixes: #5157
How I did it
Any non-Vyper exception caught during parsing is
re-raised as
CompilerPanic("unhandled exception during parsing: <Type>: <msg>")with the original exception chained via
raise ... from e.How to verify it
uv run pytest tests/unit/ast/test_parser.py::test_deeply_nested_type_raises_compiler_panicx: uint256[1][1][1]...(500 levels) — you should now get aCompilerPanicinstead of a raw
RecursionErrortraceback.Commit message
fix[parser]: wrap unhandled parser exceptions as CompilerPanic
Description for the changelog
Fix unexpected exceptions not being wrapped during parsing
Cute Animal Picture