Merge pull request #280 from ronitnallagatla/master

New Rule: Forbid unpacked array declarations

MANUAL.md

@@ -5678,6 +5678,91 @@ The most relevant clauses of IEEE1800-2017 are:
 - 12.7 Loop statements
 
 
+
+* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
+
+## Syntax Rule: `unpacked_array`
+
+### Hint
+
+Avoid using unpacked dimensions in declarations.
+
+### Reason
+
+Unpacked arrays are not guaranteed to be contiguous and can lead to synthesis issues.
+
+### Pass Example (1 of 2)
+```systemverilog
+module M;
+
+logic [31:0] a;
+
+endmodule
+```
+
+### Pass Example (2 of 2)
+```systemverilog
+module M;
+
+logic [7:0][3:0] b;
+
+endmodule
+```
+
+### Fail Example (1 of 2)
+```systemverilog
+module M;
+
+logic a [7:0];
+
+endmodule;
+```
+
+### Fail Example (2 of 2)
+```systemverilog
+module M;
+
+logic [31:0] b [0:7];
+
+endmodule;
+```
+
+### Explanation
+
+This rule forbids unpacked array declarations.
+
+Unpacked arrays are not guaranteed to be represented as contiguous memory, and
+can cause issues with synthesis tools, especially with how multidimensional
+arrays are synthesized. For example, a synthesis tool might synthesize out
+unused memory locations of an unpacked array which is not the intended behavior.
+
+Additionally, packed arrays allow the user to intuitively index and slice the
+array and apply bitwise operations.
+
+This rule by default targets data declarations, but can be configured to target
+other declarations. To target a declaration, enable the corresponding boolean
+option in the configuration file.
+
+``` toml
+[option.unpacked_array]
+localparam_declaration  = false
+param_declaration  = false
+specparam_declaration  = false
+inout_declaration  = false
+ansi_port_declaration  = false
+input_declaration  = false
+output_declaration  = false
+intf_port_declaration  = false
+ref_declaration  = false
+data_declaration  = true # enabled by default
+net_declaration = false
+```
+
+The most relevant clauses of IEEE1800-2017 are:
+
+- 7.4 Packed and unpacked arrays
+
+
 # Naming Convention Syntax Rules
 
 Rules for checking against naming conventions are named with either the suffix

md/syntaxrules-explanation-unpacked_array.md

@@ -0,0 +1,32 @@
+This rule forbids unpacked array declarations.
+
+Unpacked arrays are not guaranteed to be represented as contiguous memory, and
+can cause issues with synthesis tools, especially with how multidimensional
+arrays are synthesized. For example, a synthesis tool might synthesize out
+unused memory locations of an unpacked array which is not the intended behavior.
+
+Additionally, packed arrays allow the user to intuitively index and slice the
+array and apply bitwise operations.
+
+This rule by default targets data declarations, but can be configured to target
+other declarations. To target a declaration, enable the corresponding boolean
+option in the configuration file.
+
+``` toml
+[option.unpacked_array]
+localparam_declaration  = false
+param_declaration  = false
+specparam_declaration  = false
+inout_declaration  = false
+ansi_port_declaration  = false
+input_declaration  = false
+output_declaration  = false
+intf_port_declaration  = false
+ref_declaration  = false
+data_declaration  = true # enabled by default
+net_declaration = false
+```
+
+The most relevant clauses of IEEE1800-2017 are:
+
+- 7.4 Packed and unpacked arrays

src/config.rs

@@ -1,4 +1,4 @@
-use crate::linter::{TextRule, SyntaxRule};
+use crate::linter::{SyntaxRule, TextRule};
 use crate::rules::*;
 use regex::Regex;
 use serde_derive::{Deserialize, Serialize};
@@ -158,6 +158,35 @@ pub struct ConfigOption {
     pub re_forbidden_var_class: String,
     #[serde(default = "default_re_unconfigured")]
     pub re_forbidden_var_classmethod: String,
+
+    #[serde(default)]
+    pub unpacked_array: UnpackedArrayOption,
+}
+
+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub struct UnpackedArrayOption {
+    #[serde(default)]
+    pub localparam_declaration: bool,
+    #[serde(default)]
+    pub param_declaration: bool,
+    #[serde(default)]
+    pub specparam_declaration: bool,
+    #[serde(default)]
+    pub inout_declaration: bool,
+    #[serde(default)]
+    pub ansi_port_declaration: bool,
+    #[serde(default)]
+    pub input_declaration: bool,
+    #[serde(default)]
+    pub output_declaration: bool,
+    #[serde(default)]
+    pub interface_port_declaration: bool,
+    #[serde(default)]
+    pub ref_declaration: bool,
+    #[serde(default)]
+    pub data_declaration: bool,
+    #[serde(default)]
+    pub net_declaration: bool,
 }
 
 include!(concat!(env!("OUT_DIR"), "/config_rules.rs"));
@@ -180,6 +209,24 @@ impl Default for ConfigSyntaxRules {
     }
 }
 
