Patch for wrong no declaration seen for command-line-arguments

Patch for wrong no declaration seen for command-line-arguments

[Matthieu Vachon]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

Hi Per,

Here my try at fixing the incorrect warning that occurs when
generating a main method and using the `command-line-arguments`. It's
probably not the optimal way to fix the issue but I hope it is good
enough to be included in the trunk.

I added two test cases but I'm unsure if the syntax I used is right.
Talking about that, what is the procedure to run the test suite? I
would like to know so I can test my patches when developing them prior
to sending them to you.

Regards,
Matt

[-- Attachment #2: fix-wrong-command-line-arguments-warning.patch --]
[-- Type: application/octet-stream, Size: 3437 bytes --]

Index: gnu/expr/ChangeLog
===================================================================
--- gnu/expr/ChangeLog	(revision 7584)
+++ gnu/expr/ChangeLog	 (working copy)
@@ -1,3 +1,10 @@
+2013-09-11  Matthieu Vachon <matthieu.o.vachon@gmail.com>
+
+    * FindCapturedVars.java (visitReferenceExp): Added a check
+    to see if unknown declaration will be bound at runtime
+    which is the case for command-line-arguments when a main
+    method needs to be generated.
+
 2013-09-07  Per Bothner  <per@bothner.com>
 
 	* PrimProcedure.java (mostSpecific): Moved from MethodProc.
Index: gnu/expr/FindCapturedVars.java
===================================================================
--- gnu/expr/FindCapturedVars.java	(revision 7584)
+++ gnu/expr/FindCapturedVars.java	 (working copy)
@@ -476,9 +476,10 @@ public class FindCapturedVars extends ExpExpVisitor<Void>
 				exp.isProcedureName());
 	exp.setBinding(decl);
       }
-    if (decl.getFlag(Declaration.IS_UNKNOWN))
+    if (decl.getFlag(Declaration.IS_UNKNOWN) && !isBoundAtRuntime(decl, comp)) {
       maybeWarnNoDeclarationSeen(exp.getSymbol(), exp.isProcedureName(),
                                  comp, exp);
+    }
 
     capture(exp.contextDecl(), decl);
     return exp;
@@ -540,4 +541,20 @@ public class FindCapturedVars extends ExpExpVisitor<Void>
     return super.visitSetExp(exp, ignored);
   }
 
+  /**
+   * Returns a boolean determining if a declaration is to be bound
+   * into the environment at runtime only. This is the case for the
+   * special command-line-arguments symbol which is bound to arguments
+   * only at runtime.
+   *
+   * @param declaration The declaration to check for runtime injection.
+   * @param compilation The compilation unit associated to the declaration.
+   *
+   * @return true if declaration will be bound at runtime, false otherwise.
+   */
+  boolean isBoundAtRuntime(Declaration declaration, Compilation compilation)
+  {
+    return compilation.generateMainMethod() &&
+           "command-line-arguments".equals(declaration.getName());
+  }
 }
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog	(revision 7584)
+++ testsuite/ChangeLog	 (working copy)
@@ -1,3 +1,13 @@
+2013-09-13  Matthieu Vachon  <matthieu.o.vachon@gmail.com>
+
+	* unknown1.scm: New test to check that no warning
+	is issued when generating a main method and using
+	command-line-arguments.
+
+	* unknown2.scm: New test to check that a warning
+	is issued when not generating a main method and using
+	command-line-arguments.
+
 2013-09-10  Matthieu Vachon  <matthieu.o.vachon@gmail.com>
 	    Per Bothner  <per@bothner.com>
 
