Add MISINDENT lint warning for misleading indentation.

Changes

@@ -16,6 +16,7 @@ Verilator 5.013 devel
 * Deprecation planned for 32-bit pointer -m32 mode (#4268).
 * Support some stream operations on queues (#4292). [Ryszard Rozak, Antmicro Ltd]
 * Add GENUNNAMED lint warning. [Srinivasan Venkataramanan, Deepa Palaniappan]
+* Add MISINDENT lint warning for misleading indentation.
 * Fix 'VlForkSync' redeclaration (#4277). [Krzysztof Bieganski, Antmicro Ltd]
 * Fix processes that can outlive their parents (#4253). [Krzysztof Boronski, Antmicro Ltd]
 * Fix duplicate fork names (#4295). [Ryszard Rozak, Antmicro Ltd]

docs/guide/exe_verilator.rst

@@ -1562,10 +1562,11 @@ Summary:
 
    Enable all lint-related warning messages (note that by default, they are already
    enabled), but do not affect style messages.  This is equivalent to
-   ``-Wwarn-ALWCOMBORDER -Wwarn-BSSPACE -Wwarn-CASEINCOMPLETE
-   -Wwarn-CASEOVERLAP -Wwarn-CASEX -Wwarn-CASTCONST -Wwarn-CASEWITHX -Wwarn-CMPCONST
-   -Wwarn-COLONPLUS -Wwarn-IMPLICIT -Wwarn-ASCRANGE
-   -Wwarn-PINMISSING -Wwarn-REALCVT -Wwarn-UNSIGNED -Wwarn-WIDTH``.
+   ``-Wwarn-ALWCOMBORDER -Wwarn-ASCRANGE -Wwarn-BSSPACE -Wwarn-CASEINCOMPLETE
+   -Wwarn-CASEOVERLAP -Wwarn-CASEWITHX -Wwarn-CASEX -Wwarn-CASTCONST -Wwarn-CMPCONST
+   -Wwarn-COLONPLUS -Wwarn-IMPLICIT -Wwarn-IMPLICITSTATIC -Wwarn-LATCH -Wwarn-MISINDENT
+   -Wwarn-NEWERSTD -Wwarn-PINMISSING -Wwarn-REALCVT -Wwarn-STATICVAR -Wwarn-UNSIGNED
+   -Wwarn-WIDTHTRUNC -Wwarn-WIDTHEXPAND -Wwarn-WIDTHXZEXPAND``.
 
 .. option:: -Wwarn-style
 

docs/guide/warnings.rst

@@ -1039,6 +1039,51 @@ List Of Warnings
    unsupported. Verilator uses only the typical delay value.
 
 
+.. option:: MISINDENT
+
+   Warns that the indentation of a statement is misleading, suggesting the
+   statement is part of a previous :code:`if` or :code:`while` block while
+   it is not.
+
+   Verilator suppresses this check when there is an inconsistent mix of
+   spaces and tabs, as it cannot ensure the width of tabs.  Verilator also
+   ignores blocks with :code:`begin`/:code:`end`, as the :code:`end`
+   visually indicates the earlier statement's end.
+
+   Ignoring this warning will only suppress the lint check; it will
+   simulate correctly.
+
+   For example
+
+   .. code-block:: sv
+      :linenos:
+      :emphasize-lines: 3
+
+         if (something)
+            statement_in_if;
+            statement_not_in_if;  //<--- Warning
+
+   Results in:
+
+   .. code-block::
+
+         %Warning-MISINDENT: example.v:3:9: Misleading indentation
+
+   To fix this repair the indentation to match the correct earlier
+   statement, for example:
+
+   .. code-block:: sv
+      :linenos:
+      :emphasize-lines: 3
+
+         if (something)
+            statement_in_if;
+         statement_not_in_if;  //<--- Repaired
+
+   Other tools with similar warnings: GCC -Wmisleading-indentation,
+   clang-tidy readability-misleading-indentation.
+
+
 .. option:: MODDUP
 
    .. TODO better example

src/V3Error.h

@@ -115,6 +115,7 @@ public:
         LATCH,          // Latch detected outside of always_latch block
         LITENDIAN,      // Little endian, renamed to ASCRANGE
         MINTYPMAXDLY,   // Unsupported: min/typ/max delay expressions
+        MISINDENT,      // Misleading indentation
         MODDUP,         // Duplicate module
         MULTIDRIVEN,    // Driven from multiple blocks
         MULTITOP,       // Multiple top level modules
@@ -198,7 +199,7 @@ public:
             "IFDEPTH", "IGNOREDRETURN",
             "IMPERFECTSCH", "IMPLICIT", "IMPLICITSTATIC", "IMPORTSTAR", "IMPURE",
             "INCABSPATH", "INFINITELOOP", "INITIALDLY", "INSECURE",
-            "LATCH", "LITENDIAN", "MINTYPMAXDLY", "MODDUP",
+            "LATCH", "LITENDIAN", "MINTYPMAXDLY", "MISINDENT", "MODDUP",
             "MULTIDRIVEN", "MULTITOP", "NEWERSTD", "NOLATCH", "NULLPORT", "PINCONNECTEMPTY",
             "PINMISSING", "PINNOCONNECT",  "PINNOTFOUND", "PKGNODECL", "PROCASSWIRE",
             "PROFOUTOFDATE", "PROTECTED", "RANDC", "REALCVT", "REDEFMACRO", "RISEFALLDLY",
@@ -237,9 +238,9 @@ public:
         return (m_e == ALWCOMBORDER || m_e == ASCRANGE || m_e == BSSPACE || m_e == CASEINCOMPLETE
                 || m_e == CASEOVERLAP || m_e == CASEWITHX || m_e == CASEX || m_e == CASTCONST
                 || m_e == CMPCONST || m_e == COLONPLUS || m_e == IMPLICIT || m_e == IMPLICITSTATIC
-                || m_e == LATCH || m_e == NEWERSTD || m_e == PINMISSING || m_e == REALCVT
-                || m_e == STATICVAR || m_e == UNSIGNED || m_e == WIDTH || m_e == WIDTHTRUNC
-                || m_e == WIDTHEXPAND || m_e == WIDTHXZEXPAND);
+                || m_e == LATCH || m_e == MISINDENT || m_e == NEWERSTD || m_e == PINMISSING
+                || m_e == REALCVT || m_e == STATICVAR || m_e == UNSIGNED || m_e == WIDTH
+                || m_e == WIDTHTRUNC || m_e == WIDTHEXPAND || m_e == WIDTHXZEXPAND);
     }
     // Warnings that are style only
     bool styleError() const VL_MT_SAFE {

src/V3FileLine.cpp

@@ -433,6 +433,12 @@ string FileLine::source() const VL_MT_SAFE {
     }  // LCOV_EXCL_STOP
     return m_contentp->getLine(m_contentLineno);
 }
+string FileLine::sourcePrefix(int toColumn) const VL_MT_SAFE {
+    const std::string src = source();
+    if (toColumn > static_cast<int>(src.length())) toColumn = static_cast<int>(src.length());
+    if (toColumn < 1) return "";
+    return src.substr(0, toColumn - 1);
+}
 string FileLine::prettySource() const VL_MT_SAFE {
     string out = source();
     // Drop ignore trailing newline

src/V3FileLine.h

@@ -241,6 +241,7 @@ public:
     // as the parser errors etc generally make more sense pointing at the last parse point
     int lineno() const VL_MT_SAFE { return m_lastLineno; }
     string source() const VL_MT_SAFE;
+    string sourcePrefix(int toColumn) const VL_MT_SAFE;
     string prettySource() const VL_MT_SAFE;  // Source, w/stripped unprintables and newlines
     FileLine* parent() const VL_MT_SAFE { return m_parent; }
     V3LangCode language() const { return singleton().numberToLang(filenameno()); }

src/V3LinkParse.cpp

@@ -127,6 +127,58 @@ private:
                 && !nodep->stmtsp()->nextp());  // Has only one item
     }
 
+    void checkIndent(AstNode* nodep, AstNode* childp) {
+        // Try very hard to avoid false positives
+        AstNode* nextp = nodep->nextp();
+        if (!childp) return;
+        if (!nextp && VN_IS(nodep, While) && VN_IS(nodep->backp(), Begin))
+            nextp = nodep->backp()->nextp();
+        if (!nextp) return;
+        if (VN_IS(childp, Begin)) return;
+        FileLine* const nodeFlp = nodep->fileline();
+        FileLine* const childFlp = childp->fileline();
+        FileLine* const nextFlp = nextp->fileline();
+        // UINFO(0, "checkInd " << nodeFlp->firstColumn() << " " << nodep << endl);
+        // UINFO(0, "  child  " << childFlp->firstColumn() << " " << childp << endl);
+        // UINFO(0, " next    " << nextFlp->firstColumn() << " " << nextp << endl);
+        // Same filename, later line numbers (no macro magic going on)
+        if (nodeFlp->filenameno() != childFlp->filenameno()) return;
+        if (nodeFlp->filenameno() != nextFlp->filenameno()) return;
+        if (nodeFlp->lastLineno() >= childFlp->firstLineno()) return;
+        if (childFlp->lastLineno() >= nextFlp->firstLineno()) return;
+        // This block has indent 'a'
+        // Child block has indent 'b' where indent('b') > indent('a')
+        // Next block has indent 'b'
+        // (note similar code below)
+        if (!(nodeFlp->firstColumn() < childFlp->firstColumn()
+              && nextFlp->firstColumn() >= childFlp->firstColumn()))
+            return;
+        // Might be a tab difference in spaces up to the node prefix, if so
+        // just ignore this warning
+        // Note it's correct we look at nodep's column in all of these
+        const std::string nodePrefix = nodeFlp->sourcePrefix(nodeFlp->firstColumn());
+        const std::string childPrefix = childFlp->sourcePrefix(nodeFlp->firstColumn());
+        const std::string nextPrefix = nextFlp->sourcePrefix(nodeFlp->firstColumn());
+        if (childPrefix != nodePrefix) return;
+        if (nextPrefix != childPrefix) return;
+        // Some file lines start after the indentation, so make another check
+        // using actual file contents
+        const std::string nodeSource = nodeFlp->source();
+        const std::string childSource = childFlp->source();
+        const std::string nextSource = nextFlp->source();
+        if (!(VString::leadingWhitespaceCount(nodeSource)
+                  < VString::leadingWhitespaceCount(childSource)
+              && VString::leadingWhitespaceCount(nextSource)
+                     >= VString::leadingWhitespaceCount(childSource)))
+            return;
+        nextp->v3warn(MISINDENT,
+                      "Misleading indentation\n"
+                          << nextp->warnContextPrimary() << '\n'
+                          << nodep->warnOther()
+                          << "... Expected indentation matching this earlier statement's line:\n"
+                          << nodep->warnContextSecondary());
+    }
+
     // VISITs
     void visit(AstNodeFTask* nodep) override {
         if (!nodep->user1SetOnce()) {  // Process only once.
@@ -517,6 +569,7 @@ private:
         VL_RESTORER(m_insideLoop);
         {
             m_insideLoop = true;
+            checkIndent(nodep, nodep->stmtsp());
             iterateChildren(nodep);
         }
     }
@@ -533,6 +586,7 @@ private:
         VL_RESTORER(m_insideLoop);
         {
             m_insideLoop = true;
+            checkIndent(nodep, nodep->stmtsp());
             iterateChildren(nodep);
         }
     }
@@ -628,6 +682,7 @@ private:
     }
     void visit(AstGenIf* nodep) override {
         cleanFileline(nodep);
+        checkIndent(nodep, nodep->elsesp() ? nodep->elsesp() : nodep->thensp());
         const bool nestedIf
             = (VN_IS(nodep->backp(), Begin) && nestedIfBegin(VN_CAST(nodep->backp(), Begin)));
         if (nestedIf) {
@@ -674,6 +729,7 @@ private:
     }
     void visit(AstIf* nodep) override {
         cleanFileline(nodep);
+        checkIndent(nodep, nodep->elsesp() ? nodep->elsesp() : nodep->thensp());
         iterateChildren(nodep);
     }
     void visit(AstPrintTimeScale* nodep) override {

src/V3String.cpp

@@ -158,6 +158,15 @@ bool VString::isWhitespace(const string& str) {
     return true;
 }
 
+string::size_type VString::leadingWhitespaceCount(const string& str) {
+    string::size_type result = 0;
+    for (const char c : str) {
+        ++result;
+        if (!std::isspace(c)) break;
+    }
+    return result;
+}
+
 double VString::parseDouble(const string& str, bool* successp) {
     char* const strgp = new char[str.size() + 1];
     char* dp = strgp;

src/V3String.h

@@ -113,6 +113,8 @@ public:
     static string removeWhitespace(const string& str);
     // Return true if only whitespace or ""
     static bool isWhitespace(const string& str);
+    // Return number of spaces/tabs leading in string
+    static string::size_type leadingWhitespaceCount(const string& str);
     // Return double by parsing string
     static double parseDouble(const string& str, bool* successp);
     // Replace all occurrences of the word 'from' in 'str' with 'to'. A word is considered

test_regress/t/t_lint_misindent_bad.out

@@ -0,0 +1,27 @@
+%Warning-MISINDENT: t/t_lint_misindent_bad.v:14:9: Misleading indentation
+   14 |         $display("bad1");   
+      |         ^~~~~~~~
+                    t/t_lint_misindent_bad.v:12:7: ... Expected indentation matching this earlier statement's line:
+   12 |       if (0)
+      |       ^~
+                    ... For warning description see https://verilator.org/warn/MISINDENT?v=latest
+                    ... Use "/* verilator lint_off MISINDENT */" and lint_on around source to disable this message.
+%Warning-MISINDENT: t/t_lint_misindent_bad.v:20:9: Misleading indentation
+   20 |         $display("bad2");   
+      |         ^~~~~~~~
+                    t/t_lint_misindent_bad.v:16:7: ... Expected indentation matching this earlier statement's line:
+   16 |       if (0)
+      |       ^~
+%Warning-MISINDENT: t/t_lint_misindent_bad.v:24:9: Misleading indentation
+   24 |         $display("bad3");   
+      |         ^~~~~~~~
+                    t/t_lint_misindent_bad.v:22:7: ... Expected indentation matching this earlier statement's line:
+   22 |       for (;0;)
+      |       ^~~
+%Warning-MISINDENT: t/t_lint_misindent_bad.v:28:9: Misleading indentation
+   28 |         $display("bad4");   
+      |         ^~~~~~~~
+                    t/t_lint_misindent_bad.v:26:7: ... Expected indentation matching this earlier statement's line:
+   26 |       while (0)
+      |       ^~~~~
+%Error: Exiting due to

test_regress/t/t_lint_misindent_bad.pl

@@ -0,0 +1,20 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2003-2009 by Wilson Snyder. This program is free software; you
+# can redistribute it and/or modify it under the terms of either the GNU
+# Lesser General Public License Version 3 or the Perl Artistic License
+# Version 2.0.
+# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0
+
+scenarios(vlt => 1);
+
+lint(
+    fails => 1,
+    verilator_flags2 => ["--lint-only -Wall -Wno-DECLFILENAME"],
+    expect_filename => $Self->{golden_filename},
+    );
+
+ok(1);
+1;

test_regress/t/t_lint_misindent_bad.v

@@ -0,0 +1,48 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2023 by Wilson Snyder.
+// SPDX-License-Identifier: CC0-1.0
+
+// Do not reindent - spaces are critical to this test
+
+module t (/*AUTOARG*/);
+
+   initial begin
+      if (0)
+        $display("ok");
+        $display("bad1");  // <--- Bad
+
+      if (0)
+        $display("ok");
+      else
+        $display("ok");
+        $display("bad2");  // <--- Bad
+
+      for (;0;)
+        $display("ok");
+        $display("bad3");  // <--- Bad
+
+      while (0)
+        $display("ok");
+        $display("bad4");  // <--- Bad
+
+      // Normal styles
+      if (0) $display("ok");
+      $display("ok");
+      for (;0;) $display("ok");
+      $display("ok");
+      while (0) $display("ok");
+      $display("ok");
+
+      // Questionable but pops up in some cases e.g. SweRV
+      // (all statements have similar indent)
+      if (0)
+        begin
+          $display("ok");
+        end
+          $display("ok");
+
+   end
+
+endmodule