+impl Default for UnpackedArrayOption {
+    fn default() -> Self {
+        Self {
+            localparam_declaration: false,
+            param_declaration: false,
+            specparam_declaration: false,
+            inout_declaration: false,
+            ansi_port_declaration: false,
+            input_declaration: false,
+            output_declaration: false,
+            interface_port_declaration: false,
+            ref_declaration: false,
+            data_declaration: true,
+            net_declaration: false,
+        }
+    }
+}
+
 #[allow(dead_code)]
 fn default_as_true() -> bool {
     true

src/syntaxrules/unpacked_array.rs

@@ -0,0 +1,139 @@
+use crate::config::ConfigOption;
+use crate::linter::{SyntaxRule, SyntaxRuleResult};
+use sv_parser::{NodeEvent, RefNode, SyntaxTree};
+
+#[derive(Default)]
+pub struct UnpackedArray {
+    under_localparam_decl: bool,
+    under_param_decl: bool,
+    under_specparam_decl: bool,
+    under_inout_decl: bool,
+    under_ansi_port_decl: bool,
+    under_input_decl: bool,
+    under_output_decl: bool,
+    under_intf_port_decl: bool,
+    under_ref_decl: bool,
+    under_data_decl: bool,
+    under_net_decl: bool,
+}
+
+impl SyntaxRule for UnpackedArray {
+    fn check(
+        &mut self,
+        _syntax_tree: &SyntaxTree,
+        event: &NodeEvent,
+        option: &ConfigOption,
+    ) -> SyntaxRuleResult {
+        let node = match event {
+            NodeEvent::Enter(x) => {
+                match x {
+                    RefNode::LocalParameterDeclaration(_) => {
+                        self.under_localparam_decl = true;
+                    }
+                    RefNode::ParameterDeclaration(_) => {
+                        self.under_param_decl = true;
+                    }
+                    RefNode::SpecparamDeclaration(_) => {
+                        self.under_specparam_decl = true;
+                    }
+                    RefNode::InoutDeclaration(_) => {
+                        self.under_inout_decl = true;
+                    }
+                    RefNode::AnsiPortDeclaration(_) => {
+                        self.under_ansi_port_decl = true;
+                    }
+                    RefNode::InputDeclaration(_) => {
+                        self.under_input_decl = true;
+                    }
+                    RefNode::OutputDeclaration(_) => {
+                        self.under_output_decl = true;
+                    }
+                    RefNode::InterfacePortDeclaration(_) => {
+                        self.under_intf_port_decl = true;
+                    }
+                    RefNode::RefDeclaration(_) => {
+                        self.under_ref_decl = true;
+                    }
+                    RefNode::DataDeclaration(_) => {
+                        self.under_data_decl = true;
+                    }
+                    RefNode::NetDeclaration(_) => {
+                        self.under_net_decl = true;
+                    }
+
+                    _ => (),
+                }
+                x
+            }
+            NodeEvent::Leave(x) => {
+                match x {
+                    RefNode::LocalParameterDeclaration(_) => {
+                        self.under_localparam_decl = false;
+                    }
+                    RefNode::ParameterDeclaration(_) => {
+                        self.under_param_decl = false;
+                    }
+                    RefNode::SpecparamDeclaration(_) => {
+                        self.under_specparam_decl = false;
+                    }
+                    RefNode::InoutDeclaration(_) => {
+                        self.under_inout_decl = false;
+                    }
+                    RefNode::InputDeclaration(_) => {
+                        self.under_input_decl = false;
+                    }
+                    RefNode::OutputDeclaration(_) => {
+                        self.under_output_decl = false;
+                    }
+                    RefNode::InterfacePortDeclaration(_) => {
+                        self.under_intf_port_decl = false;
+                    }
+                    RefNode::RefDeclaration(_) => {
+                        self.under_ref_decl = false;
+                    }
+                    RefNode::DataDeclaration(_) => {
+                        self.under_data_decl = false;
+                    }
+                    RefNode::NetDeclaration(_) => {
+                        self.under_net_decl = false;
+                    }
+
+                    _ => (),
+                }
+                return SyntaxRuleResult::Pass;
+            }
+        };
+
+        if let (true, RefNode::UnpackedDimension(_)) = (
+            (self.under_localparam_decl && option.unpacked_array.localparam_declaration
+                || self.under_param_decl && option.unpacked_array.param_declaration
+                || self.under_specparam_decl && option.unpacked_array.specparam_declaration
+                || self.under_inout_decl && option.unpacked_array.inout_declaration
+                || self.under_ansi_port_decl && option.unpacked_array.ansi_port_declaration
+                || self.under_input_decl && option.unpacked_array.input_declaration
+                || self.under_output_decl && option.unpacked_array.output_declaration
+                || self.under_intf_port_decl && option.unpacked_array.interface_port_declaration
+                || self.under_ref_decl && option.unpacked_array.ref_declaration
+                || self.under_data_decl && option.unpacked_array.data_declaration
+                || self.under_net_decl && option.unpacked_array.net_declaration),
+            node,
+        ) {
+            SyntaxRuleResult::Fail
+        } else {
+            SyntaxRuleResult::Pass
+        }
+    }
+    fn name(&self) -> String {
+        String::from("unpacked_array")
+    }
+
+    fn hint(&self, _option: &ConfigOption) -> String {
+        String::from("Avoid using unpacked dimensions in declarations.")
+    }
+
+    fn reason(&self) -> String {
+        String::from(
+            "Unpacked arrays are not guaranteed to be contiguous and can lead to synthesis issues.",
+        )
+    }
+}

testcases/syntaxrules/fail/unpacked_array.sv

@@ -0,0 +1,11 @@
+module M;
+
+logic a [7:0];
+
+endmodule;
+////////////////////////////////////////////////////////////////////////////////
+module M;
+
+logic [31:0] b [0:7];
+
+endmodule;

testcases/syntaxrules/pass/unpacked_array.sv

@@ -0,0 +1,11 @@
+module M;
+
+logic [31:0] a;
+
+endmodule
+////////////////////////////////////////////////////////////////////////////////
+module M;
+
+logic [7:0][3:0] b;
+
+endmodule