nim-works / nimskull

An in development statically typed systems programming language; with sustainability at its core. We, the community of users, maintain it.
https://nim-works.github.io/nimskull/index.html
Other
272 stars 38 forks source link

c code error type mismatch on generic objects with same T parameters but one has a T from FFI #1431

Open mrgaturus opened 2 weeks ago

mrgaturus commented 2 weeks ago

Example

type
  MyValue[T] = object
    a, b: T

proc value[T](a, b: T): MyValue[T] =
  result.a = a
  result.b = b

let
  a: cint = 10
  b: cint = 10
  # -- C Code Error Here --
  # -- type cint {.importc: "int".} = int32
  v0: MyValue[int32] = value(a, b) # MyValue[cint]
  v1: MyValue[int32] = value(b, a) # MyValue[cint]

echo v0.a - v0.b
echo v1.a - v1.b

Actual Output

~/.cache/nimskull/type_d/@mtype.nim.c: In function ‘NimMainModule’:
~/.cache/nimskull/type_d/@mtype.nim.c:109:23: error: incompatible types when assigning to type ‘_I7MyValue_16777236’ from type ‘_I7MyValue_16777231’
  109 |         v0__type_24 = _7;
      |                       ^~
~/.cache/nimskull/type_d/@mtype.nim.c:113:23: error: incompatible types when assigning to type ‘_I7MyValue_16777236’ from type ‘_I7MyValue_16777231’
  113 |         v1__type_30 = _8;

Expected Output

i think it should be a type mismatch error regarding using int16 or other implicit convertible types marks as type mismatch error

Possible Solution

without using a generic proc and use object initialization syntax instead it compiles

v0: MyValue[int32] = MyValue[cint](a: a, b: b)
v1: MyValue[int32] = MyValue[cint](a: b, b: a)

Additional Information

References

zerbina commented 2 weeks ago

The underlying issue here is that imported integer-like types are treated inconsistent in the compiler:

Since typeRel considers cint and int32 equal, it also considers MyValue[cint] and MyValue[int32] equal, even though they're not, thus erroneously not rejecting the assignment.

The object construction assignment only works because a = T(x: ...) doesn't result in a C assignment where the RHS is of type T (a nimZeroMem plus assigning the fields individually is used instead).


Depending on how the rework of imported types goes, the example code will either be rejected by the NimSkull compiler, or compile and run successfully.

saem commented 2 weeks ago
  • typeRel considers cint and int32 to be equal (so that var x: cint = 0'i32 works)

Curious what you think, but should these be treated as equal? AFAIK, cint means a platform dependent sized integer-like, should that really be int32 or should it be an int?

zerbina commented 2 weeks ago

Curious what you think, but should these be treated as equal?

According to the current rules, yes, since cint is just a type alias defined like this:

type cint {.importc: "int", nodecl.} = int32

typeRel treats this type like any other alias, whereas hashType and sameTypeAux consider the sfImportc flag to be part of the type representation.

int32 is of course nonsense, since int in C is not guaranteed to really be a 32-bit integer.


Regarding what cint should be, my opinion is that is should be one of the following:

  1. an opaque imported type
  2. a fixed-size NimSkull integer type, with the correct size and alignment either derived from the selected target, or queried from the C compiler
saem commented 2 weeks ago

Curious what you think, but should these be treated as equal?

According to the current rules, yes, since cint is just a type alias defined like this:

type cint {.importc: "int", nodecl.} = int32

typeRel treats this type like any other alias, whereas hashType and sameTypeAux consider the sfImportc flag to be part of the type representation.

int32 is of course nonsense, since int in C is not guaranteed to really be a 32-bit integer.

Regarding what cint should be, my opinion is that is should be one of the following:

  1. an opaque imported type
  2. a fixed-size NimSkull integer type, with the correct size and alignment either derived from the selected target, or queried from the C compiler

OK, in which case we're on the same page, in that the current definition is nonsense. Additionally, I'm onboard with your definition (opaque plus compiler query).