Closed saem closed 1 month ago
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 wrappernkError
is reported.
Doubled error reporting compared to a crash is an improvement in my mind.
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 notresult
).sigmatch
would then detect that case,localReport
the error, and fail the overload.
I think not having result
be an error is the wrong intention for the procedure, if anything s.ast
shouldn't be wrapped. As far as the signature goes the procedure initially matches enough, but the body has to sem in order for the inference to be considered successful, the body is sort of a contract on the auto
parameter.
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
In my mind I consider the body of the lambda to be a part of the contract on the auto
parameter.
- 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
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 auto
parameter via the body vs a programmer error.
If I keep following this reasoning then it seems to me that anything with auto
can't exactly participate in overload resolution, or rather it needs to be "terminal" in overload resolution, because of the ambiguity above. We can't have it silently match something else, just because there is an error in the body as we can't tell if that's a contract failure or a programmer error.
So what that tells me we need to produce an error in order to halt further analysis of overloads.
Does that sound about right?
Doubled error reporting compared to a crash is an improvement in my mind.
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 think not having
result
be an error is the wrong intention for the procedure
I agree. I was just describing a temporary compromise that could be made.
If I keep following this reasoning then it seems to me that anything with
auto
can't exactly participate in overload resolution
This is more of a remark than anything else, but as far as typeRel
and proc
type inference are concerned, a lambda with auto
parameters is no different from the symbol of a generic routine. That is:
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.
Does that sound about right?
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:
break
)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 semInferredLambda
failed for one overload since, because:
untyped
might exist and could recover from the errorIf semInferredLambda
fails due to an inference-related error (e.g., when arg is Type: {.error.}
), another non-untyped
overload might still succeed.
/merge
Merge requested by: @saem
Contents after the first section break of the PR description has been removed and preserved below:
## Notes for Reviewers * not an ideal fix with the eager output, but I considered it a reasonable compromise to keep the change small
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 reachtransf
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 https://github.com/nim-works/nimskull/issues/1381
In addition, some minor refactoring was done to avoid shadowing the input parameter
n
and making it look like we were mutating the input.