Re: [PATCH 3/3] Let scrutinizer infer types for foreign types with retconv/argconv given

[Evan Hanson]

On 2019-12-26 15:51, megane wrote:
> It would be ideal if we could add some bits of type information and let
> the scrutinizer infer the rest. I think that would need some thought,
> and/or experimentation.
> 
> In these patches I dropped the annotations that added no information the
> scrutinizer couldn't infer. For example in (* * -> *) the only
> information is the arity of the function, which the scrutinizer can
> infer.

Indeed. I don't want to make perfect the enemy of good, so I've pushed
these. I do think we should try to recollapse/resimplify the code in
`annotate-foreign-procedure' if we can get the scrutinizer doing the
right thing with those annotations (or with the stub call sites, like
you suggest in the commit message), but this is a good improvement for
now. Thanks megane!

> There's only 1 return value from foreign functions, too (right?).

Right; C_values, which is the only way to return multiple values from
foreign code, only works with foreign-primitive.

> There should a way for the user to override (if compatible) the type for
> definitions. If the user specifies (: foo (-> *)) the return type of foo
> should be * even if the scrutinizer could infer the actual return type
> to be, say a fixnum. That gives more freedom for a library writer from
> an API design perspective.

Yeah, ATM `##core#the' nodes are kind of doing two different jobs; one
is carrying user specifiations, and the other is passing info from one
compiler phase to another. I guess in the first case we don't want to
change anything, but in the second we want to improve the info wherever
possible. Could the "strict" flag on those nodes be used for this?

Evan

Re: [PATCH 3/3] Let scrutinizer infer types for foreign types with retconv/argconv given

[megane]

Evan Hanson  writes:

> Hi megane, happy holidays!

Happy holidays, Evan and everyone!

>
> A quick question about this -- ought the scrutinizer try to improve the
> specificity of the ##core#the annotation, rather than having
> annotate-foreign-procedure skip emitting it entirely?

It would be ideal if we could add some bits of type information and let
the scrutinizer infer the rest. I think that would need some thought,
and/or experimentation.

In these patches I dropped the annotations that added no information the
scrutinizer couldn't infer. For example in (* * -> *) the only
information is the arity of the function, which the scrutinizer can
infer. There's only 1 return value from foreign functions, too (right?).

>
> ISTR it doesn't even look at the types in ##core#the nodes, but maybe it
> should? I'd guess that would simplify this code, and might also give
> benefits in other places, but maybe there's a reason it doesn't...

Scrutinizer does walk ##core#the nodes. There's the error message
r-type-mismatch-in-the for the situation where the annotation is
incompatible with the inferred type.

---

More on the ideal behaviour:

There should a way for the user to override (if compatible) the type for
definitions. If the user specifies (: foo (-> *)) the return type of foo
should be * even if the scrutinizer could infer the actual return type
to be, say a fixnum. That gives more freedom for a library writer from
an API design perspective.

>
> Evan

Re: [PATCH 3/3] Let scrutinizer infer types for foreign types with retconv/argconv given

[Evan Hanson]

Hi megane, happy holidays!

A quick question about this -- ought the scrutinizer try to improve the
specificity of the ##core#the annotation, rather than having
annotate-foreign-procedure skip emitting it entirely?

ISTR it doesn't even look at the types in ##core#the nodes, but maybe it
should? I'd guess that would simplify this code, and might also give
benefits in other places, but maybe there's a reason it doesn't...

Evan

[PATCH 3/3] Let scrutinizer infer types for foreign types with retconv/argconv given

[megane]

