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

sem: fix type inference for static parameters #1433

Merged

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Aug 25, 2024

Summary

  • fix compiler crash when passing empty aggregate values to static
    parameters
  • fix static T parameter values not being of type T, when the
    argument is not directly of type T but requires a conversion
  • fix converters not being considered for arguments to static
    parameters

Details

When matching an expression against a formal parameter that:

  • is a static parameter
  • contains a static type somewhere (e.g., int or static[float])

the expression was compile-time evaluated first, and on success,
assigned a tyStatic type. If the tyStatic argument type matched the
formal tyStatic type, the argument type was bound to the formal
tyStatic as-is.

In case an implicit conversion is necessary, the conversion was applied
only after the evaluated tyStatic was bound to the parameter type,
leading to:

  • the actual value inlined at usages of the static parameter having
    the wrong type
  • type inference for empty aggregate types not working

Solution

If the formal type is a static T, full argument matching (including
implicit conversion and converter handling) for between the argument
and T is performed first, and only in case of success is the
expression compile-time evaluated, and the resulting tyStatic type
bound to the parameter type.

Post-match fitting has to take place prior to const evaluation (in
order to correctly type empty aggregates), so a new procedure
tryEvalStaticArgument is introduced that handles the static
arguments.

The new static handling renders the macro/template static special-
casing obsolete; replacing the static argument with the evaluation
result is now done in evalTemplateArgs.

Typed AST

For calls to routines with static arguments, the typed argument
expression now stays in the AST as-is, instead of being replaced with
the evaluated value.

The evaluated expression now contains conversions, fixing typing
problems with empty container expressions and also making sure
converters are taken into account.
It's now taken care of in a unified manner by the new `tyStatic` late
evaluation.
This is necessary for the original expression to be replaced with their
evaluated-to value. The remaining macro/template static inlining in
`sigmatch` thus becomes obsolete.
Without it, empty container types wouldn't be patched up properly.
Instead of doing this in `sigmatch`, the new `tryEvalStaticArgument`
procedure is added to `sem` and made available to `sigmatch` via
`PContext`.

It applies the post-match fitting plus the `tyStatic` wrapping.
The new ones cover `static` parameters.
The test would have previously failed, as `sigmatch` previously looked
for a `float` -> `static int` converter.
@zerbina zerbina added bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler labels Aug 25, 2024
@zerbina
Copy link
Collaborator Author

zerbina commented Aug 25, 2024

The failure of tgeneric_various.nim is due to proc symbols passed to static[proc()] parameters now being turned into closures (as they should). The evaluated result is an nkClosure, however, which can currently not pass through sem again.

compiler/sem/sem.nim Outdated Show resolved Hide resolved
Only the no-environment form is handled, as closures with an
environment shouldn't be able to leave compile-time evaluation in the
first place.
Restructure the type creation a bit, with the goal of making it easier
to follow.
Use `fitNode` to restore the implicit conversion, which makes sure that
an error is reported in case there's a type mismatch (only possible in
macro-generated AST).
@zerbina
Copy link
Collaborator Author

zerbina commented Aug 26, 2024

For making static[proc()] parameters work again, I've added handling for nkClosure to semExpr -- nkClosure AST is now turned back into an nkHiddenStdConv. I think it's an okay solution for now; the alternatives would have been to either:

  • implement full analysis for nkClosure
  • OR turn closures without an environment into just an nkSym node in vmcompilerserdes

Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the this solution, where semexprs handles nkClosure is better as it's less action at a distance (compared to vmserdes), and full nkClosure analysis would be built into sem, likely involving semexprs anyhow.

@saem
Copy link
Collaborator

saem commented Aug 27, 2024

/merge

Copy link

Merge requested by: @saem

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


Notes for Reviewers

@chore-runner chore-runner bot added this pull request to the merge queue Aug 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 27, 2024
@zerbina zerbina added this pull request to the merge queue Aug 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 27, 2024
@zerbina zerbina added this pull request to the merge queue Aug 27, 2024
Merged via the queue into nim-works:devel with commit 17f96a1 Aug 27, 2024
35 checks passed
@zerbina zerbina deleted the fix-type-inference-for-static-parameters branch August 27, 2024 19:48
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