feat(analyser): banDropNotNull rule

crates/pg_analyser/CONTRIBUTING.md

@@ -17,12 +17,14 @@ We follow a naming convention according to what the rule does:
 1. Forbid a concept
 
    ```block
-   no<Concept>
+   ban<Concept>
    ```
 
-   When a rule's sole intention is to **forbid a single concept** the rule should be named using the `no` prefix.
+   When a rule's sole intention is to **forbid a single concept** the rule should be named using the `ban` prefix.
 
-1. Mandate a concept
+   Example: "banDropColumn"
+
+2. Mandate a concept
 
    ```block
    use<Concept>
@@ -35,9 +37,10 @@ We follow a naming convention according to what the rule does:
 A rule should be informative to the user, and give as much explanation as possible.
 
 When writing a rule, you must adhere to the following **pillars**:
+
 1. Explain to the user the error. Generally, this is the message of the diagnostic.
-1. Explain to the user **why** the error is triggered. Generally, this is implemented with an additional node.
-1. Tell the user what they should do. Generally, this is implemented using a code action. If a code action is not applicable a note should tell the user what they should do to fix the error.
+2. Explain to the user **why** the error is triggered. Generally, this is implemented with an additional node.
+3. Tell the user what they should do. Generally, this is implemented using a code action. If a code action is not applicable a note should tell the user what they should do to fix the error.
 
 ### Create and implement the rule
 
@@ -53,10 +56,11 @@ Let's say we want to create a new **lint** rule called `useMyRuleName`, follow t
    ```shell
    just new-lintrule safety useMyRuleName
    ```
+
    The script will generate a bunch of files inside the `pg_analyser` crate.
    Among the other files, you'll find a file called `use_my_new_rule_name.rs` inside the `pg_analyser/lib/src/lint/safety` folder. You'll implement your rule in this file.
 
-1. The `Option` type doesn't have to be used, so it can be considered optional. However, it has to be defined as `type Option = ()`.
+1. The `Options` type doesn't have to be used, so it can be considered optional. However, it has to be defined as `type Options = ()`.
 1. Implement the `run` function: The function is called for every statement, and should return zero or more diagnostics. Follow the [pillars](#explain-a-rule-to-the-user) when writing the message of a diagnostic
 
 Don't forget to format your code with `just f` and lint with `just l`.
@@ -107,7 +111,7 @@ pub enum Behavior {
 ```
 
 Note that we use a boxed slice `Box<[Box<str>]>` instead of `Vec<String>`.
-This allows saving memory: [boxed slices and boxed str use 2 words instead of three words](https://nnethercote.github.io/perf-book/type-sizes.html#boxed-slices).
+This allows saving memory: [boxed slices and boxed str use two instead of three words](https://nnethercote.github.io/perf-book/type-sizes.html#boxed-slices).
 
 With these types in place, you can set the associated type `Options` of the rule:
 
@@ -127,6 +131,7 @@ The compiler should warn you that `MyRuleOptions` does not implement some requir
 We currently require implementing _serde_'s traits `Deserialize`/`Serialize`.
 
 Also, we use other `serde` macros to adjust the JSON configuration:
+
 - `rename_all = "snake_case"`: it renames all fields in camel-case, so they are in line with the naming style of the `pglsp.toml`.
 - `deny_unknown_fields`: it raises an error if the configuration contains extraneous fields.
 - `default`: it uses the `Default` value when the field is missing from `pglsp.toml`. This macro makes the field optional.
@@ -159,7 +164,6 @@ pub enum Behavior {
 
 Below, there are many tips and guidelines on how to create a lint rule using our infrastructure.
 
-
 #### `declare_lint_rule`
 
 This macro is used to declare an analyzer rule type, and implement the [RuleMeta] trait for it.
@@ -235,6 +239,7 @@ impl Rule for BanDropColumn {
 ### Document the rule
 
 The documentation needs to adhere to the following rules:
+
 - The **first** paragraph of the documentation is used as brief description of the rule, and it **must** be written in one single line. Breaking the paragraph in multiple lines will break the table content of the rules page.
 - The next paragraphs can be used to further document the rule with as many details as you see fit.
 - The documentation must have a `## Examples` header, followed by two headers: `### Invalid` and `### Valid`. `### Invalid` must go first because we need to show when the rule is triggered.
@@ -246,7 +251,7 @@ The documentation needs to adhere to the following rules:
 
 Here's an example of how the documentation could look like:
 
-```rust
+````rust
 declare_lint_rule! {
     /// Dropping a column may break existing clients.
     ///
@@ -269,7 +274,7 @@ declare_lint_rule! {
         sources: &[RuleSource::Squawk("ban-drop-column")],
     }
 }
-```
+````
 
 This will cause the documentation generator to ensure the rule does emit
 exactly one diagnostic for this code, and to include a snapshot for the
@@ -294,15 +299,14 @@ Stage and commit your changes:
 > git commit -m 'feat(pg_analyser): myRuleName'
 ```
 
-
 ### Deprecate a rule
 
 There are occasions when a rule must be deprecated, to avoid breaking changes. The reason
 of deprecation can be multiple.
 
 In order to do, the macro allows adding additional field to add the reason for deprecation
 
-```rust
+````rust
 use pg_analyse::declare_lint_rule;
 
 declare_lint_rule! {
@@ -328,5 +332,4 @@ declare_lint_rule! {
         sources: &[RuleSource::Squawk("ban-drop-column")],
     }
 }
-```
-
+````

crates/pg_analyser/src/lint/safety.rs

@@ -2,4 +2,5 @@
 
 use pg_analyse::declare_lint_group;
 pub mod ban_drop_column;
-declare_lint_group! { pub Safety { name : "safety" , rules : [self :: ban_drop_column :: BanDropColumn ,] } }
+pub mod ban_drop_not_null;
+declare_lint_group! { pub Safety { name : "safety" , rules : [self :: ban_drop_column :: BanDropColumn , self :: ban_drop_not_null :: BanDropNotNull ,] } }

crates/pg_analyser/src/lint/safety/ban_drop_column.rs

@@ -1,13 +1,6 @@
 use pg_analyse::{context::RuleContext, declare_lint_rule, Rule, RuleDiagnostic, RuleSource};
 use pg_console::markup;
 
-#[derive(Clone, Debug, Default, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
-// #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
-#[serde(rename_all = "camelCase", deny_unknown_fields, default)]
-pub struct Options {
-    test: String,
-}
-
 declare_lint_rule! {
     /// Dropping a column may break existing clients.
     ///
@@ -19,7 +12,7 @@ declare_lint_rule! {
     ///
     /// ### Invalid
     ///
-    /// ```sql,expect_diagnostic
+    /// ```sql,ignore
     /// alter table test drop column id;
     /// ```
     ///
@@ -32,7 +25,7 @@ declare_lint_rule! {
 }
 
 impl Rule for BanDropColumn {
-    type Options = Options;
+    type Options = ();
 
     fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
         let mut diagnostics = Vec::new();
@@ -47,7 +40,7 @@ impl Rule for BanDropColumn {
                             markup! {
                                 "Dropping a column may break existing clients."
                             },
-                        ).detail(None, format!("[{}] You can leave the column as nullable or delete the column once queries no longer select or modify the column.", ctx.options().test)));
+                        ).detail(None, "You can leave the column as nullable or delete the column once queries no longer select or modify the column."));
                     }
                 }
             }

crates/pg_analyser/src/lint/safety/ban_drop_not_null.rs

@@ -0,0 +1,51 @@
+use pg_analyse::{context::RuleContext, declare_lint_rule, Rule, RuleDiagnostic, RuleSource};
+use pg_console::markup;
+
+declare_lint_rule! {
+    /// Dropping a NOT NULL constraint may break existing clients.
+    ///
+    /// Application code or code written in procedural languages like PL/SQL or PL/pgSQL may not expect NULL values for the column that was previously guaranteed to be NOT NULL and therefore may fail to process them correctly.
+    ///
+    /// You can consider using a marker value that represents NULL. Alternatively, create a new table allowing NULL values, copy the data from the old table, and create a view that filters NULL values.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```sql,ignore
+    /// alter table users alter column email drop not null;
+    /// ```
+    pub BanDropNotNull {
+        version: "next",
+        name: "banDropNotNull",
+        recommended: true,
+        sources: &[RuleSource::Squawk("ban-drop-not-null")],
+
+    }
+}
+
+impl Rule for BanDropNotNull {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        let mut diagnostics = Vec::new();
+
+        if let pg_query_ext::NodeEnum::AlterTableStmt(stmt) = &ctx.stmt() {
+            for cmd in &stmt.cmds {
+                if let Some(pg_query_ext::NodeEnum::AlterTableCmd(cmd)) = &cmd.node {
+                    if cmd.subtype() == pg_query_ext::protobuf::AlterTableType::AtDropNotNull {
+                        diagnostics.push(RuleDiagnostic::new(
+                            rule_category!(),
+                            None,
+                            markup! {
+                                "Dropping a NOT NULL constraint may break existing clients."
+                            },
+                        ).detail(None, "Consider using a marker value that represents NULL. Alternatively, create a new table allowing NULL values, copy the data from the old table, and create a view that filters NULL values."));
+                    }
+                }
+            }
+        }
+
+        diagnostics
+    }
+}

