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

Overloaded generic procvar: useless instantiations that create type mismatch #9997

Open
mratsim opened this issue Dec 15, 2018 · 0 comments
Open

Comments

@mratsim
Copy link
Collaborator

mratsim commented Dec 15, 2018

Here is a Christmas tree bug that touches upon generic instantiation, procvar, overloading and semcheck.

Scenario 1 - benign impact besides extra NimVM processing

Let's start with a simple example that establish the context. I'm working in tree datastructure and make generous usage of overloading (and generics). In my tree I also need to store a generic handler proc.

When passing the generic handler proc to initialise the tree, Nim will instantiate all overloads that match, even when the initialiser signature restrict to a single instantiation:

import typetraits

type
  MyProc[T] = proc(s: seq[T]): seq[T] {.nimcall.}

  Node[T] = object
    value: seq[T]
    handler: MyProc[T]

proc double[T: SomeNumber](x: T): T =
  static: echo "I'm in double[", x.type.name, "]"
  x + x

proc double[T](x: seq[T]): seq[T] =
  static: echo "I'm in double[", x.type.name, "]"
  result = newSeq[T](x.len)
  for i in 0 ..< x.len:
    result[i]= x[i] + x[i]

proc double[T](x: Node[T]): Node[T] =
  static: echo "I'm in double[", x.type.name, "]"
  result.value = x.value.double()

proc newNode[T](val: seq[T], handler: MyProc[T]): Node[T] =
  result.value = val
  result.handler = handler

let foo = newNode(@[1, 2, 3], double[int])
echo foo.handler.type.name

## Compile-time
# I'm in double[int]        # <---- useless instantiation
# I'm in double[seq[int]]
# I'm in double[tree[int]]  # <---- useless instantiation

## Run-time
# MyProc[system.int]

Scenario 2 - Prevents using overloaded procvar

While scenario 1 only cost is increased compilation-time due to extra instantiation and semantic checking, there is actually a side effect: your code even though correct will not compile if in any of those instantiations there is a call invalid for any of the input types.

Example:

import typetraits

type
  Container[T] = object
    value: T

  # CT for container
  RefWrapper[CT] = ref object
    raw: CT

  MyProc[CT] = proc(s: CT): CT {.nimcall.}

  Node[CT] = object
    payload: RefWrapper[CT]
    handler: MyProc[CT]

proc mul[T](a, b: Container[T]): Container[T] =
  result.value = a.value * b.value

proc mul[CT](a, b: RefWrapper[CT]): RefWrapper[CT] =
  new result
  result.raw = mul(a.raw, b.raw)

proc register_node[CT](payload: RefWrapper[CT], handler: MyProc[CT]) =
  static:
    echo payload.type.name
    echo handler.type.name

proc newRefWrapper[T](ct: Container[T]): RefWrapper[Container[T]] =
  new result
  result.raw = ct

let x = Container[int](value: 2)
let x_ref = newRefWrapper(x)

register_node(x_ref, mul[Container[int]])

# proc_inst2.nim(37, 25) template/generic instantiation of `mul` from here
# proc_inst2.nim(18, 26) Error: type mismatch: got <Container[system.int], Container[system.int]>
# but expected one of:
# proc `*`(x, y: float32): float32
#   first type mismatch at position: 1
#   required type: float32
#   but expression 'a.value' is of type: Container[system.int]

Workaround

Generic procvar should not be overloaded. It might be that not all procvar and no only generic ones have this issue

Proposed long-term solutions

  1. If there is a type restriction for procvar, either because we have let: MyProc[int] = mul[Container[int]] or we pass to another proc, do not instantiate the useless ones.

  2. Alternatively, delete the useless instantiations before semcheck starts.

mratsim added a commit to mratsim/Arraymancer that referenced this issue Dec 15, 2018
mratsim added a commit to mratsim/Arraymancer that referenced this issue Dec 15, 2018
Araq pushed a commit that referenced this issue Sep 2, 2024
fixes #16376

The way the compiler handled generic proc instantiations in calls (like
`foo[int](...)`) up to this point was to instantiate `foo[int]`, create
a symbol for the instantiated proc (or a symchoice for multiple procs
excluding ones with mismatching generic param counts), then perform
overload resolution on this symbol/symchoice. The exception to this was
when the called symbol was already a symchoice node, in which case it
wasn't instantiated and overloading was called directly ([these
lines](https://github.com/nim-lang/Nim/blob/b7b1313d21deb687adab2b4a162e716ba561a26b/compiler/semexprs.nim#L3366-L3371)).

This has several problems:

* Templates and macros can't create instantiated symbols, so they
couldn't participate in overloaded explicit generic instantiations,
causing the issue #16376.
* Every single proc that can be instantiated with the given generic
params is fully instantiated including the body. #9997 is about this but
isn't fixed here since the instantiation isn't in a call.

The way overload resolution handles explicit instantiations by itself is
also buggy:

* It doesn't check constraints.
* It allows only partially providing the generic parameters, which makes
sense for implicit generics, but can cause ambiguity in overloading.

Here is how this PR deals with these problems:

* Overload resolution now always handles explicit generic instantiations
in calls, in `initCandidate`, as long as the symbol resolves to a
routine symbol.
* Overload resolution now checks the generic params for constraints and
correct parameter count (ignoring implicit params). If these don't
match, the entire overload is considered as not matching and not
instantiated.
* Special error messages are added for mismatching/missing/extra generic
params. This is almost all of the diff in `semcall`.
* Procs with matching generic parameters now instantiate only the type
of the signature in overload resolution, not the proc itself, which also
works for templates and macros.

Unfortunately we can't entirely remove instantiations because overload
resolution can't handle some cases with uninstantiated types even though
it's resolved in the binding (see the last 2 blocks in
`texplicitgenerics`). There are also some instantiation issues with
default params that #24005 didn't fix but I didn't want this to become
the 3rd huge generics PR in a row so I didn't dive too deep into trying
to fix them. There is still a minor instantiation fix in `semtypinst`
though for subscripts in calls.

Additional changes:

* Overloading of `[]` wasn't documented properly, it somewhat is now
because we need to mention the limitation that it can't be done for
generic procs/types.
* Tests can now enable the new type mismatch errors with just
`-d:testsConciseTypeMismatch` in the command.

Package PRs:

- using fork for now:
[combparser](PMunch/combparser#7) (partial
generic instantiation)
- merged: [cligen](c-blake/cligen#233) (partial
generic instantiation but non-overloaded + template)
- merged: [neo](andreaferretti/neo#56) (trying
to instantiate template with no generic param)
Araq pushed a commit that referenced this issue Nov 6, 2024
fixes issue described in https://forum.nim-lang.org/t/12579

In #24065 explicit generic parameter matching was made to fail matches
on arguments with unresolved types in generic contexts (the sigmatch
diff, following #24010), similar to what is done for regular calls since
#22029. However unlike regular calls, a failed match in a generic
context for a standalone explicit generic instantiation did not convert
the expression into one with `tyFromExpr` type, which means it would
error immediately given any unresolved parameter. This is now done to
fix the issue.

For explicit generic instantiations on single non-overloaded symbols, a
successful match is still instantiated. For multiple overloads (i.e.
symchoice), if any of the overloads fail the match, the entire
expression is considered untyped and any instantiations are not used, so
as to not void overloads that would match later. This means even
symchoices without unresolved arguments aren't instantiated, which may
be too restrictive, but it could also be too lenient and we might need
to make symchoice instantiations always untyped. The behavior for
symchoice is not sound anyway given it causes #9997 so this is something
to consider for a redesign.

Diff follows #24276.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants