-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix(sem): nimsuggest
crashing for ill-formed AST
#939
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Can the other PR be closed now?
The details section in the PR message is good, but the title and summary need to be adjusted. This PR represents a significant fix for nimsuggest
, but it also affects compiles
, both which needs to be highlighted.
Regarding the implementation, I've left some requests regarding code style, documentation, and test improvements.
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
reportAndForceRaise
checkSonsLen
, checkSonsMinLen
not fast fails in suggest mode
checkSonsLen
, checkSonsMinLen
not fast fails in suggest modecheckSonsLen
, checkSonsMinLen
not fast fail in suggest mode
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not noticing this earlier. As is, the implementation ignores the default actions, which means that --errorMax
is ignored by reportAndForceRaise
.
You need to change reportAndForceRaise
to first go through errorActions
(refer to handleReport
for how this should look like), before forcing doNothing
to doRaise
.
At this point, I think that reportAndFail
would be a better name for the procedure.
like this? case conf.report(rep)
of doAbort:
quit 1
of doDefault:
let (action, trace) = errorActions(conf, rep, eh)
case action
of doAbort:
quit 1
of doRaise, doNothing:
raiseRecoverableError("report")
of doRaise, doNothing:
raiseRecoverableError("report") |
Hmm, I just noticed a much simpler and also more broad fix that also fixes However, I don't want to block this PR any longer, and would thus make a follow-up PR myself. In the meantime, this fix works well enough. Sorry for all the detours, and thank you for working on this. |
checkSonsLen
, checkSonsMinLen
not fast fail in suggest modenimsuggest
crashing for ill-formed AST
@bung87 I've updated the PR message and title, please take a look at it:
Once the remaining suggestion is applied, this PR is good to be merged. |
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
I also think this can be simpler, eg. the condition can depends on |
/merge |
Summary
Fix
nimsuggest
, and other tooling that uses a custom structuredreport hook, crashing when encountering ill-formed AST that has the
wrong number of nodes.
Details
Many of the semantic analysis procedures first verify that the input
PNode
has the right number of child nodes. This is done via thecheckSonsLen
andcheckMinSonsLen
procedures, with the expectationbeing that both raise an
ERecoverableError
(throughglobalReport
)when the check fails.
However, since reporting the error happens via
handleReport
, it'spossible that the error handling strategy is overridden with a mode
(e.g.,
doNothing
) that results in execution to continue without anyexception being raised.
nimsuggest
sets the error handling mode todoNothing
, which then resulted incheckSonsLen
andcheckMinSonsLen
not ending the active routine, usually leading toIndexDefect
s or other crashes.As an interim solution, the
reportAndFail
procedure is introduced.reportAndForceRaise
works similar to similar tohandleReport
, butit ensures that at least an exception is raised -- both
checkSonsLen
andcheckMinSonsLen
use it to ensure that the callingroutine is exited.