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

[WIP] [Refactor] Defining clippy rules #91

Merged
merged 14 commits into from
Dec 4, 2023
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
31 changes: 24 additions & 7 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,14 +1,31 @@
[workspace]
members = [
"parser",
"parser/visitable-derive",
"parser/generator",
"checker",
"checker/binary-serialize-derive",
# "lsp/server",
# "checker/specification"
"parser",
"parser/visitable-derive",
"parser/generator",
"checker",
"checker/binary-serialize-derive",
# "lsp/server",
# "checker/specification"
]


[workspace.lints.clippy]
all = "deny"
pedantic = "deny"
cast_precision_loss = "warn"
cast_possible_truncation = "warn"
cast_sign_loss = "warn"
default_trait_access = "allow"
missing_errors_doc = "allow"
missing_panics_doc = "allow"
implicit_hasher = "allow"
module_name_repetitions = "allow"
too_many_lines = "allow"
new_without_default = "allow"
result_unit_err = "allow"


[package]
name = "ezno"
description = "A JavaScript type checker and compiler. For use as a library or through the CLI"
Expand Down
5 changes: 4 additions & 1 deletion checker/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ categories = ["compilers"]

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[lints]
workspace = true

[features]
default = []
ezno-parser = ["parser"]
Expand All @@ -23,7 +26,7 @@ binary-serialize-derive = { path = "./binary-serialize-derive", version = "0.0.1
temporary-annex = "0.1.0"
derive-enum-from-into = "0.1.0"
derive-debug-extras = { version = "0.2.0", features = [
"auto-debug-single-tuple-inline",
"auto-debug-single-tuple-inline",
] }
enum-variants-strings = "0.2"
iterator-endiate = "0.2"
Expand Down
10 changes: 5 additions & 5 deletions checker/examples/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ fn main() {

let (diagnostics, post_check_data) = check_project::<_, synthesis::EznoParser>(
path.to_path_buf(),
HashSet::from_iter(std::iter::once(ezno_checker::INTERNAL_DEFINITION_FILE_PATH.into())),
std::iter::once(ezno_checker::INTERNAL_DEFINITION_FILE_PATH.into()).collect(),
|path: &std::path::Path| {
if path == PathBuf::from(ezno_checker::INTERNAL_DEFINITION_FILE_PATH) {
Some(ezno_checker::INTERNAL_DEFINITION_FILE.to_owned())
Expand All @@ -30,22 +30,22 @@ fn main() {
if args.iter().any(|arg| arg == "--types") {
eprintln!("Types:");
for item in post_check_data.types.into_vec_temp() {
eprintln!("\t{:?}", item);
eprintln!("\t{item:?}");
}
}
if args.iter().any(|arg| arg == "--events") {
eprintln!("Events on entry:");
let entry_module =
post_check_data.modules.remove(&post_check_data.entry_source).unwrap();
for item in entry_module.facts.get_events() {
eprintln!("\t{:?}", item);
eprintln!("\t{item:?}");
}
}
}

eprintln!("Diagnostics:");
for diagnostic in diagnostics.into_iter() {
eprintln!("\t{}", diagnostic.reason())
for diagnostic in diagnostics {
eprintln!("\t{}", diagnostic.reason());
}
}

Expand Down
7 changes: 4 additions & 3 deletions checker/specification/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,14 @@ fn markdown_lines_append_test_to_rust(
let blocks = {
let mut blocks = Vec::new();
let mut current_filename = None;
while let Some((_, line)) = lines.next() {
for (_, line) in lines.by_ref() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I have used .by_ref() on iterator methods that consume brefore. Didn't know it could be used in the case where you don't want for to consume 👍

if line == "```ts" {
break;
}
}
let mut code = String::new();
while let Some((_, line)) = lines.next() {

for (_, line) in lines.by_ref() {
if let Some(path) = line.strip_prefix("// in ") {
if !code.trim().is_empty() {
blocks.push((
Expand All @@ -93,7 +94,7 @@ fn markdown_lines_append_test_to_rust(
};
let errors = {
let mut errors = Vec::new();
while let Some((_, line)) = lines.next() {
for (_, line) in lines.by_ref() {
if line.is_empty() || !line.starts_with('-') {
if !errors.is_empty() {
break;
Expand Down
16 changes: 7 additions & 9 deletions checker/specification/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,13 @@ fn check_errors(
|path| {
if path == std::path::Path::new(checker::INTERNAL_DEFINITION_FILE_PATH) {
Some(checker::INTERNAL_DEFINITION_FILE.to_owned())
} else if code.len() == 1 {
Some(code[0].1.to_owned())
} else {
if code.len() == 1 {
Some(code[0].1.to_owned())
} else {
code.iter().find_map(|(code_path, content)| {
(std::path::Path::new(code_path) == path)
.then_some(content.to_owned().to_owned())
})
}
code.iter().find_map(|(code_path, content)| {
(std::path::Path::new(code_path) == path)
.then_some(content.to_owned().to_owned())
})
}
},
type_check_options,
Expand All @@ -91,7 +89,7 @@ fn check_errors(
})
.collect();

if &diagnostics != expected_diagnostics {
if diagnostics != expected_diagnostics {
panic!(
"{}",
pretty_assertions::Comparison::new(expected_diagnostics, &diagnostics).to_string()
Expand Down
1 change: 1 addition & 0 deletions checker/src/behavior/assignments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ pub enum AssignmentReturnStatus {
}

impl Reference {
#[must_use]
pub fn get_position(&self) -> SpanWithSource {
match self {
Reference::Variable(_, span) | Reference::Property { span, .. } => span.clone(),
Expand Down
16 changes: 8 additions & 8 deletions checker/src/behavior/constant_functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub(crate) fn call_constant_function(
.last()
.ok_or(ConstantFunctionError::BadCall)?
.to_type()
.map_err(|_| ConstantFunctionError::BadCall)?,
.map_err(|()| ConstantFunctionError::BadCall)?,
);

let Type::Constant(Constant::Number(num)) = second_argument_type else {
Expand Down Expand Up @@ -115,7 +115,7 @@ pub(crate) fn call_constant_function(
.first()
.ok_or(ConstantFunctionError::BadCall)?
.to_type()
.map_err(|_| ConstantFunctionError::BadCall)?;
.map_err(|()| ConstantFunctionError::BadCall)?;
let ty_as_string = print_type(ty, types, &environment.as_general_context(), debug);
Ok(ConstantOutput::Diagnostic(format!("Type is: {ty_as_string}")))
}
Expand All @@ -124,14 +124,14 @@ pub(crate) fn call_constant_function(
.first()
.ok_or(ConstantFunctionError::BadCall)?
.to_type()
.map_err(|_| ConstantFunctionError::BadCall)?;
.map_err(|()| ConstantFunctionError::BadCall)?;
if let Type::Function(func, _) | Type::FunctionReference(func, _) =
types.get_type_by_id(ty)
{
let effects =
&types.functions.get(func).ok_or(ConstantFunctionError::BadCall)?.effects;
// TODO print using a different function
Ok(ConstantOutput::Diagnostic(format!("{:#?}", effects)))
Ok(ConstantOutput::Diagnostic(format!("{effects:#?}")))
} else {
Ok(ConstantOutput::Diagnostic("not a function".to_owned()))
}
Expand All @@ -145,7 +145,7 @@ pub(crate) fn call_constant_function(
Some(this_ty),
) = (on, first_argument)
{
let type_id = this_ty.to_type().map_err(|_| ConstantFunctionError::BadCall)?;
let type_id = this_ty.to_type().map_err(|()| ConstantFunctionError::BadCall)?;
let value = types.register_type(Type::Function(*func, ThisValue::Passed(type_id)));
Ok(ConstantOutput::Value(value))
} else {
Expand All @@ -171,7 +171,7 @@ pub(crate) fn call_constant_function(
.facts
.prototypes
.get(&first.to_type().unwrap())
.cloned()
.copied()
.unwrap_or(TypeId::NULL_TYPE);
Ok(ConstantOutput::Value(prototype))
} else {
Expand All @@ -197,7 +197,7 @@ pub(crate) fn call_constant_function(
.first()
.ok_or(ConstantFunctionError::BadCall)?
.to_type()
.map_err(|_| ConstantFunctionError::BadCall)?;
.map_err(|()| ConstantFunctionError::BadCall)?;
// TODO temp!!!
let arg = call_site_type_args
.iter()
Expand Down Expand Up @@ -235,7 +235,7 @@ pub(crate) fn call_constant_function(
.first()
.ok_or(ConstantFunctionError::BadCall)?
.to_type()
.map_err(|_| ConstantFunctionError::BadCall)?
.map_err(|()| ConstantFunctionError::BadCall)?
)
.is_dependent()
))),
Expand Down
16 changes: 9 additions & 7 deletions checker/src/behavior/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,20 @@ pub enum ThisValue {

impl ThisValue {
pub(crate) fn get(
&self,
self,
environment: &mut Environment,
types: &TypeStore,
position: SpanWithSource,
position: &SpanWithSource,
) -> TypeId {
match self {
ThisValue::Passed(value) => *value,
ThisValue::Passed(value) => value,
ThisValue::UseParent => environment.get_value_of_this(types, position),
}
}

pub(crate) fn get_passed(&self) -> Option<TypeId> {
pub(crate) fn get_passed(self) -> Option<TypeId> {
match self {
ThisValue::Passed(value) => Some(*value),
ThisValue::Passed(value) => Some(value),
ThisValue::UseParent => None,
}
}
Expand Down Expand Up @@ -115,7 +115,7 @@ pub fn synthesise_hoisted_statement_function<T: crate::ReadFromFS, M: crate::AST
}

pub fn function_to_property(
getter_setter: GetterSetter,
getter_setter: &GetterSetter,
function: FunctionType,
types: &mut TypeStore,
) -> PropertyValue {
Expand Down Expand Up @@ -210,7 +210,7 @@ pub trait SynthesisableFunction<M: crate::ASTImplementation> {
);
}

/// TODO might be generic if FunctionBehavior becomes generic
/// TODO might be generic if `FunctionBehavior` becomes generic
pub enum FunctionRegisterBehavior<'a, M: crate::ASTImplementation> {
ArrowFunction {
expecting: TypeId,
Expand Down Expand Up @@ -252,6 +252,7 @@ pub enum FunctionRegisterBehavior<'a, M: crate::ASTImplementation> {
pub struct ClassPropertiesToRegister<'a, M: ASTImplementation>(pub Vec<ClassValue<'a, M>>);

impl<'a, M: crate::ASTImplementation> FunctionRegisterBehavior<'a, M> {
#[must_use]
pub fn is_async(&self) -> bool {
match self {
FunctionRegisterBehavior::ArrowFunction { is_async, .. }
Expand All @@ -263,6 +264,7 @@ impl<'a, M: crate::ASTImplementation> FunctionRegisterBehavior<'a, M> {
}
}

#[must_use]
pub fn is_generator(&self) -> bool {
match self {
FunctionRegisterBehavior::ExpressionFunction { is_generator, .. }
Expand Down
3 changes: 2 additions & 1 deletion checker/src/behavior/objects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ impl ObjectBuilder {
value: PropertyValue,
position: Option<SpanWithSource>,
) {
environment.facts.register_property(self.object, publicity, under, value, true, position)
environment.facts.register_property(self.object, publicity, under, value, true, position);
}

#[must_use]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I haven't had much experience with #[must_use]. Is there a heuristic for when to it is needed? Was there any code that didn't use this result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this (old) reddit post by one of clippy maintainer here is how they decide which function should be marked with #[must_use]. Also, a lot of people were complaining about false positive, but it looks like it has been fixed and is working pretty well now.

After adding #[must_use], there was no additional error, so I guess everything was used

pub fn build_object(self) -> TypeId {
self.object
}
Expand Down
Loading
Loading