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

better dependent-expression support for range types #850

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Aug 20, 2023

Summary

Fix complex type-variable-dependent expressions not working reliably
for range and array-index types used in routine signatures.
Internally, expressions in both contexts are now first treated as
generic, and using replaceTypesInBody for expressions with
potentially unbound type variables is removed.

Language changes:

  • syntax macros and templates (i.e., those where all parameters are
    untyped) used in range- and index-type expression are expanded
    before all further analysis (and thus typed macros/template, static
    expressions, etc.)
  • typed macros and templates used in range- and index-type expressions
    depending on type variables are potentially expanded multiple times

Fixes:

  • type-variable-dependent range and array-index types in routine
    signatures can now be arbitrarily complex (refer to the added tests
    for an example of what is now possible)

Details

Overview:

  • all expressions used with range types and in array-index positions
    now use the generic pre-pass
  • type-variable-dependent expressions in range type constructors are
    now always generic expressions (i.e., symbols are looked up, but
    not AST is not typed)
  • replaceTypesInBody is renamed to instantiateTypesInBody and
    support for unresolved type variables is removed -- the new
    replaceTypeVarsInBody now has to be used for that case

Semantic analysis has very basic, scattered support for expressions
depending on unresolved type variables, allowing semExprWithType to
be used for typing expressions in range and array type constructors.
However, in more complex cases, this usually results in errors with
unclear descriptions. Until more complete support for typing expressions
depending on unresolved type variables is implemented, it is more
robust to treat them as fully generic.

Analyzing the expressions

Both semRangeAux and semArrayIndex now always run the generic pre-
pass first (semGenericStmt, which also expands syntax macros), which
is required for detecting whether unresolved type variables are used.
When no unresolved type variables exist in the analyzed expression, it
is directly passed on to semExprWithType.

The procedure for querying whether a symbol is that of an unresolved
type variable (isUnresolvedSym), is moved to the ast_query
module.

Looking for unresolved type variables is combined with making sure that
all type variables are referenced in the AST via their symbols
(fixupTypeVars) -- something that is not always already the case. Some
type variables not having their symbol bound would mean that the later
pass for replacing type variables cannot detect them.

As an unintended side-effect of using semGenericStmt, unrelated
symbols are marked as used, which is what causes the different error
message in tgeneric_backtrack_type_inference.nim (the presence of the
sfUsed flag for generic parameters of routines decides whether a
routine is marked with tfUnresolved).

Processing the generic expressions

For processing generic expressions used in the context of static
parameter inference, where unbound type variables are possible, the
replaceTypeVarsInBody procedure is introduced. While using
replaceTypesInBody with allowMetaTypes = true would have sufficed,
this is intended as a step towards removing the "meta-types allowed"
mode from semtypinst.

Implementation-wise, replaceTypeVarsInBody is similar to prepareNode,
with the differences being that replaceTypeVarsInBody:

  • does not instantiates or traverses into any types
  • only considers symbol nodes referring to type variables

Expression already having a type are not analyzed again when used as call
operands, and thus two compromises are required for the generic-expression
approach to work:

  • both prepareNode and replaceTypeVarsInBody have to mirror
    how semSym assigns types to symbol nodes of type variables (fixType
    implements this)
  • replaceTypeVarsInBody has to remove from the expression the types
    previously assigned by both semRangeAux and semArrayIndex

replaceTypesInBody is renamed to instantiateTypesInBody. In addition,
the unnecessarily exported replaceTypeVarsN is un-exported.

Misc

  • restore the name of the tgenericshardcases test, which was
    erroneously changed to tsharedcases during a test suite cleanup

Range expressions and expressions in the array-index type position are
now first sem'ed as generic expressions, and only if they don't depend
on type variables are they fully semantically analyzed.

This is going to require some changes to the type variables replacing
logic in `semtypinst`. As a preparation for this, the AST coming out of
the generic pre-pass is pre-processed to have symbols bound for all type
variables.
In addition, the test labeled `tsharedcases` is changed back to its
correct original title `tgenericshardcases`.
@zerbina zerbina added bug Something isn't working enhancement New feature or request compiler/sem Related to semantic-analysis system of the compiler labels Aug 20, 2023
@zerbina zerbina linked an issue Aug 20, 2023 that may be closed by this pull request
5 tasks
@zerbina zerbina requested a review from saem August 20, 2023 20:39
@zerbina
Copy link
Collaborator Author

zerbina commented Aug 22, 2023

The PR message was quite the mess, sorry for that. I've cleaned it up a bit, and hopefully it's easier to read and parse now.

Looking ahead, the new fixupTypeVars, if merged, would be a great help in fixing the issues with when statements inside generic objects (they too use semExprWithType at the moment).

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.

The way it works now is quite a bit better. Only some non-blocking suggestions.

Test coverage seems good. I tried to come up with some meaningful range tests to suggest, but nothing particularly novel occurred to me. The best I could come up with is:

proc r(t: typedesc, rng: range[-typeNameLen(t), typeNameLen(t]): int =
  result = rng.type.high - rng.type.low

doAssert r(MyType, 0) == 12

Comment on lines +160 to +161
## Computes the correct type to assign to the symbol node of a replaced
## type variable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Computes the correct type to assign to the symbol node of a replaced
## type variable.
## Computes the correct type to assign to the symbol of a replaced type
## variable.

not sure if that's too pendantic, but we're technically working with the symbol and not an nkSym.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, we're working with the symbol, but the returned type is meant to be assigned to the node, hence the explicit mention.

compiler/sem/semtypinst.nim Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
discard """
errormsg: "\'vectFunc\' doesn't have a concrete type, due to unspecified generic parameters."
errormsg: "type mismatch: got <proc (x: vector[vectFunc.T]): vector[vectFunc.T], vector[3]>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is really nice.

@saem
Copy link
Collaborator

saem commented Aug 22, 2023

The PR message was quite the mess, sorry for that. I've cleaned it up a bit, and hopefully it's easier to read and parse now.

thanks for the additional edits, in fairness i found the earlier version was already pretty good, but the improvement is appreciated all the same.

Looking ahead, the new fixupTypeVars, if merged, would be a great help in fixing the issues with when statements inside generic objects (they too use semExprWithType at the moment).

considering i'd like to lean on when and generics a whole lot more (reducing macro usage) that's a win all around. :D


Awesome PR.

@zerbina zerbina marked this pull request as draft August 22, 2023 19:47
@zerbina
Copy link
Collaborator Author

zerbina commented Aug 22, 2023

Thanks a lot for the review, @saem.

Adding a test for range was a good call, as it uncovered unrelated issues with typeRel for range types. They are not hard to fix, but I think it'd be better to do so in a follow-up PR (I've marked the added test as a known issue).

zerbina and others added 2 commits August 22, 2023 22:26
Doesn't work yet, due to unrelated issues with `typeRel`.
Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
@zerbina zerbina marked this pull request as ready for review August 23, 2023 14:10
@zerbina
Copy link
Collaborator Author

zerbina commented Aug 23, 2023

/merge

@github-actions
Copy link

Merge requested by: @zerbina

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


Notes for Reviewers

  • the original goal was only to remove the "meta-types allowed" mode from replaceTypesInBody, but the PR effectively turned into a partial reboot of sem: simplify generic parameter symbol handling #644
  • the changes are a preparation for multiple other improvements to generic symbol handling, sigmatch, and semtypinst

@chore-runner chore-runner bot added this pull request to the merge queue Aug 23, 2023
Merged via the queue into nim-works:devel with commit 05da5d2 Aug 23, 2023
27 checks passed
@zerbina zerbina deleted the better-dependent-expression-support-for-ranges branch August 23, 2023 16:38
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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

simplify instantiation of generic types (semtypinst)
2 participants