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

Compiler error does not provide location of error when crossing package boundaries #3997

Closed
redvers opened this issue Feb 6, 2022 · 10 comments · Fixed by #4065
Closed

Compiler error does not provide location of error when crossing package boundaries #3997

redvers opened this issue Feb 6, 2022 · 10 comments · Fixed by #4065
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@redvers
Copy link
Contributor

redvers commented Feb 6, 2022

This is a weird one, an edge case - but I'm documenting it here as it may become relevant at some point.

When you have a class in DirectoryA use an Interface in DirectoryB, and the Interface in DirectoryB makes an FFI call, DirectoryA needs to have a copy of the FFI use declarations.

The compiler complaining is completely appropriate, the reason for documenting the issue is that the error message from the compiler gives no reference to the actual problem at all.

The compiler produced pages and pages of this:

/home/red/projects/pony/GTK-prims/pony-gir/test/GObject/GObject.pony:20:6: An FFI call needs a declaration
    @g_object_get_data(apply(), key.cstring())
     ^
Error:
/home/red/projects/pony/GTK-prims/pony-gir/test/GObject/GObject.pony:23:6: An FFI call needs a declaration
    @g_object_set_data(apply(), key.cstring(), data)
     ^
Error:
/home/red/projects/pony/GTK-prims/pony-gir/test/GObject/GObject.pony:20:6: An FFI call needs a declaration
    @g_object_get_data(apply(), key.cstring())
     ^
Error:
/home/red/projects/pony/GTK-prims/pony-gir/test/GObject/GObject.pony:23:6: An FFI call needs a declaration
    @g_object_set_data(apply(), key.cstring(), data)
     ^

The file itself looks like this:

use @g_object_get_data[Pointer[None]](gobject: Pointer[GObject] tag, key: Pointer[U8] tag)
use @g_object_set_data[None](gobject: Pointer[GObject] tag, key: Pointer[U8] tag, data: Pointer[None] tag)

class GObject is GObjectInterface
  var _ptr: Pointer[GObject] tag

  new from_ptr(ptr: Pointer[GObject] tag) => _ptr = ptr

  fun apply(): Pointer[GObject] tag => _ptr

interface GObjectInterface
  fun apply(): Pointer[GObject] tag

  fun g_object_get_data(key: String): Pointer[None] =>
    @g_object_get_data(apply(), key.cstring())

  fun g_object_set_data(key: String, data: Pointer[None] tag): None =>
    @g_object_set_data(apply(), key.cstring(), data)

Methinks this will likely be resolved by the RFC that's currently open.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 6, 2022
@redvers
Copy link
Contributor Author

redvers commented Feb 6, 2022

If I drop:

use "../GObject"

use @g_object_get_data[Pointer[None]](gobject: Pointer[GObject] tag, key: Pointer[U8] tag)
use @g_object_set_data[None](gobject: Pointer[GObject] tag, key: Pointer[U8] tag, data: Pointer[None] tag)

… in the other directory, everything compiles appropriately.

@ergl
Copy link
Member

ergl commented Feb 6, 2022

Yes, the problem here is that "use" declarations are only valid in the local package.

Since your interface has default methods, the ast gets copied to the package that declares the class. The compiler can't really point to a line in your file because they don't exist. We should make the error more explicit so that it says something like "the error is in a default method of an interface you're implementing".

@SeanTAllen
Copy link
Member

I believe this is #2150 although there could be a bit of FFI specific-ness.

@SeanTAllen SeanTAllen added bug Something isn't working and removed discuss during sync Should be discussed during an upcoming sync labels Feb 15, 2022
@ergl
Copy link
Member

ergl commented Feb 15, 2022

Yes, there will probably be some things to iron out if we import FFI declarations into other package, but we can probably tackle that after fixing #2150

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 15, 2022
@ergl ergl removed the discuss during sync Should be discussed during an upcoming sync label Feb 15, 2022
@SeanTAllen
Copy link
Member

SeanTAllen commented Feb 21, 2022

I have a fix for #2150. @redvers is there a small minimal example I can try out on this?

Otherwise, can you take the branch for #4027 and do a build and test to see if this issue is fixed for you? Or if #4027 has been merged and is part of a nightly, use that nightly to test.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 21, 2022
@redvers
Copy link
Contributor Author

redvers commented Feb 22, 2022

I can build a quick minimal example - no problem. I'll also test locally.

@redvers
Copy link
Contributor Author

redvers commented Feb 24, 2022

It was not successful:

red@ub0:~/projects/minimal-case-3997$ ../ponyc/build/release/ponyc
Building builtin -> /home/red/projects/ponyc/packages/builtin
Building . -> /home/red/projects/minimal-case-3997
Building A -> /home/red/projects/minimal-case-3997/A
Building ../B -> /home/red/projects/minimal-case-3997/B
Error:
/home/red/projects/minimal-case-3997/B/B.pony:6:6: An FFI call needs a declaration
    @printf("Success\n".cstring())
     ^
Error:
/home/red/projects/minimal-case-3997/B/B.pony:6:6: An FFI call needs a declaration
    @printf("Success\n".cstring())
     ^

I'm uploading this minimal case for you now for testing.

@redvers
Copy link
Contributor Author

redvers commented Feb 24, 2022

See attached:
minimal-case-3997.tar.gz

@SeanTAllen
Copy link
Member

There's two solutions to this problem.

  1. Look up the FFI declaration in the package it came from using the same mechanism we use for other method body symbol lookup.

This is somewhat complicated by the fact that technically, we only allow one declaration of a given FFI function per-package. So, how do we deal with that? Do we say, you can only declare once, but you can use different declarations that get pulled in from different packages?

  1. Don't allow FFI calls in default method bodies. Require the FFI calls to be put on a primitive that lives in the same package. Then when the default method body is brought in, it will look up the primitive in the usual expected way and we don't need to mess with the complications of solution 1.

Solution two is easier to maintain. It has less cognitive overhead for implementers and for the programmer, but it does require a bit more code and work from the developer per FFI method to call from the default method. I think the wrapping FFI calls in primitives is a generally good idea that I would favor advocating, but the more methods you are wrapping, the more arduous it becomes.

@ponylang/committer any preference between solution 1 or 2.

@ergl ergl removed the discuss during sync Should be discussed during an upcoming sync label Mar 1, 2022
@SeanTAllen
Copy link
Member

We decided during sync that #2 is the better option for a "pragmatic fix" for this.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Mar 1, 2022
@SeanTAllen SeanTAllen added good first issue Good for newcomers help wanted Extra attention is needed and removed discuss during sync Should be discussed during an upcoming sync labels Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants