From b455f9b59109ad6f152e49b79e0186c0909df072 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sun, 14 Sep 2025 08:28:47 -0400 Subject: [PATCH] Add ASSIGNEQEXPR when use `=` inside expressions (#5567). --- Changes | 1 + docs/gen/ex_ASSIGNEQEXPR_faulty.rst | 9 ++++++ docs/gen/ex_ASSIGNEQEXPR_msg.rst | 5 ++++ docs/guide/exe_verilator.rst | 18 ++++++------ docs/guide/warnings.rst | 23 ++++++++++++++++ src/V3Error.h | 31 +++++++++++---------- src/verilog.y | 10 ++++++- test_regress/t/t_assign_expr.v | 1 + test_regress/t/t_lint_assigneqexpr.py | 18 ++++++++++++ test_regress/t/t_lint_assigneqexpr.v | 32 ++++++++++++++++++++++ test_regress/t/t_lint_assigneqexpr_bad.out | 11 ++++++++ test_regress/t/t_lint_assigneqexpr_bad.py | 29 ++++++++++++++++++++ test_regress/t/t_nba_assign_on_rhs.v | 2 ++ 13 files changed, 165 insertions(+), 25 deletions(-) create mode 100644 docs/gen/ex_ASSIGNEQEXPR_faulty.rst create mode 100644 docs/gen/ex_ASSIGNEQEXPR_msg.rst create mode 100755 test_regress/t/t_lint_assigneqexpr.py create mode 100644 test_regress/t/t_lint_assigneqexpr.v create mode 100644 test_regress/t/t_lint_assigneqexpr_bad.out create mode 100755 test_regress/t/t_lint_assigneqexpr_bad.py diff --git a/Changes b/Changes index b443b07fb..789983199 100644 --- a/Changes +++ b/Changes @@ -16,6 +16,7 @@ Verilator 5.041 devel * Add error on non-packed struct randc (#5999). [Seth Pellegrino] * Add configure `--enable-asan` to compile verilator_bin with the address sanitizer (#6404). [Geza Lore] * Add $(LDFLAGS) and $(LIBS) to when building shared libraries (#6425) (#6426). [Ahmed El-Mahmoudy] +* Add ASSIGNEQEXPR when use `=` inside expressions (#5567). [Ethan Sifferman] * Support pure functions in sensitivity lists (#6393). [Krzysztof Bieganski, Antmicro Ltd.] * Improve automatic selection of logic for DFG synthesis (#6370). [Geza Lore] * Improve `covergroup with function sample` handling (#6387). [Jakub Wasilewski] diff --git a/docs/gen/ex_ASSIGNEQEXPR_faulty.rst b/docs/gen/ex_ASSIGNEQEXPR_faulty.rst new file mode 100644 index 000000000..7fd7546e0 --- /dev/null +++ b/docs/gen/ex_ASSIGNEQEXPR_faulty.rst @@ -0,0 +1,9 @@ +.. comment: generated by t_lint_assigneqexpr_bad +.. code-block:: sv + :linenos: + :emphasize-lines: 3 + + assign d_o = // Note = not == below + ( + c_o = 1 // <--- Warning: ASSIGNEQEXPR + ) ? 1 : ( diff --git a/docs/gen/ex_ASSIGNEQEXPR_msg.rst b/docs/gen/ex_ASSIGNEQEXPR_msg.rst new file mode 100644 index 000000000..05e4485a8 --- /dev/null +++ b/docs/gen/ex_ASSIGNEQEXPR_msg.rst @@ -0,0 +1,5 @@ +.. comment: generated by t_lint_assigneqexpr_bad +.. code-block:: + + %Warning-ASSIGNEQEXPR: example.v:1:11 Assignment '=' inside expression + : ... Was a '==' intended, or suggest use a separate statement diff --git a/docs/guide/exe_verilator.rst b/docs/guide/exe_verilator.rst index 39b03b18c..b08b036c6 100644 --- a/docs/guide/exe_verilator.rst +++ b/docs/guide/exe_verilator.rst @@ -1878,7 +1878,7 @@ Summary: .. option:: -Wno-lint Disable all lint-related warning messages, and all style warnings. This - is equivalent to ``-Wno-ALWCOMBORDER`` ``-Wno-ASCRANGE`` + is equivalent to ``-Wno-ALWCOMBORDER`` ``-Wno-ASCRANGE`` ``-Wno-ASSIGNEQEXPR`` ``-Wno-BSSPACE`` ``-Wno-CASEINCOMPLETE`` ``-Wno-CASEOVERLAP`` ``-Wno-CASEX`` ``-Wno-CASTCONST`` ``-Wno-CASEWITHX`` ``-Wno-CMPCONST`` ``-Wno-COLONPLUS`` ``-Wno-IMPLICIT`` ``-Wno-IMPLICITSTATIC`` @@ -1932,14 +1932,14 @@ 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-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-PREPROCZERO`` ``-Wwarn-PINMISSING`` ``-Wwarn-REALCVT`` - ``-Wwarn-STATICVAR`` ``-Wwarn-UNSIGNED`` ``-Wwarn-WIDTHTRUNC`` - ``-Wwarn-WIDTHEXPAND`` ``-Wwarn-WIDTHXZEXPAND``. + to ``-Wwarn-ALWCOMBORDER`` ``-Wwarn-ASCRANGE`` ``-Wno-ASSIGNEQEXPR`` + ``-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-PREPROCZERO`` ``-Wwarn-PINMISSING`` + ``-Wwarn-REALCVT`` ``-Wwarn-STATICVAR`` ``-Wwarn-UNSIGNED`` + ``-Wwarn-WIDTHTRUNC`` ``-Wwarn-WIDTHEXPAND`` ``-Wwarn-WIDTHXZEXPAND``. .. option:: -Wwarn-style diff --git a/docs/guide/warnings.rst b/docs/guide/warnings.rst index 79dd779f2..ce8bf3d7f 100644 --- a/docs/guide/warnings.rst +++ b/docs/guide/warnings.rst @@ -169,6 +169,29 @@ List Of Warnings This warning is issued only if Verilator is run with :vlopt:`--no-timing`. +.. option:: ASSIGNEQEXPR + + Warning that an assignment with `=` appears in a complex expression. + The intent may have been to use `==`, or, if `=` is correct this may be + a readability issue. + + Faulty example: + + .. include:: ../../docs/gen/ex_ASSIGNEQEXPR_faulty.rst + + Results in: + + .. include:: ../../docs/gen/ex_ASSIGNEQEXPR_msg.rst + + To repair, make the assignment into a separate statement. + + Disabled by default as this is a code-style warning; it will simulate + correctly. + + Ignoring this warning will only suppress the lint check; it will + simulate correctly. + + .. option:: ASSIGNIN .. TODO better example diff --git a/src/V3Error.h b/src/V3Error.h index b945a3926..6f9ea1c9b 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -75,6 +75,7 @@ public: ALWNEVER, // always will never execute ASCRANGE, // Ascending bit range vector ASSIGNDLY, // Assignment delays + ASSIGNEQEXPR, // Assignment equal (=) in expression ASSIGNIN, // Assigning to input BADSTDPRAGMA, // Any error related to pragmas BADVLTPRAGMA, // Unknown Verilator pragma @@ -206,15 +207,15 @@ public: "CONSTWRITTEN", "LIFETIME", "NEEDTIMINGOPT", "NOTIMING", "PORTSHORT", "TASKNSVAR", "UNSUPPORTED", // Warnings - " EC_FIRST_WARN", "ALWCOMBORDER", "ALWNEVER", "ASCRANGE", "ASSIGNDLY", "ASSIGNIN", - "BADSTDPRAGMA", "BADVLTPRAGMA", "BLKANDNBLK", "BLKLOOPINIT", "BLKSEQ", "BSSPACE", - "CASEINCOMPLETE", "CASEOVERLAP", "CASEWITHX", "CASEX", "CASTCONST", "CDCRSTLOGIC", - "CLKDATA", "CMPCONST", "COLONPLUS", "COMBDLY", "CONSTRAINTIGN", "CONTASSREG", - "COVERIGN", "DECLFILENAME", "DEFOVERRIDE", "DEFPARAM", "DEPRECATED", "ENCAPSULATED", - "ENDLABEL", "ENUMITEMWIDTH", "ENUMVALUE", "EOFNEWLINE", "GENCLK", "GENUNNAMED", - "HIERBLOCK", "IFDEPTH", "IGNOREDRETURN", "IMPERFECTSCH", "IMPLICIT", "IMPLICITSTATIC", - "IMPORTSTAR", "IMPURE", "INCABSPATH", "INFINITELOOP", "INITIALDLY", "INSECURE", - "LATCH", "LITENDIAN", "MINTYPMAXDLY", "MISINDENT", "MODDUP", "MODMISSING", + " EC_FIRST_WARN", "ALWCOMBORDER", "ALWNEVER", "ASCRANGE", "ASSIGNDLY", "ASSIGNEQEXPR", + "ASSIGNIN", "BADSTDPRAGMA", "BADVLTPRAGMA", "BLKANDNBLK", "BLKLOOPINIT", "BLKSEQ", + "BSSPACE", "CASEINCOMPLETE", "CASEOVERLAP", "CASEWITHX", "CASEX", "CASTCONST", + "CDCRSTLOGIC", "CLKDATA", "CMPCONST", "COLONPLUS", "COMBDLY", "CONSTRAINTIGN", + "CONTASSREG", "COVERIGN", "DECLFILENAME", "DEFOVERRIDE", "DEFPARAM", "DEPRECATED", + "ENCAPSULATED", "ENDLABEL", "ENUMITEMWIDTH", "ENUMVALUE", "EOFNEWLINE", "GENCLK", + "GENUNNAMED", "HIERBLOCK", "IFDEPTH", "IGNOREDRETURN", "IMPERFECTSCH", "IMPLICIT", + "IMPLICITSTATIC", "IMPORTSTAR", "IMPURE", "INCABSPATH", "INFINITELOOP", "INITIALDLY", + "INSECURE", "LATCH", "LITENDIAN", "MINTYPMAXDLY", "MISINDENT", "MODDUP", "MODMISSING", "MULTIDRIVEN", "MULTITOP", "NEWERSTD", "NOEFFECT", "NOLATCH", "NONSTD", "NULLPORT", "PARAMNODEFAULT", "PINCONNECTEMPTY", "PINMISSING", "PINNOCONNECT", "PINNOTFOUND", "PKGNODECL", "PREPROCZERO", "PROCASSINIT", "PROCASSWIRE", "PROFOUTOFDATE", "PROTECTED", @@ -264,12 +265,12 @@ public: } // Warnings that are lint only bool lintError() const VL_MT_SAFE { - 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 == MISINDENT || m_e == NEWERSTD || m_e == PREPROCZERO - || m_e == PINMISSING || m_e == REALCVT || m_e == STATICVAR || m_e == UNSIGNED - || m_e == WIDTH || m_e == WIDTHTRUNC || m_e == WIDTHEXPAND + return (m_e == ALWCOMBORDER || m_e == ASCRANGE || m_e == ASSIGNEQEXPR || 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 == MISINDENT || m_e == NEWERSTD + || m_e == PREPROCZERO || 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 diff --git a/src/verilog.y b/src/verilog.y index 38821427b..f0d41d541 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -157,6 +157,13 @@ static void ERRSVKWD(FileLine* fileline, const string& tokname) { : "")); } +static void ASSIGNEQEXPR(FileLine* fileline) { + fileline->v3warn(ASSIGNEQEXPR, + "Assignment '=' inside expression\n" + << fileline->warnMore() + << "... Was a '==' intended, or suggest use a separate statement"); +} + static void UNSUPREAL(FileLine* fileline) { fileline->v3warn(SHORTREAL, "Unsupported: shortreal being promoted to real (suggest use real instead)"); @@ -4828,7 +4835,8 @@ expr: // IEEE: part of expression/constant_expression/ // // Need exprScope of variable_lvalue to prevent conflict | '(' ~p~exprScope '=' expr ')' { $$ = new AstExprStmt{$1, new AstAssign{$3, $2, $4}, - $2->cloneTreePure(true)}; } + $2->cloneTreePure(true)}; + ASSIGNEQEXPR($3); } | '(' ~p~exprScope yP_PLUSEQ expr ')' { $$ = new AstExprStmt{$1, new AstAssign{$3, $2, new AstAdd{$3, $2->cloneTreePure(true), $4}}, $2->cloneTreePure(true)}; } diff --git a/test_regress/t/t_assign_expr.v b/test_regress/t/t_assign_expr.v index e2cbf5f4f..7eb4b6f28 100644 --- a/test_regress/t/t_assign_expr.v +++ b/test_regress/t/t_assign_expr.v @@ -13,6 +13,7 @@ module t; int b; int i; + // verilator lint_off ASSIGNEQEXPR initial begin a = 10; i = (a = 2); diff --git a/test_regress/t/t_lint_assigneqexpr.py b/test_regress/t/t_lint_assigneqexpr.py new file mode 100755 index 000000000..3844c944c --- /dev/null +++ b/test_regress/t/t_lint_assigneqexpr.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2024 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 + +import vltest_bootstrap + +test.scenarios('simulator') + +test.compile(verilator_flags2=['--binary', '--trace', '-Wno-ASSIGNEQEXPR']) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_lint_assigneqexpr.v b/test_regress/t/t_lint_assigneqexpr.v new file mode 100644 index 000000000..0c56cb583 --- /dev/null +++ b/test_regress/t/t_lint_assigneqexpr.v @@ -0,0 +1,32 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2025 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +module t ( + input logic a2_i, + a1_i, + a0_i, + input logic b_i, + output logic d_o +); + // verilator lint_off PINMISSING + Sub sub (.a_i({a2_i, a1_i, a0_i}), .b_i, .d_o); + // verilator lint_on PINMISSING +endmodule + +module Sub ( + input logic [2:0] a_i, + input logic b_i, + output logic c_o, + output logic d_o +); + assign c_o = (a_i != 0) ? 1 : 0; + assign d_o = // Note = not == below + ( + c_o = 1 // <--- Warning: ASSIGNEQEXPR + ) ? 1 : ( + c_o = 0 // <--- Warning: ASSIGNEQEXPR + ) ? b_i : 0; +endmodule diff --git a/test_regress/t/t_lint_assigneqexpr_bad.out b/test_regress/t/t_lint_assigneqexpr_bad.out new file mode 100644 index 000000000..6c7aab3b8 --- /dev/null +++ b/test_regress/t/t_lint_assigneqexpr_bad.out @@ -0,0 +1,11 @@ +%Warning-ASSIGNEQEXPR: t/t_lint_assigneqexpr.v:28:11: Assignment '=' inside expression + : ... Was a '==' intended, or suggest use a separate statement + 28 | c_o = 1 + | ^ + ... For warning description see https://verilator.org/warn/ASSIGNEQEXPR?v=latest + ... Use "/* verilator lint_off ASSIGNEQEXPR */" and lint_on around source to disable this message. +%Warning-ASSIGNEQEXPR: t/t_lint_assigneqexpr.v:30:11: Assignment '=' inside expression + : ... Was a '==' intended, or suggest use a separate statement + 30 | c_o = 0 + | ^ +%Error: Exiting due to diff --git a/test_regress/t/t_lint_assigneqexpr_bad.py b/test_regress/t/t_lint_assigneqexpr_bad.py new file mode 100755 index 000000000..8b0058925 --- /dev/null +++ b/test_regress/t/t_lint_assigneqexpr_bad.py @@ -0,0 +1,29 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2025 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 + +import vltest_bootstrap + +test.scenarios('vlt') +test.top_filename = "t/t_lint_assigneqexpr.v" + +root = ".." + +test.lint(verilator_flags2=['-Wall -Wno-DECLFILENAME'], + fails=True, + expect_filename=test.golden_filename) + +test.extract(in_filename=test.top_filename, + out_filename=root + "/docs/gen/ex_ASSIGNEQEXPR_faulty.rst", + lines="26-29") + +test.extract(in_filename=test.golden_filename, + out_filename=root + "/docs/gen/ex_ASSIGNEQEXPR_msg.rst", + lines="7-8") + +test.passes() diff --git a/test_regress/t/t_nba_assign_on_rhs.v b/test_regress/t/t_nba_assign_on_rhs.v index ed6d0ee84..312ee0366 100644 --- a/test_regress/t/t_nba_assign_on_rhs.v +++ b/test_regress/t/t_nba_assign_on_rhs.v @@ -21,7 +21,9 @@ module t (/*AUTOARG*/ x <= 1; end else if (cyc == 1) begin + // verilator lint_off ASSIGNEQEXPR y <= (x = 2); + // verilator lint_on ASSIGNEQEXPR end else begin if (x != 2) $stop; if (y != 2) $stop;