Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support specific params to be deprecated and properly deprecate instance.new's second param #594

Merged
merged 6 commits into from
Apr 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion docs/src/usage/std.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
---
Expand Down
5 changes: 5 additions & 0 deletions selene-lib/default_std/roblox_base.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
69 changes: 61 additions & 8 deletions selene-lib/src/lints/deprecated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*, *};

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -94,7 +99,7 @@ impl<'a> DeprecatedVisitor<'a> {
node: &N,
what: &str,
name_path: &[String],
parameters: &[String],
arguments: &[Argument],
) {
assert!(!name_path.is_empty());

Expand All @@ -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}"));
}

Expand All @@ -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(),
));
};
}
}
}
}

Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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(
Expand All @@ -242,6 +294,7 @@ mod tests {
"deprecated_allowed".to_owned(),
"more.*".to_owned(),
"wow.*.deprecated_allowed".to_owned(),
"deprecated_param".to_owned(),
],
})
.unwrap(),
Expand Down
4 changes: 4 additions & 0 deletions selene-lib/src/standard_library/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
1 change: 1 addition & 0 deletions selene-lib/src/standard_library/v1_upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions selene-lib/tests/lints/deprecated/deprecated_params.lua
Original file line number Diff line number Diff line change
@@ -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 {}
17 changes: 17 additions & 0 deletions selene-lib/tests/lints/deprecated/deprecated_params.std.yml
Original file line number Diff line number Diff line change
@@ -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
40 changes: 40 additions & 0 deletions selene-lib/tests/lints/deprecated/deprecated_params.stderr
Original file line number Diff line number Diff line change
@@ -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

2 changes: 2 additions & 0 deletions selene-lib/tests/lints/deprecated/specific_allow.lua
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,5 @@ more.deprecated_allowed()
wow.extra.deprecated_allowed()

deprecated_allowed.more()

deprecated_param(1)
7 changes: 7 additions & 0 deletions selene-lib/tests/lints/deprecated/specific_allow.std.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,10 @@ globals:
args: []
deprecated:
message: "this is deprecated"
deprecated_param:
args:
- required: false
type:
display: any
deprecated:
message: this is deprecated
24 changes: 19 additions & 5 deletions selene/src/roblox/generate_std.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ impl RobloxGenerator {
argument_type: ArgumentType::Any,
required: Required::NotRequired,
observes: Observes::ReadWrite,
deprecated: None,
})
.collect(),
method: true,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
Loading