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

Using ^ from stdlib/math along with converters gives a match for types that aren't SomeNumber #15033

Closed
HugoGranstrom opened this issue Jul 21, 2020 · 4 comments · Fixed by #15034

Comments

@HugoGranstrom
Copy link
Contributor

The ^ proc in stdlib/math doesn't restrict T to just floats and ints, and this gives problems when implementing ^ for custom types involving converters. In my case, it's when both operands need to be converted that I stumble upon this.

Example

import math

type
  A = object
    val*: int
  B = object
    value*: int

converter intToA(a: int): A =
  result.val = a

converter NotAToA(a: B): A =
  result.val = a.value
  
proc `^`(a: A, b: A): A =
  result.val = a.val ^ b.val

let a = A(val: 2)
let b = B(value: 2)
echo a ^ b # one conversion, works fine 
echo a ^ 2 # one conversions, works fine
echo b ^ 2 # two conversions, does not work

Current Output

template/generic instantiation of `^` from here
/home/hugo/.choosenim/toolchains/nim-#devel/lib/pure/math.nim(981, 18) Error: type mismatch: got <int literal(1)> but expected 'B = object'

Expected Output

(val: 4)
(val: 4)
(val: 4)

Possible Solution

The line that gives the error is this where it sets result = 1. But the proc's prototype is proc `^`*[T](x: T, y: Natural): T which causes problems when T isn't int- or float-like and gives the above error message. A fix that worked for me is to narrow down what T can be:

proc `^`*[T: SomeNumber](x: T, y: Natural): T

This makes sure that this proc is only called when it actually works with T.

Additional Information

  • I don't know if ^ is meant to work with any type except SomeNumber but if there is just close this.
  • For the curious-minded, in my real code I'm getting this error in, I'm trying to implement a SymbolicExpression library. I have an ExpressionType and then I do also have a VariableType for symbolic variables. When doing arithmetics with variables, they get converted automatically to ExpressionType. The problem arises when I'm using ints as well as variables. Both need to be converted, but the compiler finds a match for it so the conversion never happens.
$ nim -v
Nim Compiler Version 1.3.5 [Linux: amd64]
HugoGranstrom added a commit to HugoGranstrom/Nim that referenced this issue Jul 21, 2020
@andreaferretti
Copy link
Collaborator

I guess the right type would be T: Multiplicable, where

type Multiplicable = concept x, y
  x * y is type(x)

or something like that. One can take powers of things that are not numbers, such as matrices or, as you mentioned, symbolic expressions

@HugoGranstrom
Copy link
Contributor Author

HugoGranstrom commented Jul 22, 2020

Yeah that could make sense, but then we would have to rewrite the proc, right? Epically the case where y = 0. Then we would require the type to have some sort of x.zero/x.one or something, because x.default isn't always 0 or valid for that matter (take rationals for example: default is 0/0)

@HugoGranstrom
Copy link
Contributor Author

But I think for custom types that one does most of the time want to implement ^ by oneself. For example, I just take the base and exponent and put them in a new SymbolicExpression, no repeated multiplication. And I imagine there are more efficient ways to do it for Matrices? So perhaps the stdlib should only be concerned about floats and ints?

Araq pushed a commit that referenced this issue Jul 22, 2020
@andreaferretti
Copy link
Collaborator

So perhaps the stdlib should only be concerned about floats and ints?

Agreed, maybe it's easier like this

mildred pushed a commit to mildred/Nim that referenced this issue Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants