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

sigmatch: simpler procedural type matching #647

Merged
merged 7 commits into from
May 15, 2023

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Apr 26, 2023

Summary

  • simplify the logic for handling generic routines using auto as the
    return type
  • reject un-instantiated routines earlier when used in branch and array
    constructor expressions, resulting in clearer error messages
    (has no concrete type instead of cannot instantiate)
  • fix spurious cannot instantiate errors when passing generic routines
    as arguments to parameters that are also generic
  • support lambda expressions coming from template expansions

Details

Introduced by nim-lang/Nim#3234, when both the
formal and actual type are unresolved meta types, the relation is
considered to be isBothMetaConvertible. The intention seems to have
been supporting passing auto-returning routines or lambda-expressions
to generic procedure-type parameters, but the logic introduced by the
aforementioned PR is not necessary for achieving that.

auto return types now use dedicated handling in procTypeRel, both
the formal and actual type being unresolved generics results in a
mismatch again, and typeRel is no longer applied repeatedly for
isBothMetaConvertible matches.

There was also the issue of generateTypeInstance being misused, which
could result in spurious cannot instantiate or internal compiler
errors. Until type instantiation properly supports the case where it's
not certain whether all used generic parameters have been inferred
already, the tryGenerateInstance routine is introduced as a temporary
solution.

Finally, both nkStmtListExpr and nkBlockExpr nodes are now
considered when instantiating a generic routine as part of parameter
matching, which allows for complex expressions yielding un-instantiated
generic routines (e.g.: call((discard x; genericRoutine))).

@zerbina zerbina added bug Something isn't working enhancement New feature or request refactor Implementation refactor compiler/sem Related to semantic-analysis system of the compiler labels Apr 26, 2023
@zerbina
Copy link
Collaborator Author

zerbina commented Apr 26, 2023

Regarding the effect on overload quality, the only difference is for generic procedural types with an auto return type. If after instantiation, the concrete procedural type matches exactly, the match is now considered as 1 conversion + 1 generic instead of 1 conversion + 1 exact. Example case:

proc call(x: proc(x: int): int {.nimcall.}) = discard

proc generic[T](x: T): auto {.nimcall.} =
  result = x

call(generic) # after instantiation, the types match exactly

I'm unsure whether this is a good idea or not.

@zerbina zerbina added the simplification Removal of the old, unused, unnecessary or un/under-specified language features. label Apr 26, 2023
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.

This is far more sensible than before. 🎉

I made some doc suggestions and possibly discovered a case that needs covering (nkBlockExpr).

compiler/sem/sem.nim Outdated Show resolved Hide resolved
compiler/ast/ast_types.nim Outdated Show resolved Hide resolved
compiler/sem/semtypinst.nim Outdated Show resolved Hide resolved
compiler/sem/sigmatch.nim Outdated Show resolved Hide resolved
compiler/sem/sigmatch.nim Outdated Show resolved Hide resolved
# possible in a concept context. There's nothing to instantiate
return nil
else:
# nothing else is able to provide uninstantiated generic routines
Copy link
Collaborator

Choose a reason for hiding this comment

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

(just thinking outloud; entirely ignorable)

This isn't a bug, just an observation, sigmatch has an assumption that all name lookups have been complete and we're dealing with symbols only (not counting untyped AST parameter values, but sigmatch just passes those around like opaque values). This is one of those invariants of the module that I'm figuring out how to capture in the long run (entirely outside this PR).

compiler/sem/sem.nim Outdated Show resolved Hide resolved
tests/lang_callable/overload/tproc_type_inference.nim Outdated Show resolved Hide resolved
tests/lang_callable/overload/tproc_type_inference.nim Outdated Show resolved Hide resolved
@saem
Copy link
Collaborator

saem commented Apr 26, 2023

Regarding the effect on overload quality, the only difference is for generic procedural types with an auto return type. If after instantiation, the concrete procedural type matches exactly, the match is now considered as 1 conversion + 1 generic instead of 1 conversion + 1 exact. Example case:

proc call(x: proc(x: int): int {.nimcall.}) = discard

proc generic[T](x: T): auto {.nimcall.} =
  result = x

call(generic) # after instantiation, the types match exactly

I'm unsure whether this is a good idea or not.

It's not ideal, but I'm not quite sure if +1 generic and exact is either. If it's cumbersome to fix right now I would note it in a comment as an issue and move on.