Index: testsuite/unknown1.scm
===================================================================
--- testsuite/unknown1.scm	(revision 0)
+++ testsuite/unknown1.scm	 (working copy)
@@ -0,0 +1,5 @@
+(module-compile-options main: #t)
+
+;; No warning should be generated
+(format #t "Argument count: ~a~%" (vector-length command-line-arguments))
+;; Output: Argument count: 0
Index: testsuite/unknown2.scm
===================================================================
--- testsuite/unknown2.scm	(revision 0)
+++ testsuite/unknown2.scm	 (working copy)
@@ -0,0 +1,3 @@
+;; No main method, a warning should be issued
+(vector-length command-line-arguments)
+;; Diagnostic: unknown2.scm:2:16: warning - no declaration seen for command-line-arguments

Re: Patch for wrong no declaration seen for command-line-arguments

[Charles Turner]

Hi Matt,

I can't comment on the patch as I haven't been following along.

The test scripts you've written can be installed by adding the
filenames (in this case unknown1.scm and unknown2.scm) to the
testsuite/Makefile.am file.Specifically, add them to the variable
SCRIPTS_TO_RUN. You then need to regenerate the Makefiles using
autotools. I'm no expert, but aclocal && autoreconf has worked for me
in the past from the top-level kawa directory. If you then do a make
check, your tests will be run. For your patch, I got the following
results:

# unknown1.scm passes
FAIL unknown2.scm: expected more diagnostics: \Qunknown2.scm:2:16:
warning - no declaration seen for command-line-arguments\E
# unknown2.scm fails

Which will require further investigation.

Hope that helps,
Charles.


On 12/09/2013, Matthieu Vachon <matthieu.o.vachon@gmail.com> wrote:
> Hi Per,
>
> Here my try at fixing the incorrect warning that occurs when
> generating a main method and using the `command-line-arguments`. It's
> probably not the optimal way to fix the issue but I hope it is good
> enough to be included in the trunk.
>
> I added two test cases but I'm unsure if the syntax I used is right.
> Talking about that, what is the procedure to run the test suite? I
> would like to know so I can test my patches when developing them prior
> to sending them to you.
>
> Regards,
> Matt
>

Re: Patch for wrong no declaration seen for command-line-arguments

[Matthieu Vachon]

Hi Charles,

Thank you for the procedure. I'm going to check this out later tonight.

As for the failed tests, I'm going to take a look.

Regards,
Matt

On Thu, Sep 12, 2013 at 4:09 PM, Charles Turner <chturne@gmail.com> wrote:
> Hi Matt,
>
> I can't comment on the patch as I haven't been following along.
>
> The test scripts you've written can be installed by adding the
> filenames (in this case unknown1.scm and unknown2.scm) to the
> testsuite/Makefile.am file.Specifically, add them to the variable
> SCRIPTS_TO_RUN. You then need to regenerate the Makefiles using
> autotools. I'm no expert, but aclocal && autoreconf has worked for me
> in the past from the top-level kawa directory. If you then do a make
> check, your tests will be run. For your patch, I got the following
> results:
>
> # unknown1.scm passes
> FAIL unknown2.scm: expected more diagnostics: \Qunknown2.scm:2:16:
> warning - no declaration seen for command-line-arguments\E
> # unknown2.scm fails
>
> Which will require further investigation.
>
> Hope that helps,
> Charles.
>
>
> On 12/09/2013, Matthieu Vachon <matthieu.o.vachon@gmail.com> wrote:
>> Hi Per,
>>
>> Here my try at fixing the incorrect warning that occurs when
>> generating a main method and using the `command-line-arguments`. It's
>> probably not the optimal way to fix the issue but I hope it is good
>> enough to be included in the trunk.
>>
>> I added two test cases but I'm unsure if the syntax I used is right.
>> Talking about that, what is the procedure to run the test suite? I
>> would like to know so I can test my patches when developing them prior
>> to sending them to you.
>>
>> Regards,
>> Matt
>>

Re: Patch for wrong no declaration seen for command-line-arguments

[Per Bothner]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On 09/11/2013 08:42 PM, Matthieu Vachon wrote:
> Hi Per,
>
> Here my try at fixing the incorrect warning that occurs when
> generating a main method and using the `command-line-arguments`. It's
> probably not the optimal way to fix the issue but I hope it is good
> enough to be included in the trunk.

Thanks for prodding me into action.  Your patch makes me think
that having a special case for command-line-arguments is the wrong approach.

Please try the attached patch instead.  That is a simpler/cleaner
solution.

You might also consider using the (command-line) procedure.
It is specified by R6RS and R7RS so is more portable.
It also gives you the "command" used to invoke the application
(roughly argv[0]).  The attached patch provides a more informative
result for (car (command-line)) - rather than plain "kawa".
I'd be interested in feedback in how portable it is: Windows,
MacOS, non-Sun/Oracle VMs (including ones that don't set the
sun.java.command property).  (I also working on updating the
documentation.)
-- 
	--Per Bothner
per@bothner.com   http://per.bothner.com/

[-- Attachment #2: command-line-args.patch --]
[-- Type: text/x-patch, Size: 3513 bytes --]

Index: gnu/expr/ApplicationMainSupport.java
===================================================================
--- gnu/expr/ApplicationMainSupport.java	(revision 7583)
+++ gnu/expr/ApplicationMainSupport.java	(working copy)
@@ -43,7 +43,6 @@
   public static void setArgs (String[] args, int arg_start)
   {
     int nargs = args.length - arg_start;
-    Object[] array = new Object[nargs];
     if (arg_start == 0)
      commandLineArgArray = args;
     else
@@ -53,12 +52,10 @@
 	  strings[i] = args[i+arg_start];
 	commandLineArgArray = strings;
       }
-    for (int i = nargs;  --i >= 0; )
-      array[i] = new FString (args[i + arg_start]);
-    commandLineArguments = new FVector (array);  // FIXME scsh has list
-    // FIXME scsh also has command-line proc
-    Environment.getCurrent().put("command-line-arguments",
-                                 commandLineArguments);
+    
+    Object[] array = new Object[nargs];
+    System.arraycopy(args, arg_start, array, 0, nargs);
+    commandLineArguments = new ConstVector(array);  // FIXME scsh has list
   }
 
   public static boolean processSetProperty (String arg)
Index: kawa/lib/rnrs/programs.scm
===================================================================
--- kawa/lib/rnrs/programs.scm	(revision 7583)
+++ kawa/lib/rnrs/programs.scm	(working copy)
@@ -3,8 +3,32 @@
 (require <kawa.lib.prim_syntax>)
 
 (define (command-line) :: list
-  (let ((arg0 "kawa")) ;; FIXME
-    (cons arg0 (gnu.lists.LList:makeList gnu.expr.ApplicationMainSupport:commandLineArgArray 0))))
+  (let* ((rest
+          (gnu.lists.LList:makeList
+           gnu.expr.ApplicationMainSupport:commandLineArgArray 0))
+         (command ::java.lang.String
+                  (try-catch
+                   (let ((raw (java.lang.System:getProperty
+                               "sun.java.command")))
+                     (if (eq? raw #!null) #!null
+                         ;; Strip off the tail of the property value that
+                         ;; duplicates the rest value.
+                         (let* ((frest (format #f "~{ ~a~}" rest))
+                                (rlen (raw:length))
+                                (flen (frest:length))
+                                (alen (- rlen flen)))
+                           (cond ((= flen 0)
+                                  raw)
+                                 ;; Sanity check
+                                 ((and (>= alen 0)
+                                       ((raw:substring alen):equals frest))
+                                  (raw:substring 0 alen))
+                                 (else
+                                  #!null)))))
+                   (exp java.lang.Throwable #!null)))
+         (arg0 (if (eq? command #!null) "kawa"
+                   ("java ":concat command))))
+    (cons arg0 rest)))
 
 (define (exit #!optional (code 0)) :: #!void
   (invoke-static <output-port> 'runCleanups)
Index: kawa/standard/Scheme.java
===================================================================
--- kawa/standard/Scheme.java	(revision 7583)
+++ kawa/standard/Scheme.java	(working copy)
@@ -565,6 +565,8 @@
 
       defProcStFld("exit", "kawa.lib.rnrs.programs");
       defProcStFld("command-line", "kawa.lib.rnrs.programs");
+      defAliasStFld("command-line-arguments", 
+                    "gnu.expr.ApplicationMainSupport", "commandLineArguments");
 
       defProcStFld("bitwise-arithmetic-shift",
                    "gnu.kawa.functions.BitwiseOp", "ashift");

Re: Patch for wrong no declaration seen for command-line-arguments

[Jamison Hope]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Sep 16, 2013, at 1:48 AM, Per Bothner <per@bothner.com> wrote:

> The attached patch provides a more informative
> result for (car (command-line)) - rather than plain "kawa".
> I'd be interested in feedback in how portable it is: Windows,
> MacOS, non-Sun/Oracle VMs (including ones that don't set the
> sun.java.command property).  (I also working on updating the
> documentation.)

With Oracle JDK 7u40 on Mac OS X 10.8, I get:

$ java -jar kawa-1.13.1.jar 
#|kawa:1|# (write (car (command-line)))
"java kawa-1.13.1.jar"

$ java kawa.repl
#|kawa:1|# (write (car (command-line)))
"java kawa.repl"

$ cat /tmp/test.scm 
(write (car (command-line)))
(newline)

$ java -jar kawa-1.13.1.jar -f /tmp/test.scm 
"java kawa-1.13.1.jar -f /tmp/test.scm"

$ java kawa.repl -f /tmp/test.scm 
"java kawa.repl -f /tmp/test.scm"

In other words, it seems to do what you'd expect.

--
Jamison Hope
The PTR Group
www.theptrgroup.com




[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 204 bytes --]

Re: Patch for wrong no declaration seen for command-line-arguments

[Matthieu Vachon]

Hi Per,

I tested your patch and it is really more clearer than what I first
proposed. Very clean.

No more `command-line-arguments` warning when using the patch.

As for the new information for (car (command-line)), it ran the same
tests Jamison did but with the platforms and Java version I had access
to. The raw results can be found at the end of the email. From my
tests, java 5 support is not working correctly. It seems that property
`sun.java.command` is not available on Java 5:

    $ java kawa.repl
    #|kawa:1|# (java.lang.System:getProperty "sun.java.command")
    #!null

Did not searched that much but did not found any information on
"sun.java.command" for java 5.

Regards,
Matt

P.S. The results:

Windows 7 x64 - Java 1.5.0_22 x64
Kawa compiled with Java 1.5.0_22 x64
=========================================
$ java -jar kawa-1.13.1.jar
#|kawa:1|# (write (car (command-line)))
"kawa"

$ java kawa.repl
#|kawa:1|# (write (car (command-line)))
"kawa"

$ java -jar kawa-1.13.1.jar -f test.scm
"kawa"

$ java kawa.repl -f test.scm
"kawa"

Windows 7 x64 - Java 1.6.0_38 x64
Kawa compile with Java 1.6.0_38 x64
=========================================
$ java -jar kawa-1.13.1.jar
#|kawa:1|# (write (car (command-line)))
"java kawa-1.13.1.jar"

$ java kawa.repl
#|kawa:1|# (write (car (command-line)))
"java kawa.repl"

$ java -jar kawa-1.13.1.jar -f test.scm
"java kawa-1.13.1.jar -f test.scm"

$ java kawa.repl -f test.scm
"java kawa.repl -f test.scm"

Windows 7 x64 - Java 1.7.0.11 x64
Kawa compile with Java 1.7.0.11 x64
=========================================
$ java -jar kawa-1.13.1.jar
#|kawa:1|# (write (car (command-line)))
"java kawa-1.13.1.jar"

$ java kawa.repl
#|kawa:1|# (write (car (command-line)))
"java kawa.repl"

$ java -jar kawa-1.13.1.jar -f test.scm
"java kawa-1.13.1.jar -f test.scm"

$ java kawa.repl -f test.scm
"java kawa.repl -f test.scm"

CentOS 6.3 x64 - Java 1.6.0_43 x64
Kawa compile with Java 1.6.0_43 x64
=========================================
$ java -jar kawa-1.13.1.jar
#|kawa:1|# (write (car (command-line)))
"java kawa-1.13.1.jar"

$ java kawa.repl
#|kawa:1|# (write (car (command-line)))
"java kawa.repl"

$ java -jar kawa-1.13.1.jar -f test.scm
"java kawa-1.13.1.jar -f test.scm"

$ java kawa.repl -f test.scm
"java kawa.repl -f test.scm"

CentOS 6.3 x64 - OpenJDK 1.6.0_24 x64 (IcedTea6 1.11.11.90)
Kawa compile with OpenJDK 1.6.0_24 x64 (IcedTea6 1.11.11.90)
=========================================
$ java -jar kawa-1.13.1.jar
#|kawa:1|# (write (car (command-line)))
"java kawa-1.13.1.jar"

$ java kawa.repl
#|kawa:1|# (write (car (command-line)))
"java kawa.repl"

$ java -jar kawa-1.13.1.jar -f test.scm
"java kawa-1.13.1.jar -f test.scm"

$ java kawa.repl -f test.scm
"java kawa.repl -f test.scm"

Re: Patch for wrong no declaration seen for command-line-arguments

[Per Bothner]

On 09/18/2013 06:19 AM, Matthieu Vachon wrote:
> I tested your patch and it is really more clearer than what I first
> proposed. Very clean.
>
> No more `command-line-arguments` warning when using the patch.

Thanks - I checked the patch in.
-- 
	--Per Bothner
per@bothner.com   http://per.bothner.com/