Fix compiler crash on partial application of method with literal-call default#5343
Draft
SeanTAllen wants to merge 2 commits into
Draft
Fix compiler crash on partial application of method with literal-call default#5343SeanTAllen wants to merge 2 commits into
SeanTAllen wants to merge 2 commits into
Conversation
… default Partially applying a method whose default desugars to a single call on a literal (the canonical example: `USize = -1`, which sugar rewrites as `(1).neg()`) crashed the compiler with an internal assertion in `lookup_base`. The mechanics: `partial_application` duplicates the unsupplied parameter's default into a synthesized lambda. `expr_object` then lifts that lambda to an anonymous class and calls `ast_resetpass(def, PASS_SUGAR)`, wiping the pass flags on the whole subtree — including the dup'd default, whose literal had already been coerced to a concrete type. The expr pass re-walks the literal and `make_literal_type` blindly overwrites the coerced type back to `TK_LITERAL`. The surrounding nodes' types don't get reset because pass_expr handlers short-circuit when a type is already set. Verify pass later calls `lookup` on the funref's now- literal-typed receiver and asserts. I'm fixing this at the symptom: guard `make_literal_type` so it only fires when the literal has no type yet. I don't think that's the most principled fix — the real problem is the `ast_resetpass` + re-walk cycle, and a cleaner approach would be to either not reset already- processed dup'd subtrees, or audit every pass_expr handler so re-running on coerced AST is genuinely idempotent. I picked the targeted fix because chasing the principled one looks like a quagmire, and I'd rather close the assertion now than dig a deeper hole trying to fix it right. Test is a full-program test rather than libponyc — the libponyc test framework hits a pre-existing issue with `neg` lookup on `USize` during on-demand default-arg typing, which is unrelated to #2480 but blocks a clean unit test. A nested literal-method-call default like `USize = (-1).abs()` still crashes, in `uifset` rather than `lookup_base`. Same bug class, different assertion site. Tracked in #5342. Closes #2480 Closes #3727
SeanTAllen
added
the
changelog - fixed
ponylang-main
added
the
discuss during sync
The earlier fix put the "don't overwrite a coerced type" guard at one of make_literal_type's five callers. Same hazard applies to the other four — anywhere the dup'd-then-re-walked subtree happens to be an array literal, tuple, recover block, or operator-literal call. Moving the guard inside make_literal_type covers all of them in one place. The guard still honours TK_INFERTYPE, which `lookup`'s on-demand default-arg typing sets when it wants a re-type. Also added pointer comments at the two sites that create the hazard — `ast_resetpass` in expr_object and the `TREE(p_default)` dup in partial_application. A maintainer changing either of those should not have to grep for the consequence in expr/literal.c.
Member
Author
|
Opening this PR was an exploration — I wanted to see what a targeted fix looked like before committing to it. With #5342 surfacing as a sibling crash this approach doesn't address, I'm reading it as a strong signal we're playing whack-a-mole. Each new symptom in this bug class will need its own guard, and the underlying re-walk-on-dup'd-AST problem stays put. I think we need to talk about a more principled fix. Probably looks like restructuring the partial-application path so it doesn't re-walk subtrees that came from already-processed code — Holding this PR pending that conversation. |
SeanTAllen
removed
the
discuss during sync
Member
|
I think the right fix here would be to update the lexer/parser area to handle negative numbers as a parse-time evaluation rather than sugaring to |
ponylang-main
added
the
discuss during sync
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Partially applying a method whose default desugars to a single call on a literal (e.g.
USize = -1→(1).neg()) crashed the compiler with an assertion inlookup_base. The duplicated default got re-walked after pass flags were reset on the lifted lambda's anonymous class, andmake_literal_typeclobbered the previously-coerced concrete type. Fix is a guard insidemake_literal_typeso it doesn't overwrite an already-set type (exceptTK_INFERTYPE, which is the explicit "please re-type me" marker thatlookup's on-demand default-arg typing uses).This is a targeted fix at the symptom site rather than a principled fix at the
ast_resetpass-then-rewalk cycle that creates the inconsistent state. The first commit's message explains the trade-off. The follow-up commit centralizes the guard (it now covers all five callers ofmake_literal_type, not just theTK_INT/TK_FLOATcase inpass_expr) and adds pointer comments at the two sites that create the hazard.Closes #2480
Closes #3727
Related
The fix doesn't cover all sibling crashes. Nested literal-method-call defaults like
USize = (-1).abs()still crash with a different assertion (uifset:484). Filed as #5342.