-
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): errors in lambdas no longer crash #1437
fix(sem): errors in lambdas no longer crash #1437
Conversation
Summary --------- Errors in lambdas requiring inference (`proc(e: auto) echo e`), no longer crash the compiler. Instead errors within the lambda body are reported and lambda inference fails as it should. Details ------- `semInferredLambda` failed to wrap its output when there were errors. This would reach `transf` and result in a compiler crash by hitting an `unreachable` assertion. Now errors in the body are reported immediately (to provide context for inference failure) and AST output from the inference attempt is correctly wrapped such that inference appropriately fails.
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.
I left a few suggestions regarding corrections/improvements.
One problem there is with the change as it is, is that errors in the body of generic lambda expressions will now be reported twice: once in semInferredLambda
, and once when the wrapper nkError
is reported.
My inability to come up with a satisfying solution is why I didn't make a PR myself. What could work is only wrapping the s.ast
in an error (but not result
). sigmatch
would then detect that case, localReport
the error, and fail the overload.
It is of course also possible to just output the errors twice, though there should be an XXX
/TODO
/FIXME
mentioning that then, I think.
Two remarks regarding the PR description:
- inferring the
auto
types in the lambda expression is already done by the timesemInferredLambda
is called, so an error happening there is - strictly speaking - not an inference failure - the tree returned by
instantiateTypesInBody
is detached from the input AST, so there was already no input AST mutation (besides altering the symbol) going on
tests/lang_callable/generics/tauto_inference_failure_due_to_body_error.nim
Outdated
Show resolved
Hide resolved
tests/lang_callable/generics/tauto_inference_failure_due_to_body_error.nim
Outdated
Show resolved
Hide resolved
…dy_error.nim Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
…dy_error.nim Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Doubled error reporting compared to a crash is an improvement in my mind.
I think not having
In my mind I consider the body of the lambda to be a part of the contract on the
Good point, will update. |
So I've given this some more thought and I think the fundamental issue is that there is no way to differentiate between a failure of the contract on If I keep following this reasoning then it seems to me that anything with So what that tells me we need to produce an error in order to halt further analysis of overloads. Does that sound about right? |
Of course. It wasn't mentioned anywhere, so I figured I'd at least bring it up, and like I said, double-reporting works as a solution for now.
I agree. I was just describing a temporary compromise that could be made. |
This is more of a remark than anything else, but as far as proc generic[T](x: T) = discard
proc p(x: proc(x: int)) = discard
p generic
# is equivalent to (for typeRel):
p proc(x: auto) = ... My opinion is that within the scope of overload resolution / parameter type inference, both should behave the same way. Also, to be clear, this is not something that I think is blocking this PR. |
Sorry for not answering earlier. I thought about this for a bit, but wasn't able to come up with a satisfactory answer. Building upon what you said, there are currently three different cases for lambda body errors:
Number 1 could be detected early on (but currently isn't), while 2 and 3 are - like you said - not really distinguishable (depending on what falls under number 2). Matching something else does seem fine to me when
If |
/merge |
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below:
|
Summary
Errors in lambdas requiring inference (
proc(e: auto) = echo e
), nolonger crash the compiler. Instead errors within the lambda body are
reported and lambda inference fails as it should.
Details
semInferredLambda
failed to wrap its output when there were errors.This would reach
transf
and result in a compiler crash by hitting anunreachable
assertion.Now errors in the body are reported immediately (to provide context for
inference failure wrt matching the body) and AST output from the
inference attempt is correctly wrapped such that inference appropriately
fails.
Fixes #1381
In addition, some minor refactoring was done to avoid shadowing the
input parameter
n
and making it look like we were mutating theinput.