Properly report default argument inference errors #2504
Conversation
errors for default arguments could leave the AST in an illegal state which was causing assertions when evaluating default args during TK_CALL evaluation in the expr pass. This PR makes those errors stop compilation immediately, so they are properly reported.
mfelsche
added
the
changelog - fixed
There was a problem hiding this comment.
I'd personally prefer this bug to be fixed with the change to uifset. Returning AST_FATAL in a pass can be problematic since it stops error reporting for this compilation, and can require the programmer to do multiple edit-compile cycles if they have lots of errors in their code. As such, we only want to issue AST_FATAL results if an error is absolutely unrecoverable and we can't wait until the end of the current pass to stop AST processing.
Generally speaking, if there is an error in the source code the AST will be in an "unwanted state" anyway, so I don't think it is a problem to guard against such edge cases as long as it is in the same pass.
|
Yeah, i totally see your point @Praetonus |
|
You're right that the error shouldn't be reported twice, and in fact I think you were right from the beginning when saying that the change in The root issue here, for both the crash and the duplicated error message, is that the default argument AST goes through the expr pass twice. This is due to the direct |
as this does not result in double error messages when evaluating default args for function definitions and function calls
|
TravisCI MacOS builds are WAY behind again. There's a backlog of about 2,800. It's completely nuts. I've cancelled the MacOS builds associated with this. I ran the tests locally for LLVM 3.9.1 which is what we really care about. They passed. |
|
@Praetonus @SeanTAllen is this mergable from your point of view? |
* Properly report default argument inference errors errors for default arguments could leave the AST in an illegal state which was causing assertions when evaluating default args during TK_CALL evaluation in the expr pass. This PR makes those errors stop compilation immediately, so they are properly reported. * use ast_passes_subtree instead of expr_seq for default arguments as this does not result in double error messages when evaluating default args for function definitions and function calls
Errors for default arguments could leave the AST in an illegal state,
which was causing assertions when evaluating default args during TK_CALL evaluation in the expr pass.
This PR makes those errors stop compilation immediately, so they are properly reported.
This fixes #2496
I did not handle
TK_LITERALinuifsetas it seemed to be more of an unwanted state of the AST in general, if such an AST node reaches this function and thus tried to avoid getting into this branch in the first place.