bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Mark Oteiza]

Hi,

I recently got the following from metar.el using KPHL:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  intern(nil)
  metar-convert-temperature("M01")
  metar-dewpoint("KPHL 172054Z 28015G23KT 10SM BKN046 10/M01 A2993 RMK AO2 PK WND 28029/2044 SLP135 T01001006 53013")
  metar-decode(((21649 60776) . "KPHL 172054Z 28015G23KT 10SM BKN046 10/M01 A2993 RMK AO2 PK WND 28029/2044 SLP135 T01001006 53013"))
  metar(1)
  funcall-interactively(metar 1)
  call-interactively(metar record nil)
  command-execute(metar record)
  execute-extended-command(nil "metar" nil)
  funcall-interactively(execute-extended-command nil "metar" nil)
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Ulf Jasper]

Mark Oteiza <mvoteiza@udel.edu> writes:

> Hi,
>
> I recently got the following from metar.el using KPHL:
>
> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>   intern(nil)
>   metar-convert-temperature("M01")
>   metar-dewpoint("KPHL 172054Z 28015G23KT 10SM BKN046 10/M01 A2993 RMK AO2 PK WND 28029/2044 SLP135 T01001006 53013")
>   metar-decode(((21649 60776) . "KPHL 172054Z 28015G23KT 10SM BKN046 10/M01 A2993 RMK AO2 PK WND 28029/2044 SLP135 T01001006 53013"))
>   metar(1)
>   funcall-interactively(metar 1)
>   call-interactively(metar record nil)
>   command-execute(metar record)
>   execute-extended-command(nil "metar" nil)
>   funcall-interactively(execute-extended-command nil "metar" nil)
>   call-interactively(execute-extended-command nil nil)
>   command-execute(execute-extended-command)

When I do `C-u C-u M-x metar RET KPHL RET' with Emacs built from current
master and metar 0.1 from GNU Elpa I get

   30 minutes ago at PHILADELPHIA INTERNATIONAL  AIRPT: 3°C, 48% humidity, 1019.3 hPa.

What versions are you using?

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Mark Oteiza]

Ulf Jasper <ulf.jasper@web.de> writes:

> Mark Oteiza <mvoteiza@udel.edu> writes:
>
>> Hi,
>>
>> I recently got the following from metar.el using KPHL:
>>
>> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>>   intern(nil)
>>   metar-convert-temperature("M01")
>>   metar-dewpoint("KPHL 172054Z 28015G23KT 10SM BKN046 10/M01 A2993 RMK AO2 PK WND 28029/2044 SLP135 T01001006 53013")
>>   metar-decode(((21649 60776) . "KPHL 172054Z 28015G23KT 10SM BKN046 10/M01 A2993 RMK AO2 PK WND 28029/2044 SLP135 T01001006 53013"))
>>   metar(1)
>>   funcall-interactively(metar 1)
>>   call-interactively(metar record nil)
>>   command-execute(metar record)
>>   execute-extended-command(nil "metar" nil)
>>   funcall-interactively(execute-extended-command nil "metar" nil)
>>   call-interactively(execute-extended-command nil nil)
>>   command-execute(execute-extended-command)
>
> When I do `C-u C-u M-x metar RET KPHL RET' with Emacs built from current
> master and metar 0.1 from GNU Elpa I get
>
>    30 minutes ago at PHILADELPHIA INTERNATIONAL  AIRPT: 3°C, 48% humidity, 1019.3 hPa.
>
> What versions are you using?

The same; emacs from master and metar 0.1 from ELPA.  It works at this
time when CODES in (metar-decode) is:

  "KPHL 181354Z 29012KT 10SM BKN041 03/M07 A3010 RMK AO2 SLP190 T00281067"

but for some reason the following from yesterday was bad input for metar.el:

  "KPHL 172054Z 28015G23KT 10SM BKN046 10/M01 A2993 RMK AO2 PK WND 28029/2044 SLP135 T01001006 53013"

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Ulf Jasper]

Mark Oteiza <mvoteiza@udel.edu> writes:

> The same; emacs from master and metar 0.1 from ELPA.  It works at this
> time when CODES in (metar-decode) is:
>
>   "KPHL 181354Z 29012KT 10SM BKN041 03/M07 A3010 RMK AO2 SLP190 T00281067"
>
> but for some reason the following from yesterday was bad input for metar.el:
>
>   "KPHL 172054Z 28015G23KT 10SM BKN046 10/M01 A2993 RMK AO2 PK WND 28029/2044 SLP135 T01001006 53013"

It appears that the temperature value -1 °C ("M01") is causing the error

    (metar-convert-temperature "M07") => (-7 . degC)
    (metar-convert-temperature "M01") => error (wrong-type-argument stringp nil)

I suspect that this is the result of `math-mul-symb-fancy' (which
metar.el calls via calc-units) treating "-1" in a special way

    (math-mul-symb-fancy -7 '(var degC var-degC)) => (* -7 (var degC var-degC))
    (math-mul-symb-fancy -1 '(var degC var-degC)) => (neg (var degC var-degC))

I have never looked at the calc library before and feel unable to cope
with it.  Hopefully someone else can stand in.

ulf

    

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Wolfgang Jenkner]

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

On Thu, Dec 18 2014, Ulf Jasper wrote:

> It appears that the temperature value -1 °C ("M01") is causing the error
>
>     (metar-convert-temperature "M07") => (-7 . degC)
>     (metar-convert-temperature "M01") => error (wrong-type-argument stringp nil)
>
> I suspect that this is the result of `math-mul-symb-fancy' (which
> metar.el calls via calc-units) treating "-1" in a special way
>
>     (math-mul-symb-fancy -7 '(var degC var-degC)) => (* -7 (var degC var-degC))
>     (math-mul-symb-fancy -1 '(var degC var-degC)) => (neg (var degC var-degC))

There are problems in calc (Bug#19582) here.  There's also a small
problem in metar, viz., the string "-degC" is not parsed in the same way
as "-1 degC".

Since `metar-convert-unit' has (at least potentially) the same problem,
the attached patch does a little refactoring of those two functions,
where the actual conversion function from the calc library is passed as
an additional optional argument to `metar-convert-unit', so that
`metar-convert-temperature' reduces to just a call to the latter.

The actual bug is simply resolved by passing t as the last argument to
`math-convert-units' resp. `math-convert-temperature', so that these
functions return the number without a unit.  After all, we already know
which unit we get...

Please note that this patch assumes that the patch from (Bug#19582) has
been applied.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: M01 bug fix --]
[-- Type: text/x-diff, Size: 3812 bytes --]

From 4d72d28ead8f864fad08a3d05f216de940be6b8f Mon Sep 17 00:00:00 2001
From: Wolfgang Jenkner <wjenkner@inode.at>
Date: Sat, 3 Jan 2015 00:34:33 +0100
Subject: [PATCH] [metar] Fix the case where the metar record contains M01
 (bug#19401).

* packages/metar/metar.el (metar-convert-unit): New optional argument.
(metar-convert-temperature): Use it to rewrite this function in terms of
metar-convert-unit.
Pass t as last argument to the underlying calc conversion functions so
that they return only the number, not the unit, thereby fixing the bug
in question.
---
 packages/metar/metar.el | 66 ++++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/packages/metar/metar.el b/packages/metar/metar.el
index 7b78f07..9575313 100644
--- a/packages/metar/metar.el
+++ b/packages/metar/metar.el
@@ -239,45 +239,43 @@ If no match if found, nil is returned."
       (when station-code
 	(cons station-code (round best-distance))))))
 
-(defun metar-convert-unit (value new-unit)
+(defun metar-convert-unit (value new-unit &optional convert-units-function)
   "Convert VALUE to NEW-UNIT.
 VALUE is a string with the value followed by the unit, like \"5 knot\"
-and NEW-UNIT should be a unit name like \"kph\" or similar."
+and NEW-UNIT should be a unit name like \"kph\" or similar.
+CONVERT-UNITS-FUNCTION designates the function actually doing the conversion.
+It must have the signature of `math-convert-units', which is the default."
   (cl-check-type value string)
-  (cl-check-type new-unit (or string symbol))
-  (cl-multiple-value-bind (value unit)
-      (split-string
-       (math-format-value
-	(math-convert-units (math-simplify (math-read-expr value))
-			    (math-read-expr
-			     (cl-etypecase new-unit
-					   (string new-unit)
-					   (symbol (symbol-name new-unit))))))
-       " ")
-    (cons (string-to-number value) (intern unit))))
+  (unless (symbolp new-unit)
+    (setq new-unit (intern new-unit)))
+  (let ((expr (math-simplify (math-read-expr value))))
+    (cl-assert (or (math-zerop expr)
+		   (not (memq (math-single-units-in-expr-p expr) '(nil wrong))))
+	       nil
+	       "Metar: Not exactly one unit in expression: %S" expr)
+    (let ((res (math-simplify-units
+		(funcall (or convert-units-function 'math-convert-units)
+			 expr
+			 (math-build-var-name new-unit)
+			 t))))
+      (cl-assert (math-realp res) nil
+		 "Metar: Not a Calc real number: %S" res)
+      (cons (string-to-number (math-format-value (if (integerp res)
+						     res
+						   (math-float res))))
+	    new-unit))))
 
 (defun metar-convert-temperature (string &optional unit)
-  (let* ((value (concat (if (= (aref string 0) ?M)
-			    (concat "-" (substring string 1))
-			  string)
-			"degC"))
-	 (expr (math-read-expr value))
-	 (old-unit (math-single-units-in-expr-p expr))
-	 (new-unit (or unit (cdr (assq 'temperature metar-units)))))
-    (if old-unit
-	(cl-multiple-value-bind (value unit)
-	    (split-string
-	     (math-format-value
-	      (math-simplify-units
-	       (math-convert-temperature
-		expr
-		(list 'var
-		      (car old-unit)
-		      (intern (concat "var-" (symbol-name (car old-unit)))))
-		(math-read-expr (cl-etypecase new-unit
-				  (string new-unit)
-				  (symbol (symbol-name new-unit))))))) " ")
-	  (cons (string-to-number value) (intern unit))))))
+  (metar-convert-unit (concat (if (= (aref string 0) ?M)
+				  (concat "-" (substring string 1))
+				string)
+			      "degC")
+		      (or unit (cdr (assq 'temperature metar-units)))
+		      (lambda (expr new-unit-var pure)
+			(math-convert-temperature expr
+						  (math-build-var-name 'degC)
+						  new-unit-var
+						  pure))))
 
 (defcustom metar-url
   "http://weather.noaa.gov/pub/data/observations/metar/stations/%s.TXT"
-- 
2.2.1

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Ulf Jasper]

Wolfgang Jenkner <wjenkner@inode.at> writes:

> Please note that this patch assumes that the patch from (Bug#19582) has
> been applied.

Looks OK to me (but I'm not an expert).  Please go ahead!

Ulf

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Wolfgang Jenkner]

On Tue, Jan 13 2015, Ulf Jasper wrote:

> Looks OK to me (but I'm not an expert).  Please go ahead!

As far as metar is concerned, there remains the problem that the
proposed patch fixes the case of (metar-convert-temperature "M01") only
in future versions of emacs (the other edge cases "00" and "01" were
broken as well, I think, but they should now work regardless of the
pending calc patch).  So, in order to thoroughly fix this on other
versions of emacs one could simply handle the conversion of "M01" as
special case.

However, it's up to the author to decide if he likes this approach at
all.

Wolfgang

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Wolfgang Jenkner]

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

On Tue, Jan 13 2015, Wolfgang Jenkner wrote:

> However, it's up to the author to decide if he likes this approach at
> all.

Mario,

Ping :-)  Do you mind if I push yesteryear's patch to elpa?

If you want to review it, I'll wait for your approval, of course.
Otherwise, due to popular demand, I'd like to push it in a week or so.
For easier reference, I've also attached it below.

I also proposed a second patch as a work-around for a calc glitch,
however that bug has been fixed in the last emacs release, so I think it
can be omitted.

Wolfgang


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: metar patch --]
[-- Type: text/x-diff, Size: 3816 bytes --]

From bc778d9ac011e1f27cfc85055a8575f39f912d9b Mon Sep 17 00:00:00 2001
From: Wolfgang Jenkner <wjenkner@inode.at>
Date: Sat, 3 Jan 2015 00:34:33 +0100
Subject: [PATCH 1/2] [metar] Fix the case where the metar record contains M01
 (bug#19401).

* packages/metar/metar.el (metar-convert-unit): New optional argument.
(metar-convert-temperature): Use it to rewrite this function in terms of
metar-convert-unit.
Pass t as last argument to the underlying calc conversion functions so
that they return only the number, not the unit, thereby fixing the bug
in question.
---
 packages/metar/metar.el | 66 ++++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/packages/metar/metar.el b/packages/metar/metar.el
index 7b78f07..9575313 100644
--- a/packages/metar/metar.el
+++ b/packages/metar/metar.el
@@ -239,45 +239,43 @@ If no match if found, nil is returned."
       (when station-code
 	(cons station-code (round best-distance))))))
 
-(defun metar-convert-unit (value new-unit)
+(defun metar-convert-unit (value new-unit &optional convert-units-function)
   "Convert VALUE to NEW-UNIT.
 VALUE is a string with the value followed by the unit, like \"5 knot\"
-and NEW-UNIT should be a unit name like \"kph\" or similar."
+and NEW-UNIT should be a unit name like \"kph\" or similar.
+CONVERT-UNITS-FUNCTION designates the function actually doing the conversion.
+It must have the signature of `math-convert-units', which is the default."
   (cl-check-type value string)
-  (cl-check-type new-unit (or string symbol))
-  (cl-multiple-value-bind (value unit)
-      (split-string
-       (math-format-value
-	(math-convert-units (math-simplify (math-read-expr value))
-			    (math-read-expr
-			     (cl-etypecase new-unit
-					   (string new-unit)
-					   (symbol (symbol-name new-unit))))))
-       " ")
-    (cons (string-to-number value) (intern unit))))
+  (unless (symbolp new-unit)
+    (setq new-unit (intern new-unit)))
+  (let ((expr (math-simplify (math-read-expr value))))
+    (cl-assert (or (math-zerop expr)
+		   (not (memq (math-single-units-in-expr-p expr) '(nil wrong))))
+	       nil
+	       "Metar: Not exactly one unit in expression: %S" expr)
+    (let ((res (math-simplify-units
+		(funcall (or convert-units-function 'math-convert-units)
+			 expr
+			 (math-build-var-name new-unit)
+			 t))))
+      (cl-assert (math-realp res) nil
+		 "Metar: Not a Calc real number: %S" res)
+      (cons (string-to-number (math-format-value (if (integerp res)
+						     res
+						   (math-float res))))
+	    new-unit))))
 
 (defun metar-convert-temperature (string &optional unit)
-  (let* ((value (concat (if (= (aref string 0) ?M)
-			    (concat "-" (substring string 1))
-			  string)
-			"degC"))
-	 (expr (math-read-expr value))
-	 (old-unit (math-single-units-in-expr-p expr))
-	 (new-unit (or unit (cdr (assq 'temperature metar-units)))))
-    (if old-unit
-	(cl-multiple-value-bind (value unit)
-	    (split-string
-	     (math-format-value
-	      (math-simplify-units
-	       (math-convert-temperature
-		expr
-		(list 'var
-		      (car old-unit)
-		      (intern (concat "var-" (symbol-name (car old-unit)))))
-		(math-read-expr (cl-etypecase new-unit
-				  (string new-unit)
-				  (symbol (symbol-name new-unit))))))) " ")
-	  (cons (string-to-number value) (intern unit))))))
+  (metar-convert-unit (concat (if (= (aref string 0) ?M)
+				  (concat "-" (substring string 1))
+				string)
+			      "degC")
+		      (or unit (cdr (assq 'temperature metar-units)))
+		      (lambda (expr new-unit-var pure)
+			(math-convert-temperature expr
+						  (math-build-var-name 'degC)
+						  new-unit-var
+						  pure))))
 
 (defcustom metar-url
   "http://weather.noaa.gov/pub/data/observations/metar/stations/%s.TXT"
-- 
2.7.0

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Wolfgang Jenkner]

On Fri, Feb 19 2016, Wolfgang Jenkner wrote:

> Mario,
>
> Ping :-)  Do you mind if I push yesteryear's patch to elpa?

So, I've done this (I think, as the push seemed to hang) and, contrary
to what I said above, I've pushed the work-around for a calc glitch as
well since people might still be running Emacs 24.4 or older, where that
calc bug is present.

I'll leave a window open for further changes (by other people) over the
week-end and, unless someone wants to maintain the package, I'll update
the copyright years, bump the version to 0.2 and close this bug report
on Monday.

Wolfgang

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Wolfgang Jenkner]

On Fri, Feb 26 2016, Wolfgang Jenkner wrote:

> I'll leave a window open for further changes (by other people) over the
> week-end and, unless someone wants to maintain the package, I'll update
> the copyright years, bump the version to 0.2 and close this bug report
> on Monday.

Done.

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Wolfgang Jenkner]

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

On Tue, Jan 13 2015, Wolfgang Jenkner wrote:

> Please note that this patch assumes that the patch from (Bug#19582) has
> been applied.

Actually, the following patch (on top of the last one) might be enough
for metar to avoid the need for that fix from bug#19582, and so to
remain compatible with older (than the future trunk ;-) emacs versions.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: calc bug work-around for metar --]
[-- Type: text/x-diff, Size: 1161 bytes --]

From ad084a9b35e79f3524e4ef01137cf256c0509266 Mon Sep 17 00:00:00 2001
From: Wolfgang Jenkner <wjenkner@inode.at>
Date: Wed, 14 Jan 2015 02:31:01 +0100
Subject: [PATCH] [metar] Add a work-around for bug#19582.

* packages/metar/metar.el (metar-convert-unit): Rewrite expr if it
starts with the `neg' operator.  Technically, this is wrong because
various functions in calc-units expect already simplified
expressions.  In practice, it should work around the above bug.
---
 packages/metar/metar.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/packages/metar/metar.el b/packages/metar/metar.el
index 9575313..a05a353 100644
--- a/packages/metar/metar.el
+++ b/packages/metar/metar.el
@@ -249,6 +249,9 @@ It must have the signature of `math-convert-units', which is the default."
   (unless (symbolp new-unit)
     (setq new-unit (intern new-unit)))
   (let ((expr (math-simplify (math-read-expr value))))
+    ;; Sneakily work around bug#19582.
+    (when (eq (car-safe expr) 'neg)
+      (setq expr `(* -1 ,(cadr expr))))
     (cl-assert (or (math-zerop expr)
 		   (not (memq (math-single-units-in-expr-p expr) '(nil wrong))))
 	       nil
-- 
2.2.1

bug#19401: 25.0.50; metar.el (metar-convert-temperature) wrong type argument stringp nil

[Mark Oteiza]

Wolfgang Jenkner <wjenkner@inode.at> writes:

> On Tue, Jan 13 2015, Wolfgang Jenkner wrote:
>
>> Please note that this patch assumes that the patch from (Bug#19582) has
>> been applied.
>
> Actually, the following patch (on top of the last one) might be enough
> for metar to avoid the need for that fix from bug#19582, and so to
> remain compatible with older (than the future trunk ;-) emacs versions.
>
>>From ad084a9b35e79f3524e4ef01137cf256c0509266 Mon Sep 17 00:00:00 2001
> From: Wolfgang Jenkner <wjenkner@inode.at>
> Date: Wed, 14 Jan 2015 02:31:01 +0100
> Subject: [PATCH] [metar] Add a work-around for bug#19582.
>
> * packages/metar/metar.el (metar-convert-unit): Rewrite expr if it
> starts with the `neg' operator.  Technically, this is wrong because
> various functions in calc-units expect already simplified
> expressions.  In practice, it should work around the above bug.
> ---
>  packages/metar/metar.el | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/packages/metar/metar.el b/packages/metar/metar.el
> index 9575313..a05a353 100644
> --- a/packages/metar/metar.el
> +++ b/packages/metar/metar.el
> @@ -249,6 +249,9 @@ It must have the signature of `math-convert-units', which is the default."
>    (unless (symbolp new-unit)
>      (setq new-unit (intern new-unit)))
>    (let ((expr (math-simplify (math-read-expr value))))
> +    ;; Sneakily work around bug#19582.
> +    (when (eq (car-safe expr) 'neg)
> +      (setq expr `(* -1 ,(cadr expr))))
>      (cl-assert (or (math-zerop expr)
>  		   (not (memq (math-single-units-in-expr-p expr) '(nil wrong))))
>  	       nil

It looks to me that the patch in bug#19401 that Wolfgang's patches
depend on has been applied some time ago. I find the issue in emacs 24
and 25 to be fixed with both metar.el patches applied.