(_ _ _ _ . _))
-`(##core#the
-  (procedure ,(map (lambda (a)
-(chicken.compiler.support#foreign-type->scrutiny-type 
(car a) 'arg))
-   (chicken.syntax#strip-syntax (caddr form)))
- ,(chicken.compiler.support#foreign-type->scrutiny-type
-   (chicken.syntax#strip-syntax (cadr form)) 'result))
-  #f
-  (##core#foreign-safe-lambda* ,@(cdr form))
+(annotate-foreign-procedure `(##core#foreign-safe-lambda* ,@(cdr form))
+   (map car (caddr form))
+   (cadr form)
 
 (##sys#extend-macro-environment
  'foreign-type-size
-- 
2.17.1

>From 3a8f526f1a5f2af633a48f787efb2e4ce073d6e6 Mon Sep 17 00:00:00 2001
From: megane 
Date: Sun, 1 Dec 2019 09:50:18 +0200
Subject: [PATCH 2/3] * chicken-ffi-syntax.scm: Convert foreign-primitive to
 use annotate-foreign-procedure

---
 chicken-ffi-syntax.scm | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/chicken-ffi-syntax.scm b/chicken-ffi-syntax.scm
index e11a6a28..9e723910 100644
--- a/chicken-ffi-syntax.scm
+++ b/chicken-ffi-syntax.scm
@@ -217,8 +217,11 @@
   `(##core#the
 (procedure ,(map (cut chicken.compiler.support#foreign-type->scrutiny-type 
<> 'arg)
 (chicken.syntax#strip-syntax argtypes))
-  ,(chicken.compiler.support#foreign-type->scrutiny-type
-(chicken.syntax#strip-syntax rtype) 'result))
+  ,@(if rtype
+(list (chicken.compiler.support#foreign-type->scrutiny-type
+   (chicken.syntax#strip-syntax rtype) 'result))
+;; special case for C_values(...). Only triggered by 
foreign-primitive.
+'*))
 #f
 ,e))
 
@@ -245,17 +248,12 @@
   (lambda (form r c)
 (##sys#check-syntax 'foreign-primitive form '(_ _ . _))
 (let* ((hasrtype (and (pair? (cddr form)) (not (string? (caddr form)
-  (rtype (and hasrtype (chicken.syntax#strip-syntax (cadr form
-  (args (chicken.syntax#strip-syntax (if hasrtype (caddr form) (cadr 
form
+  (rtype (and hasrtype (cadr form)))
+  (args (if hasrtype (caddr form) (cadr form)))
   (argtypes (map car args)))
-  `(##core#the (procedure
-   ,(map (cut 
chicken.compiler.support#foreign-type->scrutiny-type <> 'arg)
- argtypes)
-   ,@(if (not rtype)
- '* ; special case for C_values(...)
- (list 
(chicken.compiler.support#foreign-type->scrutiny-type rtype 'result
-  #f
-  (##core#foreign-primitive ,@(cdr form)))
+  (annotate-foreign-procedure `(##core#foreign-primitive ,@(cdr form))
+ argtypes
+ rtype)
 
 (##sys#extend-macro-environment
  'foreign-lambda
-- 
2.17.1

>From bb9e1ff2a43518afa9959eee686d5a2f041c60ea Mon Sep 17 00:00:00 2001
From: megane 
Date: Sun, 1 Dec 2019 12:59:26 +0200
Subject: [PATCH 3/3] Let scrutinizer infer types for foreign types with
 retconv/argconv given

Not doing any annotation gives the scrutinizer a change to infer the
reconverted arguments. Which it in many cases can do.

For example this:

  (define-foreign-type retconverted-foreign-int int identity ->string)
  (foreign-lambda retconverted-foreign-int "rand")

Gets converted to something like this:

  (set! g14 chicken.string#->string)
  (lambda () (g14 (##core#inline stub23 (##core#undefined))

Which the scrutinizer can handle.

* chicken-ffi-syntax.scm (annotate-foreign-procedure): Don't annotate if 
scrutinizer can infer

  Ideally we could drop the annotation here completely if
  create-foreign-stub just annotated the return type of the stub
  call:

   (##core#inline stub25 (##core#undefined))
   =>
   (the fixnum (##core#inline stub25 (##core#undefined)))

  Generally the scrutinizer can infer the argument types if they
  are converted by enforcing functions like this:

  (lambda (int2730)
(##core#inline
 stub28
 (##core#undefined)
 (##sys#foreign-fixnum-argument int2730)))
  =>
  (fixnum -> *)

* tests/typematch-tests.scm: Expect more specific type now
---
 chicken-ffi-syntax.scm| 35 +--
 tests/typematch-tests.scm |  3 +--
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/chicken-ffi-syntax.scm b/chicken-ffi-syntax.scm
index 9e723910..40d879ac 100644
--- a/chicken-ffi-syntax.scm
+++ b/chicken-ffi-syntax.scm
@@ -214,16 +214,31 @@
 ;;; Aliases for internal forms
 
 (define (annotate-foreign-procedure e argtypes rtype)
-  `(##core#the
-(procedure ,(map (cut chicken.compiler.support#foreign-type->scrutiny-type 
<> 'arg)
-(chicken.syntax#strip-syntax argtypes))
-  ,@(if