crates/pg_analyser/src/options.rs

@@ -3,3 +3,5 @@
 use crate::lint;
 pub type BanDropColumn =
     <lint::safety::ban_drop_column::BanDropColumn as pg_analyse::Rule>::Options;
+pub type BanDropNotNull =
+    <lint::safety::ban_drop_not_null::BanDropNotNull as pg_analyse::Rule>::Options;

crates/pg_configuration/src/analyser/linter/rules.rs

@@ -146,15 +146,22 @@ pub struct Safety {
     #[doc = "Dropping a column may break existing clients."]
     #[serde(skip_serializing_if = "Option::is_none")]
     pub ban_drop_column: Option<RuleConfiguration<pg_analyser::options::BanDropColumn>>,
+    #[doc = "Dropping a NOT NULL constraint may break existing clients."]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub ban_drop_not_null: Option<RuleConfiguration<pg_analyser::options::BanDropNotNull>>,
 }
 impl Safety {
     const GROUP_NAME: &'static str = "safety";
-    pub(crate) const GROUP_RULES: &'static [&'static str] = &["banDropColumn"];
-    const RECOMMENDED_RULES: &'static [&'static str] = &["banDropColumn"];
-    const RECOMMENDED_RULES_AS_FILTERS: &'static [RuleFilter<'static>] =
-        &[RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0])];
-    const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] =
-        &[RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0])];
+    pub(crate) const GROUP_RULES: &'static [&'static str] = &["banDropColumn", "banDropNotNull"];
+    const RECOMMENDED_RULES: &'static [&'static str] = &["banDropColumn", "banDropNotNull"];
+    const RECOMMENDED_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[
+        RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]),
+        RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]),
+    ];
+    const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[
+        RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]),
+        RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]),
+    ];
     #[doc = r" Retrieves the recommended rules"]
     pub(crate) fn is_recommended_true(&self) -> bool {
         matches!(self.recommended, Some(true))
@@ -175,6 +182,11 @@ impl Safety {
                 index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]));
             }
         }
+        if let Some(rule) = self.ban_drop_not_null.as_ref() {
+            if rule.is_enabled() {
+                index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]));
+            }
+        }
         index_set
     }
     pub(crate) fn get_disabled_rules(&self) -> FxHashSet<RuleFilter<'static>> {
@@ -184,6 +196,11 @@ impl Safety {
                 index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]));
             }
         }
+        if let Some(rule) = self.ban_drop_not_null.as_ref() {
+            if rule.is_disabled() {
+                index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]));
+            }
+        }
         index_set
     }
     #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"]
@@ -224,6 +241,10 @@ impl Safety {
                 .ban_drop_column
                 .as_ref()
                 .map(|conf| (conf.level(), conf.get_options())),
+            "banDropNotNull" => self
+                .ban_drop_not_null
+                .as_ref()
+                .map(|conf| (conf.level(), conf.get_options())),
             _ => None,
         }
     }

crates/pg_diagnostics_categories/src/categories.rs

@@ -14,6 +14,7 @@
 
 define_categories! {
     "lint/safety/banDropColumn": "https://pglsp.dev/linter/rules/ban-drop-column",
+    "lint/safety/banDropNotNull": "https://pglsp.dev/linter/rules/ban-drop-not-null",
     // end lint rules
     ;
     // General categories

pglsp.toml

@@ -25,6 +25,6 @@ recommended = true
 
 [linter.rules.safety.banDropColumn]
 level = "warn"
-options = { test = "HELLO" }
-
 
+[linter.rules.safety.banDropNotNull]
+level = "warn"