Opened 3 years ago

Closed 9 months ago

#5610 closed feature request (fixed)

Improve "Unacceptable argument type in foreign declaration" error message

Reported by: bgamari Owned by:
Priority: high Milestone:
Component: Compiler (Type checker) Version: 7.6.1-rc1
Keywords: Cc: ghc@…, bos@…, malaquias@…, rwbarton@…, shelarcy@…, vogt.adam@…, fcsernik@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

Using ghc 1ece7b27a11c6947f0ae3a11703e22b7065a6b6c, zlib fails to build,

$ cabal install zlib
Resolving dependencies...
Configuring zlib-0.5.3.1...
Preprocessing library zlib-0.5.3.1...
Building zlib-0.5.3.1...
[1 of 5] Compiling Codec.Compression.Zlib.Stream ( dist/build/Codec/Compression/Zlib/Stream.hs, dist/build/Codec/Compression/Zlib/Stream.o )

Codec/Compression/Zlib/Stream.hsc:857:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h inflateInit2_" c_inflateInit2_
        :: StreamState -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:857:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h inflateInit2_" c_inflateInit2_
        :: StreamState -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:857:1:
    Unacceptable result type in foreign declaration: IO CInt
    Safe Haskell is on, all FFI imports must be in the IO monad
    When checking declaration:
      foreign import ccall unsafe "static zlib.h inflateInit2_" c_inflateInit2_
        :: StreamState -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:865:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h inflate" c_inflate
        :: StreamState -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:865:1:
    Unacceptable result type in foreign declaration: IO CInt
    Safe Haskell is on, all FFI imports must be in the IO monad
    When checking declaration:
      foreign import ccall unsafe "static zlib.h inflate" c_inflate
        :: StreamState -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:872:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h deflateInit2_" c_deflateInit2_
        :: StreamState
           -> CInt
              -> CInt -> CInt -> CInt -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:872:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h deflateInit2_" c_deflateInit2_
        :: StreamState
           -> CInt
              -> CInt -> CInt -> CInt -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:872:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h deflateInit2_" c_deflateInit2_
        :: StreamState
           -> CInt
              -> CInt -> CInt -> CInt -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:872:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h deflateInit2_" c_deflateInit2_
        :: StreamState
           -> CInt
              -> CInt -> CInt -> CInt -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:872:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h deflateInit2_" c_deflateInit2_
        :: StreamState
           -> CInt
              -> CInt -> CInt -> CInt -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:872:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h deflateInit2_" c_deflateInit2_
        :: StreamState
           -> CInt
              -> CInt -> CInt -> CInt -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:872:1:
    Unacceptable result type in foreign declaration: IO CInt
    Safe Haskell is on, all FFI imports must be in the IO monad
    When checking declaration:
      foreign import ccall unsafe "static zlib.h deflateInit2_" c_deflateInit2_
        :: StreamState
           -> CInt
              -> CInt -> CInt -> CInt -> CInt -> Ptr CChar -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:884:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static zlib.h deflate" c_deflate
        :: StreamState -> CInt -> IO CInt

Codec/Compression/Zlib/Stream.hsc:884:1:
    Unacceptable result type in foreign declaration: IO CInt
    Safe Haskell is on, all FFI imports must be in the IO monad
    When checking declaration:
      foreign import ccall unsafe "static zlib.h deflate" c_deflate
        :: StreamState -> CInt -> IO CInt
cabal: Error: some packages failed to install:
zlib-0.5.3.1 failed during the building phase. The exception was:
ExitFailure 1

Change History (25)

comment:1 Changed 3 years ago by bgamari

While my knowledge of the restrictions imposed by Safe Haskell is quite limited, judging from the error messages it seems the declarations should be permissible (i.e. IO CInt is in the IO monad). Could this be due to the compiler?

comment:2 Changed 3 years ago by bgamari

According to Johan Tibell on haskell-cafe,

This is due to a change in how FFI imports and newtypes work. GHC was
recently changed to not allow you to use newtypes in FFI imports unless the
constructor of the newtype is in scope. This broke quite a few libraries. I
have patched a few of them and I've sent a patch to the zlib maintainer.

-- Johan

