semFor propagate iterator position error nodes

[bung87]

Summary

semFor propagates errors that occur in the iterator position. They
are
now properly reported and semFor produces errors as required.

Details

Updated semFor to propagate error nodes in the iterator position, as
well as appropriately wrap the semantically analysed nkFor if an
error
is present.

Added adSemForExpectedIterator to AstDiagKind to capture the error
as a node instead of using legacy reports localReport.

[saem]

this change needs some tests.

reach out if you get stuck.

[zerbina]

Hm, but there's no IndexDefect happening? When semExprNoDeref returns an nkError, isCallExpr will be 'false', and call is thus never indexed into.

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 IndexDefect as part of the LSP work, then it's not related to the error handling in semFor. Changing var hasError = false in semForVars to var hasError = n[^2].kind == nkError should be enough to propagate errors in the iterable expression slot.

[saem]

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

[saem]

@bung87 do you have a stacktrace or something that shows the variant field access error?

[bung87]

Hm, but there's no IndexDefect happening? When semExprNoDeref returns an nkError, isCallExpr will be 'false', and call is thus never indexed into.

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 IndexDefect as part of the LSP work, then it's not related to the error handling in semFor. Changing var hasError = false in semForVars to var hasError = n[^2].kind == nkError should be enough to propagate errors in the iterable expression slot.

it's in not isCallExpr or call[0].kind != nkSym or call[0].sym.kind != skIterator

[zerbina]

it's in not isCallExpr or call[0].kind != nkSym or call[0].sym.kind != skIterator

Yes, but if call is an nkError, then isCallExpr is 'false', and the entire rest of the or chain is short-circuited.

[saem]

If it was an nkError then it would be a field not accessible error, if it's an index error that means it's a empty (invalid) call node. Furthermore, the isCallExpr would be false, and would not evaluate the rest of the boolean expression (McCarthy evaluation).

@bung87 can you do a astrepr.debug on call or n to see the shape of the ast?

[bung87]

hmm, I can't reproduce but I encouter index error multiple times, I just remind of it is a call with empty children.

[bung87]

As it's not clear for now, I'll investigate later

[bung87]

Found this is because LSP not properly handle global error reports come from like checkMinSonsLen, checkMinSonsLen which raise error in command line mode prevent compiler continue compiles, this may affect nimsuggest as well.

[bung87]

now changed as a refactoring not a bugfix anymore.

[saem]

found a couple items

[saem]

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.

[saem]

/merge

[github-actions]

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:

---

Notes for Reviewers