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

Remove allow(pedantic) from formatter #6549

Merged
merged 1 commit into from
Aug 14, 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
2 changes: 1 addition & 1 deletion crates/ruff/src/checkers/ast/analyze/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub(crate) fn bindings(checker: &mut Checker) {
return;
}

for binding in checker.semantic.bindings.iter() {
for binding in &*checker.semantic.bindings {
if checker.enabled(Rule::UnusedVariable) {
if binding.kind.is_bound_exception()
&& !binding.is_used()
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff/src/line_width.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl Eq for LineWidth {}

impl PartialOrd for LineWidth {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
self.width.partial_cmp(&other.width)
Some(self.cmp(other))
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/ruff/src/rules/pep8_naming/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ impl TryFrom<Options> for Settings {
.ignore_names
.unwrap_or_else(default_ignore_names)
.into_iter()
.chain(options.extend_ignore_names.unwrap_or_default().into_iter())
.chain(options.extend_ignore_names.unwrap_or_default())
.map(|name| IdentifierPattern::new(&name).map_err(SettingsError::InvalidIgnoreName))
.collect::<Result<Vec<_>, Self::Error>>()?,
classmethod_decorators: options.classmethod_decorators.unwrap_or_default(),
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff/src/rules/pyupgrade/rules/format_literals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ static FORMAT_SPECIFIER: Lazy<Regex> =
Lazy::new(|| Regex::new(r"\{(?P<int>\d+)(?P<fmt>.*?)}").unwrap());

/// Remove the explicit positional indices from a format string.
fn remove_specifiers<'a>(value: &mut Expression<'a>, arena: &'a mut typed_arena::Arena<String>) {
fn remove_specifiers<'a>(value: &mut Expression<'a>, arena: &'a typed_arena::Arena<String>) {
match value {
Expression::SimpleString(expr) => {
expr.value = arena.alloc(
Expand Down Expand Up @@ -217,8 +217,8 @@ fn generate_call(

// Fix the string itself.
let item = match_attribute(&mut call.func)?;
let mut arena = typed_arena::Arena::new();
remove_specifiers(&mut item.value, &mut arena);
let arena = typed_arena::Arena::new();
remove_specifiers(&mut item.value, &arena);

// Remove the parentheses (first and last characters).
let mut output = expression.codegen_stylist(stylist);
Expand Down
14 changes: 10 additions & 4 deletions crates/ruff_formatter/src/arguments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use crate::FormatResult;
use std::ffi::c_void;
use std::marker::PhantomData;

/// Mono-morphed type to format an object. Used by the [crate::format!], [crate::format_args!], and
/// [crate::write!] macros.
/// Mono-morphed type to format an object. Used by the [`crate::format`!], [`crate::format_args`!], and
/// [`crate::write`!] macros.
///
/// This struct is similar to a dynamic dispatch (using `dyn Format`) because it stores a pointer to the value.
/// However, it doesn't store the pointer to `dyn Format`'s vtable, instead it statically resolves the function
Expand Down Expand Up @@ -33,23 +33,26 @@ impl<'fmt, Context> Argument<'fmt, Context> {
#[doc(hidden)]
#[inline]
pub fn new<F: Format<Context>>(value: &'fmt F) -> Self {
#[allow(clippy::inline_always)]
#[inline(always)]
fn formatter<F: Format<Context>, Context>(
ptr: *const c_void,
fmt: &mut Formatter<Context>,
) -> FormatResult<()> {
// SAFETY: Safe because the 'fmt lifetime is captured by the 'lifetime' field.
F::fmt(unsafe { &*(ptr as *const F) }, fmt)
#[allow(unsafe_code)]
F::fmt(unsafe { &*ptr.cast::<F>() }, fmt)
}

Self {
value: value as *const F as *const c_void,
value: (value as *const F).cast::<std::ffi::c_void>(),
lifetime: PhantomData,
formatter: formatter::<F, Context>,
}
}

/// Formats the value stored by this argument using the given formatter.
#[allow(clippy::inline_always)]
Copy link
Member

Choose a reason for hiding this comment

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

The docs says (https://rust-lang.github.io/rust-clippy/v0.0.212/index.html#inline_always):

While there are valid uses of this annotation (and once you know when to use it, by all means allow this lint), it’s a common newbie-mistake to pepper one’s code with it. As a rule of thumb, before slapping #[inline(always)] on a function, measure if that additional function call really affects your runtime profile sufficiently to make up for the increase in compile time.

Do we have measurements that #[inline(always)] is better than #[inline]?

#[inline(always)]
pub(super) fn format(&self, f: &mut Formatter<Context>) -> FormatResult<()> {
(self.formatter)(self.value, f)
Expand Down Expand Up @@ -79,6 +82,7 @@ impl<'fmt, Context> Argument<'fmt, Context> {
pub struct Arguments<'fmt, Context>(pub &'fmt [Argument<'fmt, Context>]);

impl<'fmt, Context> Arguments<'fmt, Context> {
#[allow(clippy::inline_always)]
#[doc(hidden)]
#[inline(always)]
pub fn new(arguments: &'fmt [Argument<'fmt, Context>]) -> Self {
Expand All @@ -87,6 +91,7 @@ impl<'fmt, Context> Arguments<'fmt, Context> {

/// Returns the arguments
#[inline]
#[allow(clippy::trivially_copy_pass_by_ref)] // Bug in Clippy? Sizeof Arguments is 16
pub(super) fn items(&self) -> &'fmt [Argument<'fmt, Context>] {
self.0
}
Expand All @@ -101,6 +106,7 @@ impl<Context> Clone for Arguments<'_, Context> {
}

impl<Context> Format<Context> for Arguments<'_, Context> {
#[allow(clippy::inline_always)]
#[inline(always)]
fn fmt(&self, formatter: &mut Formatter<Context>) -> FormatResult<()> {
formatter.write_fmt(*self)
Expand Down
115 changes: 57 additions & 58 deletions crates/ruff_formatter/src/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ use std::any::{Any, TypeId};
use std::fmt::Debug;
use std::ops::{Deref, DerefMut};

/// A trait for writing or formatting into [FormatElement]-accepting buffers or streams.
/// A trait for writing or formatting into [`FormatElement`]-accepting buffers or streams.
pub trait Buffer {
/// The context used during formatting
type Context;

/// Writes a [crate::FormatElement] into this buffer, returning whether the write succeeded.
/// Writes a [`crate::FormatElement`] into this buffer, returning whether the write succeeded.
///
/// # Errors
/// This function will return an instance of [crate::FormatError] on error.
/// This function will return an instance of [`crate::FormatError`] on error.
///
/// # Examples
///
Expand Down Expand Up @@ -122,7 +122,7 @@ impl BufferSnapshot {
Err(err) => {
panic!(
"Tried to unwrap snapshot of type {:?} as {:?}",
err.type_id(),
(*err).type_id(),
Copy link
Member

Choose a reason for hiding this comment

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

Do you know which rule triggered here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was calling type_id on Box<dyn >

TypeId::of::<T>()
)
}
Expand Down Expand Up @@ -160,7 +160,7 @@ impl<W: Buffer<Context = Context> + ?Sized, Context> Buffer for &mut W {
}

fn restore_snapshot(&mut self, snapshot: BufferSnapshot) {
(**self).restore_snapshot(snapshot)
(**self).restore_snapshot(snapshot);
}
}

Expand Down Expand Up @@ -426,7 +426,7 @@ where
}

fn restore_snapshot(&mut self, snapshot: BufferSnapshot) {
self.inner.restore_snapshot(snapshot)
self.inner.restore_snapshot(snapshot);
}
}

Expand Down Expand Up @@ -508,59 +508,58 @@ fn clean_interned(
interned: &Interned,
interned_cache: &mut FxHashMap<Interned, Interned>,
) -> Interned {
match interned_cache.get(interned) {
Some(cleaned) => cleaned.clone(),
None => {
// Find the first soft line break element or interned element that must be changed
let result = interned
.iter()
.enumerate()
.find_map(|(index, element)| match element {
FormatElement::Line(LineMode::Soft | LineMode::SoftOrSpace) => {
let mut cleaned = Vec::new();
cleaned.extend_from_slice(&interned[..index]);
Some((cleaned, &interned[index..]))
}
FormatElement::Interned(inner) => {
let cleaned_inner = clean_interned(inner, interned_cache);

if &cleaned_inner != inner {
let mut cleaned = Vec::with_capacity(interned.len());
cleaned.extend_from_slice(&interned[..index]);
cleaned.push(FormatElement::Interned(cleaned_inner));
Some((cleaned, &interned[index + 1..]))
} else {
None
}
}
if let Some(cleaned) = interned_cache.get(interned) {
cleaned.clone()
} else {
// Find the first soft line break element or interned element that must be changed
let result = interned
.iter()
.enumerate()
.find_map(|(index, element)| match element {
FormatElement::Line(LineMode::Soft | LineMode::SoftOrSpace) => {
let mut cleaned = Vec::new();
cleaned.extend_from_slice(&interned[..index]);
Some((cleaned, &interned[index..]))
}
FormatElement::Interned(inner) => {
let cleaned_inner = clean_interned(inner, interned_cache);

_ => None,
});

let result = match result {
// Copy the whole interned buffer so that becomes possible to change the necessary elements.
Some((mut cleaned, rest)) => {
for element in rest {
let element = match element {
FormatElement::Line(LineMode::Soft) => continue,
FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space,
FormatElement::Interned(interned) => {
FormatElement::Interned(clean_interned(interned, interned_cache))
}
element => element.clone(),
};
cleaned.push(element)
if &cleaned_inner == inner {
None
} else {
let mut cleaned = Vec::with_capacity(interned.len());
cleaned.extend_from_slice(&interned[..index]);
cleaned.push(FormatElement::Interned(cleaned_inner));
Some((cleaned, &interned[index + 1..]))
}
}

Interned::new(cleaned)
_ => None,
});

let result = match result {
// Copy the whole interned buffer so that becomes possible to change the necessary elements.
Some((mut cleaned, rest)) => {
for element in rest {
let element = match element {
FormatElement::Line(LineMode::Soft) => continue,
FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space,
FormatElement::Interned(interned) => {
FormatElement::Interned(clean_interned(interned, interned_cache))
}
element => element.clone(),
};
cleaned.push(element);
}
// No change necessary, return existing interned element
None => interned.clone(),
};

interned_cache.insert(interned.clone(), result.clone());
result
}
Interned::new(cleaned)
}
// No change necessary, return existing interned element
None => interned.clone(),
};

interned_cache.insert(interned.clone(), result.clone());
result
}
}

Expand Down Expand Up @@ -597,7 +596,7 @@ impl<Context> Buffer for RemoveSoftLinesBuffer<'_, Context> {
}

fn restore_snapshot(&mut self, snapshot: BufferSnapshot) {
self.inner.restore_snapshot(snapshot)
self.inner.restore_snapshot(snapshot);
}
}

Expand Down Expand Up @@ -658,7 +657,7 @@ pub trait BufferExtensions: Buffer + Sized {
where
I: IntoIterator<Item = FormatElement>,
{
for element in elements.into_iter() {
for element in elements {
self.write_element(element)?;
}

Expand All @@ -685,12 +684,12 @@ where
}
}

#[inline(always)]
#[inline]
pub fn write_fmt(&mut self, arguments: Arguments<B::Context>) -> FormatResult<()> {
self.buffer.write_fmt(arguments)
}

#[inline(always)]
#[inline]
pub fn write_element(&mut self, element: FormatElement) -> FormatResult<()> {
self.buffer.write_element(element)
}
Expand Down
Loading