comment:3 Changed 3 years ago by igloo

  • Milestone set to 7.4.1
  • Priority changed from normal to high

Yes, the failure isn't a GHC bug, but the error message could be better.

We shouldn't be saying

Safe Haskell is on, all FFI imports must be in the IO monad

as that's not the problem, and it would be better if we also said

Unacceptable argument type in foreign declaration: CInt
(it is a newtype, but its constructor is not in scope)

Unfortunately, it looks like being able to give better errors will mean a little refactoring.

But if we're going to do it then we should do it before 7.4.1, as that's when people will run into it the most (as existing code will start giving these errors).

comment:4 Changed 3 years ago by igloo

  • Summary changed from zlib fails to build due to Safe Haskell on GHC master to Improve "Unacceptable argument type in foreign declaration" error message

comment:5 Changed 3 years ago by simonmar

  • Owner set to simonmar

comment:6 Changed 3 years ago by simonmar

  • Resolution set to fixed
  • Status changed from new to closed

We changed tack on this one:

commit c6b0fd62fc715aa6c666eb8afe09073ac7b87a83

Author: Simon Marlow <[email protected]>
Date:   Thu Nov 24 14:04:23 2011 +0000

    Relax the restriction on using abstract newtypes in FFI declarations.
    
    Given the high impact of this change, we decided to back off and make
    abstract newtypes give a warning for one release, before we make it an
    error in 7.6.1.
    
    Codec/Compression/Zlib/Stream.hsc:884:1:
        Warning: newtype `CInt' is used in an FFI declaration,
                 but its constructor is not in scope.
                 This will become an error in GHC 7.6.1.
        When checking declaration:
          foreign import ccall unsafe "static zlib.h deflate" c_deflate
            :: StreamState -> CInt -> IO CInt

comment:7 Changed 3 years ago by Lemming

And what is the proposed way to avoid this warning? As far as I know, the constructor for CInt is hidden. But it is still suggested style to use CInt in a foreign import?

comment:8 follow-up: Changed 3 years ago by simonmar

  • difficulty set to Unknown

The constructor for CInt is not hidden, just import CInt(..).

comment:9 in reply to: ↑ 8 Changed 3 years ago by Lemming

Replying to simonmar:

The constructor for CInt is not hidden, just import CInt(..).

I looks like CInt constructor is imported since GHC-7.4 but was hidden until GHC-7.2. Importing CInt(..) gives a warning on GHC-7.2 whereas importing CInt gives a warning on GHC-7.4. Hm. Maybe I should import the types with qualification.

Btw. if the type would be named Int in the module Foreign.C.Types I would already use qualification and write C.Int.

comment:10 Changed 3 years ago by Lemming

  • Cc ghc@… added
  • Component changed from libraries (other) to Compiler (Type checker)
  • Type of failure changed from None/Unknown to GHC rejects valid program

Maybe that's not the right place to report my problem ...

Let me summarize:
GHC-6.12 accepted foreign imports containing CInt and similar types without having access to the CInt constructor. The CInt constructor was even not exported by Foreign.C.Types.
In contrast to that, GHC-7.4 expects that the CInt constructor is in scope and it is now also exported by Foreign.C.Types.

I have several places where I wrap around CInt like this:
newtype Counter = Counter CInt.

Consider a module where I use Counter in a foreign call declaration:

module M where

import A (Counter)

foreign import ccall "c_func" func :: IO Counter

This would work with GHC-6.12.
However GHC-7.4 not only requires me to import the Counter constructor,
but also the CInt constructor.

This is formally correct but becomes annoying, because CInt is not used explicitly in M.
Thus in GHC-6.12 I get "unused import" warnings that I can only avoid by dummy uses
like _dummy :: C.CInt; _dummy = undefined.
In GHC-7.4 I do not get these warnings because the CInt constructor is used implicitly.

I wonder whether the requirement for importing the constructor can be dropped, when instead arguments and results of foreign functions must be in a type class like Storable. We may call it Foreign. Then instead of importing constructors explicitly, the packing and unpacking would be encoded in an instance of Foreign and this instance would be imported automatically. This type class could be automatically derivable for newtypes and would otherwise enable the user to write instances for sophisticated types.

This is currently for me an issue because I try to write code that can be compiled without warnings from GHC-6.12 to GHC-7.4. Maybe there are other good reasons to use a type class instead of implicit unwrapping and wrapping of arguments to foreign functions.

comment:11 Changed 3 years ago by bos

  • Owner simonmar deleted
  • Resolution fixed deleted
  • Status changed from closed to new

Here is the error message reported by GHC 7.6.1 rc1:

Data/Text/ICU/Error/Internal.hsc:163:1:
    Unacceptable argument type in foreign declaration: CInt
    When checking declaration:
      foreign import ccall unsafe "static hs_text_icu.h __hs_u_errorName" u_errorName
        :: UErrorCode -> CString

This is the same message as the old error, and is very confusing.

Can the much more comprehensible and actionable language from the 7.4 warning please be reintroduced?

        Error: newtype `CInt' is used in an FFI declaration,
                 but its constructor is not in scope.

comment:12 Changed 3 years ago by bos

  • Architecture changed from x86_64 (amd64) to Unknown/Multiple
  • Cc bos@… added
  • Keywords zlib removed
  • Operating System changed from Linux to Unknown/Multiple
  • Type changed from bug to feature request
  • Type of failure changed from GHC rejects valid program to Incorrect warning at compile-time

comment:13 Changed 3 years ago by bos

  • Version changed from 7.3 to 7.6.1-rc1

comment:14 Changed 3 years ago by bos

Incidentally, Lemming's observation that requiring constructors to be imported breaks encapsulation seems correct.

I have run into exactly the situation he describes, where an aliased CInt is now causing compilation of a number of modules to break in 7.6. I have to fix this by importing both the alias and CInt, which feels quite unsatisfactory.

Furthermore, making compilation work with GHC 7.6 now introduces warnings on versions of GHC prior to 7.4:

Data/Text/ICU/Normalize.hsc:45:1:
    Warning: The import item `CInt(..)' suggests that
             `CInt' has (in-scope) constructors or class methods,
             but it has none

I make a habit of building with -Werror on my continuous integration host, but the calisthenics required to keep things warning-clean across multiple GHC versions are getting a bit rigorous.

comment:15 Changed 3 years ago by simonpj

See also #7243

comment:16 Changed 3 years ago by romildo

  • Cc malaquias@… added

comment:17 Changed 20 months ago by rwbarton

  • Cc rwbarton@… added

comment:18 Changed 20 months ago by shelarcy

  • Cc shelarcy@… added

comment:19 Changed 19 months ago by aavogt

  • Cc vogt.adam@… added

comment:20 Changed 11 months ago by archblob

I have time to work on this and do a little refactoring to improve the error messages. The comment history is a little confusing and I'm not really sure that a better error message is the only thing requested here, and if the whole importing of constructors for foreign declarations story is also in need of fixing. It's the same story with #7243, if someone could make things clear for me I would be happy to start working.

comment:21 Changed 11 months ago by archblob

  • Cc fcsernik@… added

comment:22 Changed 10 months ago by ezyang

  • Milestone 7.4.1 deleted

comment:23 Changed 10 months ago by mietek

With 7.8.2, using import Foreign.C.Types (CInt) still results in the unclear error message:

Unacceptable argument type in foreign declaration: CInt

Using import Foreign.C.Types (CInt (..)) helps, but should this be required?

comment:24 Changed 9 months ago by Simon Peyton Jones <simonpj@…>

In 92587bfefea2b78f89bcdad27e0da5711463fd1b/ghc:

Refactor FFI error messages

This patch was provoked by Trac #5610, which I finally got a moment to look at.

In the end I added a new data type ErrUtils.Validity,

  data Validity
    = IsValid            -- Everything is fine
    | NotValid MsgDoc    -- A problem, and some indication of why

with some suitable combinators, and used it where appropriate (which touches
quite a few modules).  The main payoff is that error messages improve for
FFI type validation.

comment:25 Changed 9 months ago by simonpj

  • Resolution set to fixed
  • Status changed from new to closed

OK I have at least improved the error messages, so I'll close this.

Simon

Note: See TracTickets for help on using tickets.