Skip to content
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): crash on unmatched immediate routines #929

Conversation

saem
Copy link
Collaborator

@saem saem commented Sep 27, 2023

Summary

Unmatched immediate macros and templates within a generic context
no longer result in a compiler error.

Details

Immediate macros and templates are not dispatched via sigmatch and
instead semgnrc directly calls evaltempl.evalTemplate, in turn
evalTemplateArgs is called which can produce an error if an incorrect
number of arguments are provided. evalTemplateArgs is also used by
semMacroExpr, so similar issues would also exist for macros.

Now the error from evalTemplateArgs is immediately returned by
evalTemplate and treated as a mismatch in semgnrc.semGenericStmt.
Doing so prevents a compiler crash, attempt to access the sons of an
error
node. A test has been added as part of this change, which was
originally
from: zevv/npeg#68 (comment)

Unmatched `immediate` `macro`s and `template`s within a generic context
no longer result in a compiler error.

## Details

Immediate macros and templates are not dispatched via `sigmatch` and
instead `semgnrc` directly calls `evaltempl.evalTemplate`, in turn
`evalTemplateArgs` is called which can produce an error if an incorrect
number of arguments are provided. `evalTemplateArgs` is also used by
`semMacroExpr`, so similar issues would also exist for macros.

Now the error from `evalTemplateArgs` is immediately returned by
`evalTemplate` and treated as a mismatch in `semgnrc.semGenericStmt`.
Doing so prevents a compiler crash, attempt to access the sons of an error
node. A test has been added as part of this change, which was originally
from: zevv/npeg#68 (comment)
@saem saem requested a review from Clyybber September 27, 2023 23:08
@saem saem added bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler labels Sep 27, 2023
@saem
Copy link
Collaborator Author

saem commented Sep 27, 2023

In terms of an improved implementation, I'm currently considering a "match" proc of some sort and then if that passes call evalTemplate.

@zerbina
Copy link
Collaborator

zerbina commented Sep 28, 2023

A more minimal test case:

template unique(x, y: untyped) =
  ## A template (macro would work too) that is not overloaded. 
  # it's important that `y` is actually used within the template. The
  # error wouldn't occur otherwise
  y

proc test[T]() =
  unique("test")

test()

In terms of an improved implementation, I'm currently considering a "match" proc of some sort and then if that passes call evalTemplate.

With match, do you mean a procedure that performs an arity check? If yes, then I think that's the way go.

At a later point (i.e., not something that I think has to happen as part of this PR), this could be extended to test each overload in the symbol choice, so that overloaded immediate macros/templates are resolved during the generic pre-pass.

@saem
Copy link
Collaborator Author

saem commented Sep 28, 2023

@zerbina, thanks for looking at this.

I didn't cut the bug's trigger down enough, it's the > being treated as a prefix and based on the definition inside system that's failing to match in the original. There evalTemplateArgs returned an nkError due to argument count mismatch, evalTemplateAux attempted to index into the error node, thinking it was an nkArgList, leading to a variant field access violation on sons crash. The part I'll have to try is the reduced example you provided because I can't explain why y must appear in the template body to force the error.

Also you're correct, it would be an arity check, basically doing what evalTemplateArgs is doing now.

@zerbina
Copy link
Collaborator

zerbina commented Sep 28, 2023

You're welcome :)

Sorry for the confusion, the wording in the test snippet I provided put the focus on the wrong part: the only important part is that a template parameter is used within the body, otherwise actual (the template arguments) is never indexed into in evalTemplateAux. Both the snippet I provided and the test case with > trigger the same error.

@saem
Copy link
Collaborator Author

saem commented Sep 28, 2023

thanks for the explanation. :)

@saem
Copy link
Collaborator Author

saem commented Oct 3, 2023

revised the test and creating a mismatch procedure is pretty expensive, it'd double up the analysis of evalTemplateArgs, it should be revisited after nkArgList wrapping is introduced.

@saem
Copy link
Collaborator Author

saem commented Oct 3, 2023

/merge

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Merge requested by: @saem

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


Notes for Reviewers

  • this is an immediate fix; creating a mismatch detection procedure would have
    resulted in analyses the args twice among other issues
  • longer term:
    • nkCall and friends should wrap their args in nkArgList
    • this can then be semantically analysed and better returned with error nodes
    • more legacy report removal (i.e. localReport) to clean-up the error messages

@chore-runner chore-runner bot added this pull request to the merge queue Oct 3, 2023
Merged via the queue into nim-works:devel with commit 1821094 Oct 3, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants