Type Activity/Stage/Round/Pool

Those 4 are closely coupled so it made
sense to handle them as a single PR.

Unfortunately due to the coupling, the return of
some key getters were not added, instead I added
a comment explaning the situation. Properly typing
will likely require some refactoring to split up
the per-type lists, which is better done in a follow-up.

TEST=Checked a user's available rounds, updated a project's state
to confirm that the rounds/pools were still listed.

pinc/Activity.inc

@@ -29,7 +29,26 @@ $ACCESS_CRITERIA = [
  */
 class Activities
 {
-    private static $_activities = [];
+    /*
+     * `$_activities` contains a mapping from a specific type of Activity (e.g. an
+     * Activity, a Stage, a Round or a Pool) to the instances of those. They are mapped
+     * by their name, called IDs). The logic accounts for the class hierarchy:
+     * Stage derives from Activity so any instance will be on both the Stage and
+     * the Activity lists.
+     *
+     * This poses a fundamental issue with adding types to this logic:
+     * the type of each list depends on its name.
+     *
+     * A simple solution would be to return the top-level class Activity (to match how
+     * `$_activities` is typed) but that confuses (rightly) PHPStan and would require a
+     * lot of new type checks whenever callers manipulate these classes.
+     *
+     * Thus for now, we don't return types for several getters that manipulate
+     * `$_activities`. Over time, we will have to find a way to disentangle
+     * the different lists to add proper types.
+     */
+    /** @var array<string, array<string, Activity>> */
+    private static array $_activities = [];
 
     // This maps the static container class names to the class types
     // they contain. This makes the functions in this base class return the
@@ -41,31 +60,40 @@ class Activities
         "Pools" => "Pool",
     ];
 
-    private static function _get_class_mapping()
+    private static function _get_class_mapping(): string
     {
         return self::$container_to_class_mapping[static::class];
     }
 
-    public static function add(&$activity)
+    public static function add(Activity &$activity): void
     {
         foreach (array_merge([get_class($activity)], class_parents($activity)) as $class) {
             assert(!isset(self::$_activities[$class][$activity->id]), "Duplicate activity ID; IDs must be unique");
             self::$_activities[$class][$activity->id] = $activity;
         }
     }
 
-    public static function get_all()
+    /**
+     * The return of this function is untyped for now (see the comment above `$_activities` for why).
+     */
+    public static function get_all(): array
     {
         return self::$_activities[self::_get_class_mapping()];
     }
 
-    public static function get_ids()
+    /** @return string[] */
+    public static function get_ids(): array
     {
         // get_all() will ensure they are the right object types
         return array_keys(self::get_all());
     }
 
-    protected static function _get($attribute, $value)
+    /**
+     * The return of this function is untyped for now (see the comment above `$_activities` for why).
+     *
+     * @param int|string $value
+     */
+    protected static function _get(string $attribute, $value)
     {
         // fast-track lookup by ID
         if ($attribute == "id") {
@@ -86,7 +114,10 @@ class Activities
         return null;
     }
 
-    public static function get_by_id($id)
+    /**
+     * The return of this function is untyped for now (see the comment above `$_activities` for why).
+     */
+    public static function get_by_id(string $id)
     {
         return self::$_activities[self::_get_class_mapping()][$id] ?? null;
     }
@@ -179,7 +210,7 @@ class Activity
         Activities::add($this);
     }
 
-    public function __toString()
+    public function __toString(): string
     {
         return $this->id;
     }
@@ -207,7 +238,8 @@ class Activity
      * If $n_pages_completed is non-null, use it as the number of pages
      * that the user has completed. Otherwise, consult the database.
      */
-    public function user_access($username, $n_pages_completed = null)
+    // TODO(jchaffraix): Add a class for the UserAccess object for type soundness.
+    public function user_access(?string $username, $n_pages_completed = null): object
     {
         if (is_null($username)) {
             $uao = new StdClass(); // user access object
@@ -351,7 +383,11 @@ class Activity
 
 // --------------------------------------------------------------------------
 
-function get_user_scores($username, $criteria)
+/**
+  * @param array<string, int|string> $criteria
+  * @return array<string, int>
+  */
+function get_user_scores(string $username, array $criteria): array
 {
     if (!$criteria) {
         return [];
@@ -360,13 +396,13 @@ function get_user_scores($username, $criteria)
     $user_obj = $username == User::current_username() ? User::load_current() : new User($username);
 
     $user_scores = [];
-    foreach ($criteria as $criterion_code => $criterion_descr) {
+    foreach ($criteria as $criterion_code => $_) {
         $user_scores[$criterion_code] = get_user_score($user_obj, $criterion_code);
     }
     return $user_scores;
 }
 
-function get_user_score($user_obj, $criterion_code)
+function get_user_score(User $user_obj, string $criterion_code): int
 {
     if ($criterion_code == 'days since reg') {
         $user_score = round((time() - $user_obj->date_created) / 86400, 1);
@@ -384,7 +420,7 @@ function get_user_score($user_obj, $criterion_code)
 
 // --------------------------------------------------------------------------
 
-function show_user_access_object($uao, $will_autogrant = false)
+function show_user_access_object(object $uao, bool $will_autogrant = false): void
 {
     global $code_url;
 
@@ -489,7 +525,7 @@ function show_user_access_object($uao, $will_autogrant = false)
     }
 }
 
-function grant_user_access_if_sat(&$uao)
+function grant_user_access_if_sat(object &$uao): void
 {
     $activity = Activities::get_by_id($uao->activity_id);
     if ($uao->request_status == 'sat-available' &&
@@ -505,7 +541,7 @@ function grant_user_access_if_sat(&$uao)
 
 // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
-function show_user_access_chart($username)
+function show_user_access_chart(?string $username): void
 {
     global $ACCESS_CRITERIA, $code_url;
 
@@ -682,7 +718,7 @@ function show_user_access_chart($username)
     }
 }
 
-function td_w_bgcolor($text, $bool)
+function td_w_bgcolor(string $text, bool $bool): void
 {
     $cell_class = $bool ? 'satisfied' : 'not_satisfied';
     echo "<td class='$cell_class'>$text</td>";

pinc/Pool.inc

@@ -7,7 +7,7 @@ include_once($relPath.'Stage.inc');
  */
 class Pools extends Stages
 {
-    public static function get_by_state($state)
+    public static function get_by_state(string $state): ?Pool
     {
         return self::_get("states", $state);
     }
@@ -77,12 +77,12 @@ class Pool extends Stage
 
 // ---------------------------
 
-function get_Pool_for_id($pool_id)
+function get_Pool_for_id(string $pool_id): ?Pool
 {
     return Pools::get_by_id($pool_id);
 }
 
-function get_Pool_for_state($state)
+function get_Pool_for_state(string $state): ?Pool
 {
     return Pools::get_by_state($state);
 }

pinc/Round.inc

@@ -7,7 +7,8 @@ include_once($relPath.'Stage.inc');
  */
 class Rounds extends Stages
 {
-    public static function get_page_states()
+    /** @return string[] */
+    public static function get_page_states(): array
     {
         $page_states = [];
         foreach (self::get_all() as $round) {
@@ -18,22 +19,22 @@ class Rounds extends Stages
         return $page_states;
     }
 
-    public static function get_by_number($number)
+    public static function get_by_number(int $number): ?Round
     {
         return self::_get("round_number", $number);
     }
 
-    public static function get_by_project_state($state)
+    public static function get_by_project_state(string $state): ?Round
     {
         return self::_get("project_states", $state);
     }
 
-    public static function get_by_page_state($state)
+    public static function get_by_page_state(string $state): ?Round
     {
         return self::_get("page_states", $state);
     }
 
-    public static function get_last()
+    public static function get_last(): Round
     {
         return self::get_by_id(array_key_last(self::get_all()));
     }
@@ -208,25 +209,26 @@ class Round extends Stage
         $this->mentee_round = null;
     }
 
-    public function is_a_mentee_round()
+    public function is_a_mentee_round(): bool
     {
         return !is_null($this->mentor_round);
     }
-    public function is_a_mentor_round()
+
+    public function is_a_mentor_round(): bool
     {
         return !is_null($this->mentee_round);
     }
 
     // -----------
 
-    public function has_a_daily_page_limit()
+    public function has_a_daily_page_limit(): bool
     {
         return !is_null($this->daily_page_limit);
     }
 
     // -----------
 
-    public function get_honorific_for_page_tally($page_tally)
+    public function get_honorific_for_page_tally(int $page_tally): string
     {
         // Note that krsort($this->honorifics) put it in descending order.
         foreach ($this->honorifics as $threshold => $honorific) {
@@ -243,7 +245,7 @@ class Round extends Stage
 
     // -----------
 
-    public function validate_user_can_access($user)
+    public function validate_user_can_access(string $user): void
     {
         global $code_url;
 
@@ -262,7 +264,7 @@ class Round extends Stage
 
 // ---------------------------
 
-function get_Round_for_round_id($round_id)
+function get_Round_for_round_id(string $round_id): ?Round
 {
     return Rounds::get_by_id($round_id);
 }
@@ -275,21 +277,21 @@ function get_Round_for_round_id($round_id)
  * If `$round_number` is a valid proofreading-round number,
  * return the appropriate Round instance. Otherwise, return NULL.
  */
-function get_Round_for_round_number($round_number)
+function get_Round_for_round_number(int $round_number): ?Round
 {
     return Rounds::get_by_number($round_number);
 }
 
 // ---------------------------
 
-function get_Round_for_project_state($project_state)
+function get_Round_for_project_state(string $project_state): ?Round
 {
     return Rounds::get_by_project_state($project_state);
 }
 
 // ---------------------------
 
-function get_Round_for_page_state($page_state)
+function get_Round_for_page_state(string $page_state): ?Round
 {
     return Rounds::get_by_page_state($page_state);
 }

pinc/Stage.inc

@@ -67,12 +67,12 @@ class Stage extends Activity
      * Display a page-header, either an image (if available) or a textual title
      * for this stage.
      */
-    public function page_header($title)
+    public function page_header(string $title): void
     {
         echo "<h1>$title</h1>\n" . get_page_header_image($this->id);
     }
 
-    public function page_top($uao)
+    public function page_top(object $uao): void
     {
         $this->page_header("{$this->id}: {$this->name}");
 
@@ -97,14 +97,14 @@ class Stage extends Activity
 
 // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
-function get_Stage_for_id($id)
+function get_Stage_for_id(string $id): ?Stage
 {
     return Stages::get_by_id($id);
 }
 
 // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
-function user_can_work_in_stage($username, $stage_id)
+function user_can_work_in_stage(string $username, string $stage_id): bool
 {
     $stage = get_Stage_for_id($stage_id);
     $uao = $stage->user_access($username);
@@ -113,7 +113,8 @@ function user_can_work_in_stage($username, $stage_id)
 
 // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
-function get_stages_user_can_work_in($username)
+/** @return Stage[] */
+function get_stages_user_can_work_in(string $username): array
 {
     $accessible_stages = [];
     foreach (Stages::get_all() as $stage) {
@@ -131,8 +132,10 @@ function get_stages_user_can_work_in($username)
  *
  * If $include_accessible_stages is true, this array also includes
  * stages the user has access to.
+ *
+ * @return Stage[]
  */
-function get_stages_for_which_user_has_access_to_prereqs($username, $include_accessible_stages = false)
+function get_stages_for_which_user_has_access_to_prereqs(string $username, bool $include_accessible_stages = false): array
 {
     $satisfied_prereq_stages = [];
 

tools/proofers/my_projects.php

@@ -604,7 +604,7 @@ function get_round_query_result($round_view, $round_sort, $round_column_specs, $
         // and select on those
         $avail_states = [];
         foreach (get_stages_user_can_work_in($username) as $stage) {
-            if (Rounds::get_by_id($stage->id)) {
+            if ($stage instanceof Round) {
                 $avail_states[] = $stage->project_available_state;
             }
         }

tools/proofers/my_suggestions.php

@@ -595,7 +595,7 @@ function get_user_suggestion_criteria(?string $username, $flush_cache = false)
     $states_can_work_in = [];
     foreach (get_stages_user_can_work_in($username) as $stage_id => $stage) {
         // only include rounds
-        if (Rounds::get_by_id($stage_id)) {
+        if ($stage instanceof Round) {
             $states_can_work_in[$stage_id] = $stage->project_available_state;
         }
     }