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.

Sign up for GitHub

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

[WIP] [Refactor] Defining clippy rules #91

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
dac0805
refactor(Checker): Moved wrapped arguments inside strucs
JulesGuesnon Nov 27, 2023
55a074e
fix(Checker): Allowing too many arguments
JulesGuesnon Nov 28, 2023
e4996ad
fix(Checker): Allowed too many args in synthesis
JulesGuesnon Nov 28, 2023
683c918
feat(Checker): Added clippy::pedantic
JulesGuesnon Nov 28, 2023
78bd71d
wip(Checker): Applying pedantic rules
JulesGuesnon Nov 28, 2023
555e70b
fix(Checker): All the pedantic now passes
JulesGuesnon Nov 29, 2023
c7ff384
fix(Parser): All the pedantic now passes
JulesGuesnon Nov 29, 2023
2e12e0a
fix(Ezno): All the pedantic now passes
JulesGuesnon Nov 29, 2023
c681084
fix(Parser): Runned formatter
JulesGuesnon Nov 29, 2023
f7bbd1d
fix: Made the updates for wasm
JulesGuesnon Nov 29, 2023
c74f253
fix(Parser): Added the cfg for fields
JulesGuesnon Nov 29, 2023
f2b66f3
refactor(Clippy): Moving rules into workspace
JulesGuesnon Dec 2, 2023
0138ad9
fix(Parser): Allowed pass by value for error
JulesGuesnon Dec 2, 2023
c69fe9a
fix(Parser): Removed reference for error
JulesGuesnon Dec 3, 2023
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
1 change: 1 addition & 0 deletions checker/src/behavior/assignments.rs
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
28 changes: 14 additions & 14 deletions checker/src/behavior/operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ pub fn evaluate_pure_binary_operation_handle_errors<T: crate::ReadFromFS, M: AST
PureBinaryOperation::EqualityAndInequality(operator) => {
evaluate_equality_inequality_operation(
lhs,
operator,
&operator,
rhs,
&mut checking_data.types,
checking_data.options.strict_casts,
Expand All @@ -82,7 +82,7 @@ pub fn evaluate_mathematical_operation(
) -> Result<TypeId, ()> {
fn attempt_constant_math_operator(
lhs: TypeId,
operator: MathematicalAndBitwise,
operator: &MathematicalAndBitwise,
rhs: TypeId,
types: &mut TypeStore,
strict_casts: bool,
Expand Down Expand Up @@ -153,7 +153,7 @@ pub fn evaluate_mathematical_operation(
return Ok(types.register_type(crate::Type::Constructor(constructor)));
}

attempt_constant_math_operator(lhs, operator, rhs, types, strict_casts)
attempt_constant_math_operator(lhs, &operator, rhs, types, strict_casts)
}

/// Not canonical / reducible
Expand All @@ -178,7 +178,7 @@ pub enum CanonicalEqualityAndInequality {

pub fn evaluate_equality_inequality_operation(
lhs: TypeId,
operator: EqualityAndInequality,
operator: &EqualityAndInequality,
rhs: TypeId,
types: &mut TypeStore,
strict_casts: bool,
Expand All @@ -202,7 +202,7 @@ pub fn evaluate_equality_inequality_operation(

match attempt_constant_equality(lhs, rhs, types) {
Ok(ty) => Ok(ty),
Err(_) => {
Err(()) => {
unreachable!("should have been caught by above")
}
}
Expand Down Expand Up @@ -248,7 +248,7 @@ pub fn evaluate_equality_inequality_operation(
EqualityAndInequality::StrictNotEqual => {
let equality_result = evaluate_equality_inequality_operation(
rhs,
EqualityAndInequality::StrictEqual,
&EqualityAndInequality::StrictEqual,
lhs,
types,
strict_casts,
Expand All @@ -267,7 +267,7 @@ pub fn evaluate_equality_inequality_operation(
EqualityAndInequality::NotEqual => {
let equality_result = evaluate_equality_inequality_operation(
rhs,
EqualityAndInequality::Equal,
&EqualityAndInequality::Equal,
lhs,
types,
strict_casts,
Expand All @@ -281,15 +281,15 @@ pub fn evaluate_equality_inequality_operation(
}
EqualityAndInequality::GreaterThan => evaluate_equality_inequality_operation(
rhs,
EqualityAndInequality::LessThan,
&EqualityAndInequality::LessThan,
lhs,
types,
strict_casts,
),
EqualityAndInequality::LessThanEqual => {
let lhs = evaluate_equality_inequality_operation(
rhs,
EqualityAndInequality::StrictEqual,
&EqualityAndInequality::StrictEqual,
lhs,
types,
strict_casts,
Expand All @@ -300,15 +300,15 @@ pub fn evaluate_equality_inequality_operation(
} else if lhs == TypeId::FALSE {
evaluate_equality_inequality_operation(
rhs,
EqualityAndInequality::LessThan,
&EqualityAndInequality::LessThan,
lhs,
types,
strict_casts,
)
} else {
let rhs = evaluate_equality_inequality_operation(
rhs,
EqualityAndInequality::LessThan,
&EqualityAndInequality::LessThan,
lhs,
types,
strict_casts,
Expand All @@ -318,7 +318,7 @@ pub fn evaluate_equality_inequality_operation(
}
EqualityAndInequality::GreaterThanEqual => evaluate_equality_inequality_operation(
rhs,
EqualityAndInequality::LessThanEqual,
&EqualityAndInequality::LessThanEqual,
lhs,
types,
strict_casts,
Expand All @@ -339,7 +339,7 @@ pub fn evaluate_logical_operation_with_expression<
M: crate::ASTImplementation,
>(
lhs: TypeId,
operator: Logical,
operator: &Logical,
rhs: &M::Expression,
checking_data: &mut CheckingData<T, M>,
environment: &mut Environment,
Expand Down Expand Up @@ -399,7 +399,7 @@ pub fn evaluate_logical_operation_with_expression<
Logical::NullCoalescing => {
let is_lhs_null = evaluate_equality_inequality_operation(
lhs,
EqualityAndInequality::StrictEqual,
&EqualityAndInequality::StrictEqual,
TypeId::NULL_TYPE,
&mut checking_data.types,
checking_data.options.strict_casts,
Expand Down
32 changes: 17 additions & 15 deletions checker/src/behavior/template_literal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use source_map::{Span, SpanWithSource};

use crate::{
behavior::objects::ObjectBuilder,
types::{cast_as_string, SynthesisedArgument},
types::{calling::CallingInput, cast_as_string, SynthesisedArgument},
CheckingData, Constant, Environment, Instance, Type, TypeId,
};

Expand All @@ -26,13 +26,13 @@ where
M::Expression: 'a,
{
fn part_to_type<T: crate::ReadFromFS, M: crate::ASTImplementation>(
first: TemplateLiteralPart<M::Expression>,
first: &TemplateLiteralPart<M::Expression>,
environment: &mut Environment,
checking_data: &mut CheckingData<T, M>,
) -> crate::TypeId {
match first {
TemplateLiteralPart::Static(static_part) => {
checking_data.types.new_constant_type(Constant::String(static_part.to_owned()))
checking_data.types.new_constant_type(Constant::String((*static_part).to_owned()))
}
TemplateLiteralPart::Dynamic(expression) => {
// TODO tidy
Expand Down Expand Up @@ -67,7 +67,7 @@ where
for part in parts_iter {
match part {
p @ TemplateLiteralPart::Static(_) => {
let value = part_to_type(p, environment, checking_data);
let value = part_to_type(&p, environment, checking_data);
static_parts.append(
environment,
crate::context::facts::Publicity::Public,
Expand All @@ -79,12 +79,12 @@ where
static_part_count += 1;
}
p @ TemplateLiteralPart::Dynamic(_) => {
let ty = part_to_type(p, environment, checking_data);
let ty = part_to_type(&p, environment, checking_data);
arguments.push(SynthesisedArgument::NonSpread {
ty,
// TODO position
position: SpanWithSource::NULL_SPAN,
})
});
}
}
}
Expand All @@ -101,21 +101,23 @@ where
let call_site = position.clone().with_source(environment.get_source());
crate::types::calling::call_type_handle_errors(
tag,
crate::types::calling::CalledWithNew::None,
crate::behavior::functions::ThisValue::UseParent,
None,
arguments,
call_site,
CallingInput {
called_with_new: crate::types::calling::CalledWithNew::None,
this_value: crate::behavior::functions::ThisValue::UseParent,
call_site,
call_site_type_arguments: None,
},
environment,
arguments,
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder whether arguments could be added to the CallingInput? CallingInput already encompasses the sort of hidden arguments that a function call takes, I wonder whether actual values could also be passed here. Or is there a technical reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a reason I didn't add it, it's because fn call in checker/types/callings takes arguments: &[SynthesisedArgument].
So if we want to add arguments in CallingInput, it'll mean that we need another struct with arguments only for this function. Both are fine I think, do you have a preference?

checking_data,
)
.0
} else {
// Bit weird but makes Rust happy
if let Some(first) = parts_iter.next() {
let mut acc = part_to_type(first, environment, checking_data);
let mut acc = part_to_type(&first, environment, checking_data);
for rest in parts_iter {
let other = part_to_type(rest, environment, checking_data);
let other = part_to_type(&rest, environment, checking_data);
let result = super::operations::evaluate_mathematical_operation(
acc,
crate::behavior::operations::MathematicalAndBitwise::Add,
Expand All @@ -125,14 +127,14 @@ where
);
match result {
Ok(result) => acc = result,
Err(_) => {
Err(()) => {
crate::utils::notify!("Invalid template literal concatenation");
}
}
}
acc
} else {
checking_data.types.new_constant_type(Constant::String("".into()))
checking_data.types.new_constant_type(Constant::String(String::new()))
}
}
}
2 changes: 1 addition & 1 deletion checker/src/context/bases.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ impl Bases {

pub(crate) fn merge(&mut self, mut bases: Bases, context_id: ContextId) {
self.immutable_bases.extend(bases.immutable_bases);
for (ty, (ctx_ceil, base)) in bases.mutable_bases.into_iter() {
for (ty, (ctx_ceil, base)) in bases.mutable_bases {
let existing = if ctx_ceil.0 == context_id {
self.immutable_bases.insert(ty, base).is_some()
} else {
Expand Down
Loading
Loading