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

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

Open
mrgaturus opened this issue Aug 23, 2024 · 4 comments
Labels
bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler

Comments

@mrgaturus
Copy link
Contributor

mrgaturus commented Aug 23, 2024

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 zerbina added bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler labels Aug 23, 2024
@zerbina
Copy link
Collaborator

zerbina commented Aug 23, 2024

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

  • typeRel considers cint and int32 to be equal (so that var x: cint = 0'i32 works)
  • sameTypeAux (used for comparing generic invocation arguments) considers cint and int32 to be different, hence two instantiations of MyValue being created
  • hashType consider cint and int32 to be different, hence the instantiations not being merged

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
Copy link
Collaborator

saem commented Aug 24, 2024

  • 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
Copy link
Collaborator

zerbina commented Aug 24, 2024

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
Copy link
Collaborator

saem commented Aug 24, 2024

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).

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
Projects
None yet
Development

No branches or pull requests

3 participants