Merge pull request #4126 from logseq/dev/var-sizes-linter

Add Linter for large vars

.github/workflows/build.yml

@@ -103,6 +103,9 @@ jobs:
       - name: Carve lint for unused vars
         run: scripts/carve.clj
 
+      - name: Lint for vars that are too large
+        run: scripts/large_vars.clj
+
       - name: Lint invalid translation entries
         run: bb lang:invalid-translations
 

docs/dev-practices.md

@@ -4,6 +4,9 @@ This page describes development practices for this codebase.
 
 ## Linting
 
+Most of our linters require babashka. Before running them, please install
+https://github.com/babashka/babashka#installation.
+
 ### Clojure code
 
 To lint:
@@ -21,7 +24,6 @@ There are outstanding linting items that are currently ignored to allow linting
 ### Unused vars
 
 We use https://github.com/borkdude/carve to detect unused vars in our codebase.
-Before running it, please install https://github.com/babashka/babashka.
 
 To run this linter:
 ```
@@ -39,6 +41,16 @@ scripts/carve.clj '{:interactive true}'
 When a var is ignored, it is added to `.carve/ignore`. Please add a comment for
 why a var is ignored to help others understand why it's unused.
 
+### Large vars
+
+Large vars have a lot of complexity and make it hard for the team to maintain
+and understand them. To run this linter:
+```
+scripts/large_vars.clj
+```
+
+To configure the linter, see its `config` var.
+
 ## Testing
 
 We have unit and end to end tests as described in https://github.com/logseq/logseq#5-run-tests.

scripts/large_vars.clj

@@ -0,0 +1,50 @@
+#!/usr/bin/env bb
+
+(ns large-vars
+  "This script detects vars that are too large and that make it difficult for
+  the team to maintain and understand them."
+  (:require [babashka.pods :as pods]
+            [clojure.pprint :as pprint]
+            [clojure.set :as set]))
+
+(pods/load-pod 'clj-kondo/clj-kondo "2021.12.19")
+(require '[pod.borkdude.clj-kondo :as clj-kondo])
+
+(def config
+  ;; TODO: Discuss with team and agree on lower number
+  {:max-lines-count 100
+   ;; Vars with these metadata flags are allowed. Name should indicate the reason
+   ;; it is allowed
+   :metadata-exceptions #{::data-var
+                          ;; TODO: Address vars tagged with cleanup-todo. These
+                          ;; are left mostly because they are not high priority
+                          ;; or not well understood
+                          ::cleanup-todo}})
+
+(defn -main
+  [args]
+  (let [paths (or args ["src"])
+        {{:keys [var-definitions]} :analysis}
+        (clj-kondo/run!
+         {:lint paths
+          :config {:output {:analysis {:var-definitions {:meta true}}}}})
+        vars (->> var-definitions
+                  (keep (fn [m]
+                          (let [lines-count (inc (- (:end-row m) (:row m)))]
+                            (when (and (> lines-count (:max-lines-count config))
+                                       (empty? (set/intersection (set (keys (:meta m)))
+                                                                 (:metadata-exceptions config))))
+                              {:var (:name m)
+                               :lines-count lines-count
+                               :filename (:filename m)}))))
+                  (sort-by :lines-count (fn [x y] (compare y x))))]
+    (if (seq vars)
+      (do
+        (println (format "The following vars exceed the line count max of %s:"
+                         (:max-lines-count config)))
+        (pprint/print-table vars)
+        (System/exit 1))
+      (println "All vars are below the max size!"))))
+
+(when (= *file* (System/getProperty "babashka.file"))
+  (-main *command-line-args*))

scripts/src/logseq/tasks/lang.clj

@@ -6,12 +6,13 @@
 
 (defn- get-dicts
   []
-  (dissoc (rewrite-clj/var-sexp "src/main/frontend/dicts.cljs" "dicts")
+  (dissoc (rewrite-clj/metadata-var-sexp "src/main/frontend/dicts.cljs" "dicts")
           :tongue/fallback))
 
 (defn- get-non-en-shortcuts
   []
-  (nth (rewrite-clj/var-sexp "src/main/frontend/modules/shortcut/dict.cljs" "dict")
+  (nth (rewrite-clj/metadata-var-sexp "src/main/frontend/modules/shortcut/dict.cljs"
+                                      "dict")
        3))
 
 ;; unnecessary complexity :(
@@ -22,7 +23,7 @@
 
 (defn- get-en-shortcut-dicts
   []
-  (->> (rewrite-clj/var-sexp
+  (->> (rewrite-clj/metadata-var-sexp
         "src/main/frontend/modules/shortcut/config.cljs"
         "all-default-keyboard-shortcuts")
        (map (fn [[k v]] (vector (decorate-namespace k) (:desc v))))
@@ -30,7 +31,7 @@
 
 (defn- get-en-categories
   []
-  (->> (rewrite-clj/var-sexp
+  (->> (rewrite-clj/metadata-var-sexp
         "src/main/frontend/modules/shortcut/config.cljs"
         "category")
        (map (fn [[k v]] (vector k (:doc (meta v)))))
@@ -55,14 +56,14 @@
         en-count (count (dicts :en))
         langs (get-languages)]
     (->> dicts
-           (map (fn [[locale dicts]]
-                  [locale
-                   (Math/round (* 100.0 (/ (count dicts) en-count)))
-                   (count dicts)
-                   (langs locale)]))
-           (sort-by #(nth % 2) >)
-           (map #(zipmap [:locale :percent-translated :translation-count :language] %))
-           task-util/print-table)))
+         (map (fn [[locale dicts]]
+                [locale
+                 (Math/round (* 100.0 (/ (count dicts) en-count)))
+                 (count dicts)
+                 (langs locale)]))
+         (sort-by #(nth % 2) >)
+         (map #(zipmap [:locale :percent-translated :translation-count :language] %))
+         task-util/print-table)))
 
 (defn- shorten [s length]
   (if (< (count s) length)

scripts/src/logseq/tasks/rewrite_clj.clj

@@ -2,14 +2,26 @@
   "Rewrite-clj fns"
   (:require [rewrite-clj.zip :as z]))
 
-(defn- find-symbol-first-right-sexpr
-  [zloc sym]
-  ;; Returns first symbol found
-  (-> (z/find-value zloc z/next sym)
-      z/right
-      z/sexpr))
+(defn- find-symbol-value-sexpr
+  ([zloc sym] (find-symbol-value-sexpr zloc sym z/right))
+  ([zloc sym nav-fn]
+   ;; Returns first symbol found
+   (-> (z/find-value zloc z/next sym)
+       nav-fn
+       z/sexpr)))
 
 (defn var-sexp
+  "Returns value sexp to the right of var"
   [file string-var]
-  (let [zloc (z/of-string (slurp file))]
-    (find-symbol-first-right-sexpr zloc (symbol string-var))))
+  (let [zloc (z/of-string (slurp file))
+        sexp (find-symbol-value-sexpr zloc (symbol string-var))]
+    (or sexp
+        (throw (ex-info "var-sexp must not return nil" {:file file :string-var string-var})))))
+
+(defn metadata-var-sexp
+  "Returns value sexp to the right of var with metadata"
+  [file string-var]
+  (let [zloc (z/of-string (slurp file))
+        sexp (find-symbol-value-sexpr zloc (symbol string-var) (comp z/right z/up))]
+    (or sexp
+        (throw (ex-info "sexp must not return nil" {:file file :string-var string-var})))))

src/main/frontend/components/block.cljs

@@ -814,7 +814,7 @@
     (page-cp config {:block/name namespace})]
    (namespace-hierarchy-aux config namespace children)])
 
-(defn inline
+(defn ^:large-vars/cleanup-todo inline
   [{:keys [html-export?] :as config} item]
   (match item
     ["Plain" s]
@@ -2168,7 +2168,7 @@
      children)
     (distinct @refs)))
 
-(rum/defcs block-container < rum/reactive
+(rum/defcs ^:large-vars/cleanup-todo block-container < rum/reactive
   {:init (fn [state]
            (let [[config block] (:rum/args state)]
              (when (and (not (some? (state/sub-collapsed (:block/uuid block))))
@@ -2465,7 +2465,7 @@
                      result-atom)]
     (assoc state :query-atom query-atom)))
 
-(rum/defcs custom-query < rum/reactive
+(rum/defcs ^:large-vars/cleanup-todo custom-query < rum/reactive
   {:will-mount trigger-custom-query!
    :did-mount (fn [state]
                 (when-let [query (last (:rum/args state))]
@@ -2649,7 +2649,7 @@
                (when (and (= language "clojure") (contains? (set options) ":results"))
                  (sci/eval-result code)))]))))))
 
-(defn markup-element-cp
+(defn ^:large-vars/cleanup-todo markup-element-cp
   [{:keys [html-export?] :as config} item]
   (let [format (or (:block/format config)
                    :markdown)]

src/main/frontend/components/content.cljs

@@ -146,7 +146,7 @@
                     (reset! edit? true))}
        "Make template"))))
 
-(rum/defc block-context-menu-content
+(rum/defc ^:large-vars/cleanup-todo block-context-menu-content
   [_target block-id]
 
   (let [*el-ref (rum/use-ref nil)]

src/main/frontend/components/editor.cljs

@@ -225,7 +225,7 @@
                            template)
             :class       "black"}))))))
 
-(rum/defc mobile-bar < rum/reactive
+(rum/defc ^:large-vars/cleanup-todo mobile-bar < rum/reactive
   [_parent-state parent-id]
   (let [vw-state (state/sub :ui/visual-viewport-state)
         vw-pending? (state/sub :ui/visual-viewport-pending?)

src/main/frontend/components/onboarding.cljs

@@ -5,7 +5,7 @@
             [rum.core :as rum]
             [frontend.ui :as ui]))
 
-(rum/defc intro
+(rum/defc ^:large-vars/cleanup-todo intro
   []
   [:div#logseq-intro.pl-1
    [:div.flex-1

src/main/frontend/components/page.cljs

@@ -358,7 +358,7 @@
 (defonce *orphan-pages? (atom true))
 (defonce *builtin-pages? (atom nil))
 
-(rum/defc graph-filters < rum/reactive
+(rum/defc ^:large-vars/cleanup-todo graph-filters < rum/reactive
   [graph settings n-hops]
   (let [{:keys [journal? orphan-pages? builtin-pages?]
          :or {orphan-pages? true}} settings
@@ -656,7 +656,7 @@
                     (notification/show! (str (t :tips/all-done) "!") :success)
                     (js/setTimeout #(refresh-fn) 200)))]]))
 
-(rum/defcs all-pages < rum/reactive
+(rum/defcs ^:large-vars/cleanup-todo all-pages < rum/reactive
   (rum/local nil ::pages)
   (rum/local nil ::search-key)
   (rum/local nil ::results-all)

src/main/frontend/components/plugins.cljs

@@ -152,7 +152,7 @@
       Meanwhile, make sure you have regular backups of your graphs and only install the plugins when you can read and
       understand the source code."]))
 
-(rum/defc plugin-item-card < rum/static
+(rum/defc ^:large-vars/cleanup-todo plugin-item-card < rum/static
   [{:keys [id name title settings version url description author icon usf iir repo sponsors] :as item}
    market? *search-key has-other-pending?
    installing-or-updating? installed? stat coming-update]
@@ -289,7 +289,7 @@
                        (page-handler/init-commands!))
                      true)]])]]))
 
-(rum/defc panel-control-tabs
+(rum/defc ^:large-vars/cleanup-todo panel-control-tabs
   < rum/static
   [t search-key *search-key category *category
    sort-or-filter-by *sort-or-filter-by selected-unpacked-pkg

src/main/frontend/components/query_table.cljs

@@ -54,6 +54,28 @@
       (map #(medley/dissoc-in % ks) result)
       result)))
 
+(defn- sort-by-fn [sort-by-item item]
+  (case sort-by-item
+    :created-at
+    (:block/created-at item)
+    :updated-at
+    (:block/updated-at item)
+    :block
+    (:block/content item)
+    :page
+    (:block/name item)
+    (get-in item [:block/properties key])))
+
+(defn- desc?
+  [*desc? p-desc?]
+  (cond
+    (some? @*desc?)
+    @*desc?
+    (some? p-desc?)
+    p-desc?
+    :else
+    true))
+
 (rum/defcs result-table < rum/reactive
   (rum/local nil ::sort-by-item)
   (rum/local nil ::desc?)
@@ -66,13 +88,6 @@
           *sort-by-item (get state ::sort-by-item)
           *desc? (get state ::desc?)
           sort-by-item (or @*sort-by-item (some-> p-sort-by keyword) :updated-at)
-          desc? (cond
-                  (some? @*desc?)
-                  @*desc?
-                  (some? p-desc?)
-                  p-desc?
-                  :else
-                  true)
           ;; remove templates
           result (remove (fn [b] (some? (get-in b [:block/properties :template]))) result)
           result (if page? result (attach-clock-property result))
@@ -91,19 +106,9 @@
                           (filter included-keys keys)
                           included-keys)
                   keys))
-          sort-by-fn (fn [item]
-                       (let [key sort-by-item]
-                         (case key
-                           :created-at
-                           (:block/created-at item)
-                           :updated-at
-                           (:block/updated-at item)
-                           :block
-                           (:block/content item)
-                           :page
-                           (:block/name item)
-                           (get-in item [:block/properties key]))))
-          result (sort-result-by sort-by-fn desc? result)]
+          result (sort-result-by (partial sort-by-fn sort-by-item)
+                                 (desc? *desc? p-desc?)
+                                 result)]
       [:div.overflow-x-auto {:on-mouse-down (fn [e] (.stopPropagation e))
                              :style {:width "100%"}}
        [:table.table-auto

src/main/frontend/components/repo.cljs

@@ -183,22 +183,23 @@
     (p/let [multiple-windows? (ipc/ipc "graphHasMultipleWindows" (state/get-current-repo))]
       (reset! (::electron-multiple-windows? state) multiple-windows?))))
 
+(defn- get-repo-name [repo]
+  (cond
+    (mobile-util/is-native-platform?)
+    (text/get-graph-name-from-path repo)
+
+    (config/local-db? repo)
+    (config/get-local-dir repo)
+
+    :else
+    (db/get-repo-path repo)))
+
 (rum/defcs repos-dropdown < rum/reactive
   (rum/local false ::electron-multiple-windows?)
   [state]
   (let [multiple-windows? (::electron-multiple-windows? state)]
     (when-let [current-repo (state/sub :git/current-repo)]
-      (let [get-repo-name (fn [repo]
-                            (cond
-                              (mobile-util/is-native-platform?)
-                              (text/get-graph-name-from-path repo)
-
-                              (config/local-db? repo)
-                              (config/get-local-dir repo)
-
-                              :else
-                              (db/get-repo-path repo)))
-            repos (state/sub [:me :repos])
+      (let [repos (state/sub [:me :repos])
             repos (remove (fn [r] (= config/local-repo (:url r))) repos)
             switch-repos (remove (fn [repo]
                                    (= current-repo (:url repo)))

src/main/frontend/components/search.cljs

@@ -83,7 +83,7 @@
 
 (defonce search-timeout (atom nil))
 
-(rum/defc search-auto-complete
+(rum/defc ^:large-vars/cleanup-todo search-auto-complete
   [{:keys [pages files blocks has-more?] :as result} search-q all?]
   (let [pages (when-not all? (map (fn [page]
                                     (let [alias (model/get-redirect-page-name page)]

src/main/frontend/components/settings.cljs

@@ -514,7 +514,7 @@
     {:left-label "Plug-in system"
      :action (plugin-enabled-switcher t)}))
 
-(rum/defcs settings
+(rum/defcs ^:large-vars/cleanup-todo settings
   < (rum/local :general ::active)
   {:will-mount
    (fn [state]

src/main/frontend/db/query_dsl.cljs

@@ -166,7 +166,7 @@
     [(list '= '?v v)]
     [(list 'contains? '?v v)])])
 
-(defn build-query
+(defn ^:large-vars/cleanup-todo build-query
   ([repo e env]
    (build-query repo e (assoc env :vars (atom {})) 0))
   ([repo e {:keys [sort-by blocks? sample counter current-filter vars] :as env} level]

src/main/frontend/db_schema.cljs

@@ -3,7 +3,7 @@
 (defonce version 1)
 (defonce ast-version 1)
 ;; A page is a special block, a page can corresponds to multiple files with the same ":block/name".
-(def schema
+(def ^:large-vars/data-var schema
   {:schema/version  {}
    :ast/version     {}
    :db/type         {}

src/main/frontend/dicts.cljs

@@ -3,7 +3,7 @@
             [shadow.resource :as rc]
             [tongue.core :as tongue]))
 
-(def dicts
+(def ^:large-vars/data-var dicts
   {:en {:tutorial/text (rc/inline "tutorial-en.md")
         :tutorial/dummy-notes (rc/inline "dummy-notes-en.md")
         :on-boarding/title "Hi, welcome to Logseq!"

src/main/frontend/extensions/html_parser.cljs

@@ -20,7 +20,7 @@
 
                (str (hiccup-without-style hiccup))))
 
-(defn hiccup->doc-inner
+(defn ^:large-vars/cleanup-todo hiccup->doc-inner
   [format hiccup]
   (let [transform-fn (fn [hiccup]
                        (hiccup->doc-inner format hiccup))

src/main/frontend/extensions/pdf/highlights.cljs

@@ -196,7 +196,7 @@
            :data-color color}])
        rects)]))
 
-(rum/defc pdf-highlight-area-region
+(rum/defc ^:large-vars/cleanup-todo pdf-highlight-area-region
   [^js viewer vw-hl hl
    {:keys [show-ctx-tip! upd-hl!]}]
 
@@ -311,7 +311,7 @@
          (:id hl))
        ))])
 
-(rum/defc pdf-highlight-area-selection
+(rum/defc ^:large-vars/cleanup-todo pdf-highlight-area-selection
   [^js viewer {:keys [show-ctx-tip!]}]
 
   (let [^js viewer-clt (.. viewer -viewer -classList)
@@ -436,7 +436,7 @@
      (when (and start-coord end-coord)
        [:div.shadow-rect {:style (calc-pos start-coord end-coord)}])]))
 
-(rum/defc pdf-highlights
+(rum/defc ^:large-vars/cleanup-todo pdf-highlights
   [^js el ^js viewer initial-hls loaded-pages {:keys [set-dirty-hls!]}]
 
   (let [^js doc (.-ownerDocument el)
@@ -788,7 +788,7 @@
   (fn [close-fn!]
     (docinfo-display info close-fn!)))
 
-(rum/defc pdf-toolbar
+(rum/defc ^:large-vars/cleanup-todo pdf-toolbar
   [^js viewer]
   (let [[area-mode? set-area-mode!] (use-atom *area-mode?)
         [outline-visible?, set-outline-visible!] (rum/use-state false)

src/main/frontend/format/block.cljs

@@ -480,7 +480,7 @@
         blocks (map (fn [block] (dissoc block :block/anchor)) blocks)]
     (with-path-refs blocks)))
 
-(defn extract-blocks
+(defn ^:large-vars/cleanup-todo extract-blocks
   [blocks content with-id? format]
   (try
     (let [encoded-content (utf8/encode content)

src/main/frontend/fs/nfs.cljs

@@ -63,7 +63,7 @@
         (= (string/trim decrypted-content) (string/trim db-content)))
       (p/resolved (= (string/trim disk-content) (string/trim db-content))))))
 
-(defrecord Nfs []
+(defrecord ^:large-vars/cleanup-todo Nfs []
   protocol/Fs
   (mkdir! [_this dir]
     (let [parts (->> (string/split dir "/")

src/main/frontend/handler/editor.cljs

@@ -3003,7 +3003,7 @@
         :else
         nil))))
 
-(defn keyup-handler
+(defn ^:large-vars/cleanup-todo keyup-handler
   [_state input input-id search-timeout]
   (fn [e key-code]
     (when-not (util/event-is-composing? e)

src/main/frontend/modules/shortcut/config.cljs

@@ -19,7 +19,7 @@
 
 ;; Note – when you change this file, you will need to do a hard reset.
 ;; The commands are registered when the Clojurescript code runs for the first time
-(defonce all-default-keyboard-shortcuts
+(defonce ^:large-vars/data-var all-default-keyboard-shortcuts
   {:date-picker/complete         {:desc    "Date picker: Choose selected day"
                                   :binding "enter"
                                   :fn      ui-handler/shortcut-complete}
@@ -449,7 +449,7 @@
   (reduce into {}
           (map (fn [sym] {sym (get all-default-keyboard-shortcuts sym)}) symbols)))
 
-(defonce config
+(defonce ^:large-vars/data-var config
   (atom
    {:shortcut.handler/date-picker
     (build-category-map [:date-picker/complete
@@ -584,7 +584,7 @@
      (with-meta {:before m/enable-when-not-editing-mode!}))}))
 
 ;; Categories for docs purpose
-(def category
+(def ^:large-vars/data-var category
   {:shortcut.category/basics
    ^{:doc "Basics"}
    [:editor/new-block

src/main/frontend/modules/shortcut/dict.cljs

@@ -2,7 +2,7 @@
   (:require [frontend.modules.shortcut.data-helper :as dh]
             [frontend.modules.shortcut.macro :refer [shortcut-dict]]))
 
-(def dict
+(def ^:large-vars/data-var dict
   (shortcut-dict
     (dh/desc-helper)
     (dh/category-helper)

src/main/frontend/state.cljs

@@ -17,7 +17,7 @@
             [rum.core :as rum]
             [frontend.mobile.util :as mobile-util]))
 
-(defonce state
+(defonce ^:large-vars/data-var state
   (let [document-mode? (or (storage/get :document/mode?) false)
        current-graph (let [graph (storage/get :git/current-repo)]
                        (when graph (ipc/ipc "setCurrentGraph" graph))

src/test/frontend/db/query_dsl_test.cljs

@@ -188,7 +188,7 @@ last-modified-at:: 1609084800002"}]]
                      (or [(= ?v "child page 2")] [(contains? ?v "child page 2")])))
         :count 3}))
 
-(deftest test-parse
+(deftest ^:large-vars/cleanup-todo test-parse
   []
   (testing "nil or blank strings should be ignored"
     (are [x y] (= (q x) y)