Improve advance detection of self-referential field conditions

kohler · Sep 29, 2024 · 4e3ad94 · 4e3ad94
1 parent acdd185
commit 4e3ad94
Show file tree

Hide file tree

Showing 16 changed files with 108 additions and 25 deletions.
diff --git a/src/formula.php b/src/formula.php
@@ -155,34 +155,34 @@ function format() {
     }
 
     /** @return bool */
-    function has_format() {
+    final function has_format() {
         return $this->_format !== Fexpr::FUNKNOWN;
     }
 
     /** @return bool */
-    function math_format() {
+    final function math_format() {
         return $this->_format !== Fexpr::FREVIEWER
             && $this->_format !== Fexpr::FSUBFIELD;
     }
 
     /** @return bool */
-    function nonnullable_format() {
+    final function nonnullable_format() {
         return $this->_format === Fexpr::FNUMERIC
             || $this->_format === Fexpr::FBOOL;
     }
 
     /** @return bool */
-    function nonnull_format() {
+    final function nonnull_format() {
         return $this->nonnullable_format() && $this->_format_detail;
     }
 
     /** @return mixed */
-    function format_detail() {
+    final function format_detail() {
         return $this->_format_detail;
     }
 
     /** @return string */
-    function format_description() {
+    final function format_description() {
         if ($this->_format === Fexpr::FREVIEWFIELD) {
             return $this->_format_detail->name;
         } else if ($this->_format === Fexpr::FSUBFIELD) {
@@ -194,7 +194,7 @@ function format_description() {
 
     /** @param int $format
      * @param mixed $format_detail */
-    function set_format($format, $format_detail = null) {
+    final function set_format($format, $format_detail = null) {
         assert($this->_format === Fexpr::FUNKNOWN);
         $this->_format = $format;
         $this->_format_detail = $format_detail;
@@ -348,6 +348,13 @@ function compiled_relation($cmp, $other_expr = null) {
         return $cmp;
     }
 
+    /** @param array<int,true> &$oids */
+    function paper_options(&$oids) {
+        foreach ($this->args as $a) {
+            $a->paper_options($oids);
+        }
+    }
+
     #[\ReturnTypeWillChange]
     function jsonSerialize() {
         if ($this->op) {
@@ -2871,6 +2878,13 @@ function add_query_options(&$queryOptions) {
         }
     }
 
+    /** @var array<int,true> &$ods */
+    function paper_options(&$oids) {
+        if ($this->check()) {
+            $this->_parse->fexpr->paper_options($oids);
+        }
+    }
+
     /** @return bool */
     function viewable_by(Contact $user) {
         return $this->check($this->user ?? $user)

diff --git a/src/formulas/f_optionpresent.php b/src/formulas/f_optionpresent.php
@@ -1,6 +1,6 @@
 <?php
 // formulas/f_optionpresent.php -- HotCRP helper class for formula expressions
-// Copyright (c) 2009-2022 Eddie Kohler; see LICENSE.
+// Copyright (c) 2009-2024 Eddie Kohler; see LICENSE.
 
 class OptionPresent_Fexpr extends Fexpr {
     /** @var PaperOption */
@@ -10,6 +10,9 @@ function __construct(PaperOption $option) {
         $this->option = $option;
         $this->set_format(Fexpr::FBOOL);
     }
+    function paper_options(&$oids) {
+        $oids[$this->option->id] = true;
+    }
     function viewable_by(Contact $user) {
         return $user->can_view_some_option($this->option);
     }

diff --git a/src/formulas/f_optionvalue.php b/src/formulas/f_optionvalue.php
@@ -1,6 +1,6 @@
 <?php
 // formulas/f_optionvalue.php -- HotCRP helper class for formula expressions
-// Copyright (c) 2009-2022 Eddie Kohler; see LICENSE.
+// Copyright (c) 2009-2024 Eddie Kohler; see LICENSE.
 
 class OptionValue_Fexpr extends Fexpr {
     /** @var PaperOption */
@@ -9,6 +9,9 @@ function __construct(PaperOption $option) {
         parent::__construct("optionvalue");
         $this->option = $option;
     }
+    function paper_options(&$oids) {
+        $oids[$this->option->id] = true;
+    }
     function viewable_by(Contact $user) {
         return $user->can_view_some_option($this->option);
     }

diff --git a/src/formulas/f_realnumberoption.php b/src/formulas/f_realnumberoption.php
@@ -1,6 +1,6 @@
 <?php
 // formulas/f_realnumberoption.php -- HotCRP helper class for formula expressions
-// Copyright (c) 2009-2022 Eddie Kohler; see LICENSE.
+// Copyright (c) 2009-2024 Eddie Kohler; see LICENSE.
 
 class RealNumberOption_Fexpr extends Fexpr {
     /** @var PaperOption */
@@ -9,6 +9,9 @@ function __construct(PaperOption $option) {
         parent::__construct("realnumberoption");
         $this->option = $option;
     }
+    function paper_options(&$oids) {
+        $oids[$this->option->id] = true;
+    }
     function viewable_by(Contact $user) {
         return $user->can_view_some_option($this->option);
     }

diff --git a/src/formulas/f_topic.php b/src/formulas/f_topic.php
@@ -29,6 +29,9 @@ static function parse_modifier(FormulaCall $ff, $arg, $rest, Formula $formula) {
             return false;
         }
     }
+    function paper_options(&$oids) {
+        $oids[PaperOption::TOPICSID] = true;
+    }
     function compile(FormulaCompiler $state) {
         $state->queryOptions["topics"] = true;
         $texpr = $state->_prow() . "->topic_list()";

diff --git a/src/formulas/f_topicscore.php b/src/formulas/f_topicscore.php
@@ -1,6 +1,6 @@
 <?php
 // formulas/f_topicscore.php -- HotCRP helper class for formula expressions
-// Copyright (c) 2009-2022 Eddie Kohler; see LICENSE.
+// Copyright (c) 2009-2024 Eddie Kohler; see LICENSE.
 
 class TopicScore_Fexpr extends Fexpr {
     function __construct() {
@@ -9,6 +9,9 @@ function __construct() {
     function inferred_index() {
         return Fexpr::IDX_PC;
     }
+    function paper_options(&$oids) {
+        $oids[PaperOption::TOPICSID] = true;
+    }
     function viewable_by(Contact $user) {
         return $user->isPC;
     }

diff --git a/src/paperoption.php b/src/paperoption.php
@@ -531,13 +531,18 @@ final function override_exists_condition($x) {
         }
     }
     /** @return SearchTerm */
-    private function exists_term() {
-        if ($this->_exists_state === 0 && $this->_exists_term === null) {
+    final function exists_term() {
+        if ($this->_exists_term !== null) {
+            return $this->_exists_term;
+        }
+        if ($this->_exists_state === 0) {
             $s = new PaperSearch($this->conf->root_user(), $this->exists_if);
             $s->set_expand_automatic(true);
             $this->_exists_term = $s->full_term();
             /** @phan-suppress-next-line PhanAccessReadOnlyProperty */
             $this->_phase = Phase_SearchTerm::term_phase($this->_exists_term);
+        } else {
+            $this->_exists_term = new True_SearchTerm;
         }
         return $this->_exists_term;
     }
@@ -552,7 +557,9 @@ final function has_complex_exists_condition() {
     /** @return bool */
     final function is_final() {
         // compute phase of complex exists condition
-        $this->exists_term();
+        if ($this->_exists_state === 0 && $this->_exists_term === null) {
+            $this->exists_term();
+        }
         return $this->_phase === PaperInfo::PHASE_FINAL;
     }
     /** @param bool $use_script_expression
@@ -757,7 +764,9 @@ function jsonSerialize() {
         if ($this->configurable !== true) {
             $j->configurable = $this->configurable;
         }
-        $this->exists_term(); // to set `_phase`
+        if ($this->_exists_state === 0 && $this->_exists_term === null) {
+            $this->exists_term(); // to set `_phase`
+        }
         if ($this->_phase === PaperInfo::PHASE_FINAL) {
             $j->final = true;
         }

diff --git a/src/search/st_author.php b/src/search/st_author.php
@@ -59,6 +59,7 @@ function is_sqlexpr_precise() {
             && !$this->csm->test(0);
     }
     function test(PaperInfo $row, $xinfo) {
+        // XXX presence condition
         $n = 0;
         $can_view = $this->user->allow_view_authors($row);
         if ($this->csm->has_contacts()) {

diff --git a/src/search/st_authormatch.php b/src/search/st_authormatch.php
@@ -48,6 +48,7 @@ function sqlexpr(SearchQueryInfo $sqi) {
         }
     }
     function test(PaperInfo $row, $xinfo) {
+        // XXX presence condition
         if (!$this->user->allow_view_authors($row)) {
             return false;
         }

diff --git a/src/search/st_formula.php b/src/search/st_formula.php
@@ -52,6 +52,9 @@ static function parse_graph($word, SearchWord $sword, PaperSearch $srch) {
         }
         return null;
     }
+    function paper_options(&$oids) {
+        $this->formula->paper_options($oids);
+    }
     function sqlexpr(SearchQueryInfo $sqi) {
         $this->formula->add_query_options($sqi->query_options);
         return "true";

diff --git a/src/search/st_option.php b/src/search/st_option.php
@@ -28,6 +28,10 @@ function paper_requirements(&$options) {
         }
     }
 
+    function paper_options(&$oids) {
+        $oids[$this->option->id] = true;
+    }
+
     function sqlexpr(SearchQueryInfo $sqi) {
         if ($this->option->id > 0) {
             $sqi->add_options_columns();

diff --git a/src/search/st_pdf.php b/src/search/st_pdf.php
@@ -95,6 +95,7 @@ function is_sqlexpr_precise() {
         return $this->dtype === DTYPE_SUBMISSION && $this->format_problem === null;
     }
     function test(PaperInfo $row, $xinfo) {
+        // XXX presence condition
         $dtype = $this->dtype;
         if ($dtype === null) {
             if ($row->finalPaperStorageId > 1

diff --git a/src/searchterm.php b/src/searchterm.php
@@ -61,20 +61,20 @@ function negate() {
 
     /** @param bool $negate
      * @return SearchTerm */
-    function negate_if($negate) {
+    final function negate_if($negate) {
         return $negate ? $this->negate() : $this;
     }
 
     /** @param string $command
      * @param SearchWord $sword
      * @return $this */
-    function add_view_anno($command, $sword) {
+    final function add_view_anno($command, $sword) {
         $this->float["view"][] = new SearchViewCommand($command, $sword);
         return $this;
     }
 
     /** @return list<SearchViewCommand> */
-    function view_commands() {
+    final function view_commands() {
         $v = $this->float["view"] ?? [];
         if (!empty($v)) {
             $v = SearchViewCommand::analyze($v);
@@ -84,7 +84,7 @@ function view_commands() {
 
     /** @param string $field
      * @return ?SearchViewCommand */
-    function find_view_command($field) {
+    final function find_view_command($field) {
         foreach ($this->view_commands() as $svc) {
             if ($svc->keyword === $field)
                 return $svc;
@@ -140,7 +140,7 @@ protected function assign_context($term, $clone_of = null) {
 
     /** @param ?SearchStringContext $context
      * @return ?array{int,int} */
-    function strspan_in($context) {
+    final function strspan_in($context) {
         if ($this->pos1 === null) {
             return null;
         }
@@ -184,6 +184,10 @@ function simple_search(&$options) {
     function paper_requirements(&$options) {
     }
 
+    /** @param array<int,true> &$oids */
+    function paper_options(&$oids) {
+    }
+
 
     /** @return string */
     function sqlexpr(SearchQueryInfo $sqi) {
@@ -461,6 +465,11 @@ function paper_requirements(&$options) {
             $ch->paper_requirements($options);
         }
     }
+    function paper_options(&$oids) {
+        foreach ($this->child as $ch) {
+            $ch->paper_options($oids);
+        }
+    }
     function is_sqlexpr_precise() {
         foreach ($this->child as $ch) {
             if (!$ch->is_sqlexpr_precise())
@@ -848,6 +857,7 @@ protected function _finish() {
         }
         return $this;
     }
+
     function visit($visitor) {
         // Only visit non-highlight terms
         $x = [];
@@ -857,6 +867,12 @@ function visit($visitor) {
         return $visitor($this, ...$x);
     }
 
+    function paper_options(&$oids) {
+        for ($i = 0; $i !== $this->nthen; ++$i) {
+            $this->child[$i]->paper_options($oids);
+        }
+    }
+
     /** @return int */
     function ngroups() {
         return $this->_group_offsets[$this->nthen];
@@ -1489,6 +1505,7 @@ function is_sqlexpr_precise() {
         return $this->trivial && !$this->authorish;
     }
     function test(PaperInfo $row, $xinfo) {
+        // XXX presence conditions
         $data = $row->{$this->field}();
         if ($this->authorish && !$this->user->allow_view_authors($row)) {
             $data = "";

diff --git a/src/settings/s_subfieldcondition.php b/src/settings/s_subfieldcondition.php
@@ -46,19 +46,37 @@ static private function validate1($sv, $ctr, $type, $field, $status, $prow) {
         }
         $siname = "sf/{$ctr}/" . ($type === "exists_if" ? "condition" : "edit_condition");
 
+        // save recursion state
+        $scr = $sv->conf->setting("__sf_condition_recursion");
+        $scrd = $sv->conf->setting_data("__sf_condition_recursion");
+        $sv->conf->change_setting("__sf_condition_recursion", -1);
+
+        // make search
         $ps = new PaperSearch($sv->conf->root_user(), $q);
         foreach ($ps->message_list() as $mi) {
             $sv->append_item_at($siname, $mi);
         }
 
-        $scr = $sv->conf->setting("__sf_condition_recursion");
-        $scrd = $sv->conf->setting_data("__sf_condition_recursion");
-        $sv->conf->change_setting("__sf_condition_recursion", -1);
+        // check script expression
         if ($ps->main_term()->script_expression($prow, SearchTerm::ABOUT_PAPER) === null) {
             $sv->msg_at($siname, "<0>Invalid search in field condition", $status);
             $sv->inform_at($siname, "<0>Field conditions are limited to simple search keywords.");
         }
+
+        // check recursion (catches more cases than script expression alone)
+        $pos = 0;
+        $oids = [];
+        $ps->main_term()->paper_options($oids);
+        while (count($oids) > $pos) {
+            $check = array_keys($oids);
+            while ($pos !== count($check)) {
+                $sv->conf->option_by_id($check[$pos])->exists_term()->paper_options($oids);
+                ++$pos;
+            }
+        }
+
         if ($sv->conf->setting("__sf_condition_recursion") > 0
+            || isset($oids[$field->id])
             || ($status === 1 && $scr === $field->id && $scrd === $type)) {
             $sv->msg_at($siname, "<0>Self-referential search in field condition", 2);
         }

diff --git a/src/sitype.php b/src/sitype.php
@@ -143,7 +143,7 @@ function json_examples(Si $si, SettingValues $sv) {
 
 class Radio_Sitype extends Sitype {
     function parse_reqv($vstr, Si $si, SettingValues $sv) {
-        $values = $si->values($sv);
+        $values = $si->values($sv) ?? [];
         foreach ($values as $allowedv) {
             if ((string) $allowedv === $vstr)
                 return $allowedv;