@zerbina zerbina force-pushed the simpler-proc-type-matching branch from a9b1215 to f027a54 Compare May 14, 2023 16:55
@zerbina
Copy link
Collaborator Author

zerbina commented May 14, 2023

I have reverted the effects an instantiated auto-returning generic has on an overloads quality back to how they were before this PR.

The change with prepareMetatypeForSigmatch was also not quite correct:

  • it doesn't always produce proper instances (because the instance cache is partially disabled), which causes subtle issues and can lead to C compiler errors
  • the isMetaType + containsGenericType check isn't able to detect generic phantom types like, for example, Type[T] = object, which would lead to compiler assertions (rightfully) failing

To more properly fix the misuse of generateTypeInstance for the time being, I introduced the tryGenerateInstance routine. It uses prepareMetatypeForSigmatch as a way to check if all referenced generic parameters have been inferred already, and only if that is the case creates a proper instance via generateTypeInstance. This is rather inefficient, but it's the most robust and low-impact temporary solution that I could come up with.

zerbina added 5 commits May 14, 2023 18:51
Summary
=======

* simplify the logic for handling generic routines using `auto` as the
  return types
* reject un-instantiated routines earlier when used  in branch and array
  constructor expressions, resulting in clearer error messages
  (`has no concrete type` instead of `cannot instantiate`)
* support lambda expressions coming from template expansions

Details
=======

Introduced by nim-lang/Nim#3234, when both the
formal and actual type are unresolved meta types, the relation is
considered to be `isBothMetaConvertible`. The intention seems to have
been supporting passing `auto`-returning routines or lambda-expressions
to generic procedure-type parameters, but the logic introduced by the
aforementioned PR is not necessary for achieving that.

`auto` return types now use dedicated handling in `procTypeRel`, both
the formal and actual type being unresolved generics results in a
mismatch again, and `typeRel` is no longer applied repeatedly for
`isBothMetaConvertible` matches.

There was also the issue of `generateTypeInstance` being misused, which
could result in spurious `cannot instantiate` or internal compiler
errors. Since meta types are possibly involved, the correct routine to
use is `prepareMetatypeForSigmatch`. Testing that the produced
type instance is not a meta type was also done incorrectly (`isMetaType`
was used instead of `containsGenericType`).

Finally, `nkStmtListExpr` nodes are now considered when instantiating a
generic routine as part of parameter matching, which allows for complex
expressions yielding un-instantiated generic routines (e.g.:
`call((discard x; genericRoutine))`).
* fix typos
* rename `genericProcCheck` to `exprNotGenericRoutine`
Block expressions that produce a generic routine are now also
supported.
A procedural type that matches exactly after the procedure was
instantiated now counts towards exact matches again, instead of towards
generic ones.
@zerbina zerbina force-pushed the simpler-proc-type-matching branch from f027a54 to fd92bc8 Compare May 14, 2023 20:02
@zerbina
Copy link
Collaborator Author

zerbina commented May 14, 2023

That should fix the test failures. I had forgotten that the tfHasMeta flag is also not excluded properly, meaning that it cannot be relied on at all for types produced by replaceTypeVarsT when meta-types are allowed.

zerbina added 2 commits May 14, 2023 22:00
No overload resolution is taking place in the tests.

In addition, slightly adjust the "return type of formal procedural type
inference" case in a way so that it better catches inference failures.
`generateTypeInstance` is not intended to be used when it's not certain
that all generic parameters have a concrete type bound -- it's misuse
led to spurious instantiation failure errors.

It first has to be verified that all used generic parameters resolve to
concrete types, which is for now achieved via first calling
`prepareMetatypeForSigmatch` and then checking that the produced type
is not a generic type.
@saem
Copy link
Collaborator

saem commented May 15, 2023

/merge

@github-actions
Copy link

Merge requested by: @saem

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


To-do

  • add a negative test for making sure that non-matching overloads are rejected properly
  • decide what effect a generic routine with an auto return type should have on an overload's quality
  • maybe rename isBothMetaConvertible

Notes for Reviewers

@chore-runner chore-runner bot added this pull request to the merge queue May 15, 2023
@saem
Copy link
Collaborator

saem commented May 15, 2023

This is a good step forward in making this bit of sem make more sense.

Merged via the queue into nim-works:devel with commit 804d21a May 15, 2023
@zerbina zerbina deleted the simpler-proc-type-matching branch May 22, 2023 21:32
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 refactor Implementation refactor simplification Removal of the old, unused, unnecessary or un/under-specified language features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants