Fix compiler crash in alternate name suggestion logic (issue #2508). #2552
Conversation
|
This is a possible fix for #2508. |
|
The commit WIP-Fix-issue-2508-with-dbg has some debug and an test case in minimal-cases/issue-2508. |
|
I'd like to see a regression test added for this in the compiler tests instead of the minimal-cases folder. That is, I think we can test that the correct error is returned instead of crashing the compiler (as it currently does). |
|
I don't understand, the crash is in suggest_alt_name where the compiler is assuming there was a typo and it's trying to guess what that might be. My fix removes the pony_assert, adds some additional code to improve the guess and either finds a suggestion or returns NULL. The WIP-Fix-issue-2508-with-dbg and minimal-case/issue-2508 code was only used durning developement of the fix and I'm not sure we'd want to make any specific assumtions on what suggest_alt_name should return. If you want, I can add some version(s) of my minimal-case and test that specific suggestions are returned. But, as I said up above, maybe I just don't understand what you meant. |
|
Sorry, I can try to be more clear. I meant that this PR should add a test to That test should verify that the compiler returns the correct compile error instead of crashing. |
|
Will do, txs.
…On Thu, Feb 22, 2018 at 6:27 AM Joe Eli McIlvain ***@***.***> wrote:
Sorry, I can try to be more clear.
I meant that this PR should add a test to test/libponyc/badpony.cc with
the minimal case from this comment: #2508 (comment)
<#2508 (comment)>
That test should verify that the compiler returns the correct compile
error instead of crashing.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2552 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA-hHC6D7uACO0zjDyTdgswhAnTvHpDqks5tXXlkgaJpZM4SGI_G>
.
|
The pony_assert(ast_id(id == TK_ID) assumes that ast_id(id)) is always a TK_ID, but that is not the true if the first child of case_ast is a function or behavior. This fix checks for a TK_ID for both first and second children of case_ast and gives up if neither is a TK_ID instead of asserting.
There are three cases tested plus a test for nothing can be suggested.
winksaville
force-pushed
the
Fix-issue-2508
branch
from
4b64089
to
8789ab1
Compare
|
@jemc, I've added some tests, please take a look. |
jemc
changed the title
jemc
added
the
changelog - fixed
|
Awesome. Thanks @winksaville. |
…#2508). (ponylang#2552) * Fix for issue 2508 The pony_assert(ast_id(id == TK_ID) assumes that ast_id(id)) is always a TK_ID, but that is not the true if the first child of case_ast is a function or behavior. This fix checks for a TK_ID for both first and second children of case_ast and gives up if neither is a TK_ID instead of asserting. * Add tests for suggest_alt_name There are three cases tested plus a test for nothing can be suggested.
The pony_assert(ast_id(id == TK_ID) assumes that ast_id(id)) is
always a TK_ID, but that is not the true if the first child of case_ast
is a function or behavior.
This fix checks for a TK_ID for both first and second children of
case_ast and gives up if neither is a TK_ID instead of asserting.