Support specific params to be deprecated and properly deprecate insta…

…nce.new's second param (#594) Closes #436, closes #433.
Kampfkarren · Apr 28, 2024 · abf408d · abf408d
1 parent e73cccc
commit abf408d
Show file tree

Hide file tree

Showing 12 changed files with 167 additions and 14 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,9 +4,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ## [Unreleased](https://github.com/Kampfkarren/selene/compare/0.26.1...HEAD)
 ### Added
 - Added `CFrame.lookAlong` to the Roblox standard library
+- Added `deprecated` config field to standard library function parameters
 
 ### Changed
 - Updated the warning message for the `mixed_table` lint to include why mixed tables should be avoided
+- Properly deprecated `Instance.new`'s second argument in the Roblox standard library
 
 ## [0.26.1](https://github.com/Kampfkarren/selene/releases/tag/0.26.1) - 2023-11-11
 ### Fixed

diff --git a/docs/src/usage/std.md b/docs/src/usage/std.md
@@ -154,7 +154,7 @@ globals:
 A field is understood as a table if it has fields of its own. Notice that `math` is not defined anywhere, but its fields are. This will create an implicit `math` with the property writability of `read-only`.
 
 ### Deprecated
-Any field can have a deprecation notice added to it, which will then be read by [the deprecated lint](../lints/deprecated.md).
+Any field or arg can have a deprecation notice added to it, which will then be read by [the deprecated lint](../lints/deprecated.md).
 
 ```yaml
 ---

diff --git a/selene-lib/default_std/roblox_base.yml b/selene-lib/default_std/roblox_base.yml
@@ -320,6 +320,11 @@ globals:
   Instance.new:
     args:
       - type: string
+      - required: false
+        type:
+          display: Instance
+        deprecated:
+          message: set the instance's parent separately
     # This is only must_use because we don't allow the second parameter
     must_use: true
   NumberRange.new:

diff --git a/selene-lib/src/lints/deprecated.rs b/selene-lib/src/lints/deprecated.rs
@@ -3,7 +3,7 @@ use std::convert::Infallible;
 use full_moon::{ast, visitors::Visitor};
 use serde::Deserialize;
 
-use crate::ast_util::{name_paths::*, scopes::ScopeManager};
+use crate::ast_util::{name_paths::*, range, scopes::ScopeManager};
 
 use super::{super::standard_library::*, *};
 
@@ -48,6 +48,11 @@ struct DeprecatedVisitor<'a> {
     standard_library: &'a StandardLibrary,
 }
 
+struct Argument {
+    display: String,
+    range: (usize, usize),
+}
+
 impl<'a> DeprecatedVisitor<'a> {
     fn new(
         config: &DeprecatedLintConfig,
@@ -94,7 +99,7 @@ impl<'a> DeprecatedVisitor<'a> {
         node: &N,
         what: &str,
         name_path: &[String],
-        parameters: &[String],
+        arguments: &[Argument],
     ) {
         assert!(!name_path.is_empty());
 
@@ -115,7 +120,12 @@ impl<'a> DeprecatedVisitor<'a> {
 
             let mut notes = vec![deprecated.message.to_owned()];
 
-            if let Some(replace_with) = deprecated.try_instead(parameters) {
+            if let Some(replace_with) = deprecated.try_instead(
+                &arguments
+                    .iter()
+                    .map(|arg| arg.display.clone())
+                    .collect::<Vec<_>>(),
+            ) {
                 notes.push(format!("try: {replace_with}"));
             }
 
@@ -130,6 +140,28 @@ impl<'a> DeprecatedVisitor<'a> {
                 Vec::new(),
             ));
         }
+
+        if let Some(Field {
+            field_kind: FieldKind::Function(function),
+            ..
+        }) = self.standard_library.find_global(name_path)
+        {
+            for (arg, arg_std) in arguments
+                .iter()
+                .zip(&function.arguments)
+                .filter(|(arg, _)| arg.display != "nil")
+            {
+                if let Some(deprecated) = &arg_std.deprecated {
+                    self.diagnostics.push(Diagnostic::new_complete(
+                        "deprecated",
+                        "this parameter is deprecated".to_string(),
+                        Label::new(arg.range),
+                        vec![deprecated.message.clone()],
+                        Vec::new(),
+                    ));
+                };
+            }
+        }
     }
 }
 
@@ -194,21 +226,32 @@ impl Visitor for DeprecatedVisitor<'_> {
             feature = "force_exhaustive_checks",
             deny(non_exhaustive_omitted_patterns)
         )]
-        let argument_displays = match function_args {
+        let arguments = match function_args {
             ast::FunctionArgs::Parentheses { arguments, .. } => arguments
                 .iter()
-                .map(|argument| argument.to_string())
+                .map(|argument| Argument {
+                    display: argument.to_string().trim_end().to_string(),
+                    range: range(argument),
+                })
                 .collect(),
 
-            ast::FunctionArgs::String(token) => vec![token.to_string()],
+            ast::FunctionArgs::String(token) => vec![
+                (Argument {
+                    display: token.to_string(),
+                    range: range(token),
+                }),
+            ],
             ast::FunctionArgs::TableConstructor(table_constructor) => {
-                vec![table_constructor.to_string()]
+                vec![Argument {
+                    display: table_constructor.to_string(),
+                    range: range(table_constructor),
+                }]
             }
 
             _ => Vec::new(),
         };
 
-        self.check_name_path(call, "function", &name_path, &argument_displays);
+        self.check_name_path(call, "function", &name_path, &arguments);
     }
 }
 
@@ -234,6 +277,15 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_deprecated_params() {
+        test_lint(
+            DeprecatedLint::new(DeprecatedLintConfig::default()).unwrap(),
+            "deprecated",
+            "deprecated_params",
+        );
+    }
+
     #[test]
     fn test_specific_allow() {
         test_lint(
@@ -242,6 +294,7 @@ mod tests {
                     "deprecated_allowed".to_owned(),
                     "more.*".to_owned(),
                     "wow.*.deprecated_allowed".to_owned(),
+                    "deprecated_param".to_owned(),
                 ],
             })
             .unwrap(),

diff --git a/selene-lib/src/standard_library/mod.rs b/selene-lib/src/standard_library/mod.rs
@@ -603,6 +603,10 @@ pub struct Argument {
     #[serde(default)]
     #[serde(skip_serializing_if = "is_default")]
     pub observes: Observes,
+
+    #[serde(default)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub deprecated: Option<Deprecated>,
 }
 
 #[derive(Clone, Debug, Hash, PartialEq, Eq)]

diff --git a/selene-lib/src/standard_library/v1_upgrade.rs b/selene-lib/src/standard_library/v1_upgrade.rs
@@ -41,6 +41,7 @@ impl From<v1::Argument> for Argument {
             required: v1_argument.required.into(),
             argument_type: v1_argument.argument_type.into(),
             observes: Observes::ReadWrite,
+            deprecated: None,
         }
     }
 }

diff --git a/selene-lib/tests/lints/deprecated/deprecated_params.lua b/selene-lib/tests/lints/deprecated/deprecated_params.lua
@@ -0,0 +1,8 @@
+local _ = Instance.new(a)
+local _ = Instance.new(a, b)
+local _ = Instance.new(a,    nil     )
+local _ = Instance.new(a, "nil")
+
+a(1)
+a ""
+a {}
diff --git a/selene-lib/tests/lints/deprecated/deprecated_params.std.yml b/selene-lib/tests/lints/deprecated/deprecated_params.std.yml
@@ -0,0 +1,17 @@
+---
+globals:
+  Instance.new:
+    args:
+      - type: string
+      - required: false
+        type:
+          display: Instance
+        deprecated:
+          message: set the instance's parent separately
+  a:
+    args:
+      - required: false
+        type:
+          display: any
+        deprecated:
+          message: this is deprecated
diff --git a/selene-lib/tests/lints/deprecated/deprecated_params.stderr b/selene-lib/tests/lints/deprecated/deprecated_params.stderr
@@ -0,0 +1,40 @@
+error[deprecated]: this parameter is deprecated
+  ┌─ deprecated_params.lua:2:27
+  │
+2 │ local _ = Instance.new(a, b)
+  │                           ^
+  │
+  = set the instance's parent separately
+
+error[deprecated]: this parameter is deprecated
+  ┌─ deprecated_params.lua:4:27
+  │
+4 │ local _ = Instance.new(a, "nil")
+  │                           ^^^^^
+  │
+  = set the instance's parent separately
+
+error[deprecated]: this parameter is deprecated
+  ┌─ deprecated_params.lua:6:3
+  │
+6 │ a(1)
+  │   ^
+  │
+  = this is deprecated
+
+error[deprecated]: this parameter is deprecated
+  ┌─ deprecated_params.lua:7:3
+  │
+7 │ a ""
+  │   ^^
+  │
+  = this is deprecated
+
+error[deprecated]: this parameter is deprecated
+  ┌─ deprecated_params.lua:8:3
+  │
+8 │ a {}
+  │   ^^
+  │
+  = this is deprecated
+
diff --git a/selene-lib/tests/lints/deprecated/specific_allow.lua b/selene-lib/tests/lints/deprecated/specific_allow.lua
@@ -5,3 +5,5 @@ more.deprecated_allowed()
 wow.extra.deprecated_allowed()
 
 deprecated_allowed.more()
+
+deprecated_param(1)
diff --git a/selene-lib/tests/lints/deprecated/specific_allow.std.yml b/selene-lib/tests/lints/deprecated/specific_allow.std.yml
@@ -16,3 +16,10 @@ globals:
     args: []
     deprecated:
       message: "this is deprecated"
+  deprecated_param:
+    args:
+      - required: false
+        type:
+          display: any
+        deprecated:
+          message: this is deprecated
diff --git a/selene/src/roblox/generate_std.rs b/selene/src/roblox/generate_std.rs
@@ -125,6 +125,7 @@ impl RobloxGenerator {
                                     argument_type: ArgumentType::Any,
                                     required: Required::NotRequired,
                                     observes: Observes::ReadWrite,
+                                    deprecated: None,
                                 })
                                 .collect(),
                             method: true,
@@ -260,11 +261,23 @@ impl RobloxGenerator {
         self.std.globals.insert(
             "Instance.new".to_owned(),
             Field::from_field_kind(FieldKind::Function(FunctionBehavior {
-                arguments: vec![Argument {
-                    argument_type: ArgumentType::Constant(instance_names),
-                    required: Required::Required(None),
-                    observes: Observes::ReadWrite,
-                }],
+                arguments: vec![
+                    Argument {
+                        argument_type: ArgumentType::Constant(instance_names),
+                        required: Required::Required(None),
+                        observes: Observes::ReadWrite,
+                        deprecated: None,
+                    },
+                    Argument {
+                        argument_type: ArgumentType::Display("Instance".to_string()),
+                        required: Required::Required(None),
+                        observes: Observes::ReadWrite,
+                        deprecated: Some(Deprecated {
+                            message: "set the instance's parent separately".to_owned(),
+                            replace: vec![],
+                        }),
+                    },
+                ],
                 method: false,
 
                 // Only true because we don't allow the second parameter
@@ -294,6 +307,7 @@ impl RobloxGenerator {
                     argument_type: ArgumentType::Constant(service_names),
                     required: Required::Required(None),
                     observes: Observes::ReadWrite,
+                    deprecated: None,
                 }],
                 method: true,
                 must_use: true,