-
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
semFor
propagate iterator position error nodes
#932
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.
this change needs some tests.
reach out if you get stuck.
Hm, but there's no I've tried with both your test case, and the reduced: for x in doesntExist: # provokes an error
discard and neither fails. If you're encountering an error |
I tried the following, which is a variant of what the macro did, and it failed appropriately without a crash: for i in (if true: discard else: discard):
discard |
@bung87 do you have a stacktrace or something that shows the variant field access error? |
it's in |
Yes, but if |
If it was an @bung87 can you do a |
hmm, I can't reproduce but I encouter index error multiple times, I just remind of it is a call with empty children. |
semFor
not handle semExprNoDeref
returns error nodesemFor
not handle call with empty children
As it's not clear for now, I'll investigate later |
Found this is because LSP not properly handle global error reports come from like |
semFor
not handle call with empty childrensemFor
propagate error node
929aa73
to
f5e482b
Compare
now changed as a refactoring not a bugfix anymore. |
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.
found a couple items
Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
semFor
propagate error nodesemFor
propagate iterator position error nodes
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.
The new code is better, nice.
I did have to spend a fair bit of time updating the commit message. The formatting was off and the summary contained an implementation description, instead of a description of behavioural change.
/merge |
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below:
|
Summary
semFor
propagates errors that occur in the iterator position. Theyare
now properly reported and
semFor
produces errors as required.Details
Updated
semFor
to propagate error nodes in the iterator position, aswell as appropriately wrap the semantically analysed
nkFor
if anerror
is present.
Added
adSemForExpectedIterator
toAstDiagKind
to capture the erroras a node instead of using legacy reports
localReport
.