gap icon indicating copy to clipboard operation
gap copied to clipboard

Deprecate and phase out use of `DeclareAutoreadableVariables` / `AUTO`

Open fingolfin opened this issue 3 months ago • 2 comments

These "automatic" variables are a rather obscure GAP feature that few people know. I'd argue that "magic" happening just because one inspects a variable is not a good design. I can totally see why one might think it's a great idea initially, but ultimately I don't think it warrants the added complexity for users, developers, and maintainers of the language.

GAP itself does not use this. As far as I can tell, only these packages do, and I propose we try to eliminate this usage from those packages over time.

  • [ ] atlasrep
  • [ ] browse
  • [ ] ctbllib
  • [ ] liealgdb -- https://github.com/gap-packages/liealgdb/issues/11
  • [ ] smallsemi -- https://github.com/gap-packages/smallsemi/issues/58
  • [ ] sonata -- https://github.com/gap-packages/sonata/issues/33

To illustrate how one could replace it, here are a few concrete examples:

DeclareAutoreadableVariables( "atlasrep", "gap/mindeg.g",
    [ "MinimalRepresentationInfoData" ] );

This ensures the first access to the variable MinimalRepresentationInfoData results in the atlasrep file gap/mindeg.g being read. Afterwards MinimalRepresentationInfoData is a plain GAP record.

An easy alternative to this would be to replace MinimalRepresentationInfoData by a function which returns said record. Then this function is free to have logic at the start of the form if not is_data_loaded then load_data(); fi;.

The main problem with this kind of change is if there is code in other packages accessing the auto-variables (so here: MinimalRepresentationInfoData). Luckily this is not the case here.

fingolfin avatar Sep 23 '25 13:09 fingolfin

As an example, here is a patch that avoids use of DeclareAutoreadableVariables in browse. Perhaps @frankluebeck and @ThomasBreuer would be willing to accept that as a patch there and make a new release?

From d1b1cee44f4aefb472ad53ac3e50aba42d98763b Mon Sep 17 00:00:00 2001
From: Max Horn <[email protected]>
Date: Wed, 24 Sep 2025 01:38:11 +0200
Subject: [PATCH] Avoid use of DeclareAutoreadableVariables

---
 app/tmdbatt2.g |  4 ++--
 app/tmdbattr.g | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/app/tmdbatt2.g b/app/tmdbatt2.g
index 7fe0885..52ffaef 100644
--- a/app/tmdbatt2.g
+++ b/app/tmdbatt2.g
@@ -285,8 +285,8 @@ DatabaseAttributeAdd( TomLibData.IdEnumerator, rec(
 ##
 #T  Admit an argument, as in BrowseTransitiveGroupsInfo?
 ##
-BindGlobal( "BrowseTomLibInfo", function()
-    NCurses.BrowseGeneric( 
+BindGlobal( "_BrowseTomLibInfo", function()
+    NCurses.BrowseGeneric(
       BrowseTableFromDatabaseIdEnumerator( TomLibData.IdEnumerator,
           [ "self" ],
           [ "Size", "NrClasses", "FusionsFrom", "FusionsTo", "Filename" ],
diff --git a/app/tmdbattr.g b/app/tmdbattr.g
index 779e6ed..4a5c388 100644
--- a/app/tmdbattr.g
+++ b/app/tmdbattr.g
@@ -76,8 +76,18 @@
 ##  </ManSection>
 ##  <#/GAPDoc>
 ##
-DeclareAutoreadableVariables( "Browse", "app/tmdbatt2.g",
-    [ "BrowseTomLibInfo" ] );
+BindGlobal( "BrowseTomLibInfo", function()
+    if not IsBoundGlobal(" _BrowseTomLibInfo" ) then
+      # Avoid nested calls to `RereadPackage',
+      # which could cause that `REREADING' is set to `false' too early.
+      if REREADING then
+        ReadPackage( "Browse", "app/tmdbatt2.g" );
+      else
+        RereadPackage( "Browse", "app/tmdbatt2.g" );
+      fi;
+    fi;
+    ValueGlobal("_BrowseTomLibInfo" )();
+end );
 
 
 #############################################################################
-- 
2.51.0

fingolfin avatar Sep 23 '25 23:09 fingolfin

O.k., I can remove the use of DeclareAutoreadableVariables in the packages where I am involved.

(On the other hand, it is sad that one has to adjust packages whenever some nice feature gets classified as bad design.)

ThomasBreuer avatar Sep 24 '25 11:09 ThomasBreuer