From 2ad0d1425a4053c56ffc87945a890b5bc89409fc Mon Sep 17 00:00:00 2001
From: Mike Gerwitz <mike.gerwitz@ryansg.com>
Date: Thu, 15 Jul 2021 14:55:32 -0400
Subject: [PATCH] compiler: Correct handling of TRUE matches

There was a bug whereby TRUE matches would keep whatever value was being
matched on, even if it was not a boolean.  That was an oversight from the
proof-of-concept code, and this fixes it; that's why this is behind a flag!

This also adjusts the class aliasing optimization so that it doesn't check
for a `TRUE` symbol name, which was a bad idea to begin with.

This change also ends up expanding `lv:match[@value="TRUE"]` into the long
form, where it didn't previously; this will result in slightly larger xmlo
files in some cases, but it's nothing significant, and it does not impact
compilation times.
---
 RELEASES.md                            |  8 +++++
 src/current/compiler/js.xsl            | 44 +++++++++++++++++++-------
 src/current/include/preproc/expand.xsl |  6 ++--
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/RELEASES.md b/RELEASES.md
index 83c7905c..55b4f897 100644
--- a/RELEASES.md
+++ b/RELEASES.md
@@ -20,6 +20,14 @@ Compiler
 --------
 - Make Summary Page less chatty.
 - Fix incorrect package name for generated worksheet packages.
+- Restrict `TRUE`-match optimization to classification matches (class
+  composition).
+  - This was mistakenly not considering the domain of the match, and
+    therefore was applying the optimization in situations where it should
+    not.  Results of previous classifications are currently the only place
+    we guarantee a boolean value.
+- Apply classification alias optimization to any `1`-valued constant match.
+  - Previously applied only to `TRUE`.
 
 Summary Page
 ------------
diff --git a/src/current/compiler/js.xsl b/src/current/compiler/js.xsl
index 8cc6f8c6..8c97de86 100644
--- a/src/current/compiler/js.xsl
+++ b/src/current/compiler/js.xsl
@@ -541,7 +541,6 @@
 -->
 <template mode="compile" priority="7"
           match="lv:classify[ count( lv:match ) = 1
-                                and lv:match/@value='TRUE'
                                 and not( lv:match/@preproc:inline ) ]">
   <param name="symtable-map" as="map(*)" tunnel="yes" />
 
@@ -550,9 +549,18 @@
   <variable name="src-sym" as="element( preproc:sym )"
             select="$symtable-map( $src )" />
 
+  <variable name="c" as="element()?"
+            select="lv:match/c:*" />
+
+  <variable name="cmpval" as="xs:float?"
+            select="if ( exists( $c/c:value-of ) ) then
+                        $symtable-map( $c/c:value-of/@name )/@value
+                      else
+                        $c/c:const/@value" />
+
   <choose>
     <!-- we only handle aliasing of other classifications -->
-    <when test="$src-sym/@type = 'cgen'">
+    <when test="$src-sym/@type = 'cgen' and $cmpval = 1">
       <sequence select="$compiler:nl" />
 
       <!-- simply alias the @yields -->
@@ -850,8 +858,10 @@
   <param name="symtable-map" as="map(*)" />
   <param name="match" as="element( lv:match )" />
 
-  <variable name="dim" as="xs:integer"
-            select="$symtable-map( $match/@on )/@dim" />
+  <variable name="sym" as="element( preproc:sym )"
+            select="$symtable-map( $match/@on )" />
+  <variable name="dim" as="xs:integer" select="$sym/@dim" />
+  <variable name="type" as="xs:string" select="$sym/@type" />
 
   <variable name="inner" as="xs:string"
             select="compiler:match-name-on( $symtable-map, $match )" />
@@ -862,11 +872,6 @@
             select="if ( $dim = 2 ) then 'NN' else 'N'" />
 
   <choose>
-    <!-- only basic TRUE equality can be used verbatim -->
-    <when test="$match/@value = 'TRUE'">
-      <sequence select="$inner" />
-    </when>
-
     <when test="$match/@anyOf">
       <variable name="anyof" as="xs:string"
                 select="compiler:compile-anyof( $symtable-map, $match )" />
@@ -924,11 +929,28 @@
                                  $match/parent::lv/classify/@as, '''' )" />
       </if>
 
+      <!-- Note: we currently only check for cgen, that's that's the only
+           type we can guarantee to be boolean; params can have any value
+           passed in and we are not necessarily validating the
+           domain.  Until that is in place, it's too dangerous.  We also
+           need more information in the symbol table. -->
+      <variable name="boolmatch" as="xs:boolean"
+                select="$type = 'cgen'" />
+
       <choose>
+        <!-- Boolean-TRUE matches need no translation; their values can be
+             used without modification.  This allows primarily for
+             lower-cost class composition   However, it's important that we do
+             this only when working with a boolean domain, otherwise we may
+             yield a value that is not in the domain {0,1}. -->
+        <when test="$boolmatch and $match/c:eq and $cval = 1">
+          <sequence select="$inner" />
+        </when>
+
         <when test="$dim > 0">
           <choose>
             <!-- negation, very common, so save some bytes -->
-            <when test="$match/c:eq and $cval = 0">
+            <when test="$boolmatch and $match/c:eq and $cval = 0">
               <sequence select="concat( $nf, '(', $inner, ')' )" />
             </when>
 
@@ -942,7 +964,7 @@
         <otherwise>
           <choose>
             <!-- negation, very common, so save some bytes -->
-            <when test="$match/c:eq and $cval = 0">
+            <when test="$boolmatch and $match/c:eq and $cval = 0">
               <sequence select="concat( 'n(', $inner, ')' )" />
             </when>
 
diff --git a/src/current/include/preproc/expand.xsl b/src/current/include/preproc/expand.xsl
index c4b3efb3..4c5302fd 100644
--- a/src/current/include/preproc/expand.xsl
+++ b/src/current/include/preproc/expand.xsl
@@ -596,9 +596,9 @@
 </template>
 
 
-<!-- expand lv:match/@value != 'TRUE' into a c:* expression to simpliy
-     optimizations  -->
-<template match="lv:match[ @value and @value != 'TRUE' ]"
+<!-- expand lv:match/@value into a c:* expression to simplify static
+     analysis -->
+<template match="lv:match[ @value ]"
   mode="preproc:expand" priority="7">
 
   <copy>