Skip to content

Conversation

SignalWalker
Copy link

@SignalWalker SignalWalker commented Oct 9, 2025

Partially addressing #15569, document items in clippy_utils::{consts, higher, macros, paths, res, source, str_utils, sugg, usage, visitors}. The specific items documented are listed in each commit description, along with notes.

This is my first time contributing to any official Rust internals/tools, so please let me know if I misunderstood anything. (I'm not really sure that "refers" is the right verb for the relationship between a Place and a HIR node, for example.)

As I understand it, this is considered purely an internal change, so:
changelog: none

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 9, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2025

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

- `is_potentially_mutated()`
- `is_potentially_local_place()`
- `ParamBindingIdCollector`
- `BindingUsageFinder`
- `contains_return_break_continue_macro()`
- `local_used_in()`
- `local_used_after_expr()`
- `IfLetOrMatch::scrutinee()`
- `While.label`
- `WhileLet.label`
- module
- `MacroCall::is_local()`
- `PanicExpn`
- `PanicExpn::parse()`
- `HirNode::hir_id()`
- `HirNode::span()`
I'm not really sure how to document the `PathLookup` statics,
so I'm skipping those for now.
- module
- `HasHirId::hir_id()`
- `MaybeTypeckRes`
- `MaybeDef::opt_def_id()`
- `HasSession`
- `HasSession::sess()`
- `SpanRange::into_range()`
- `IntoSpan::into_span()`
- `IntoSpan::with_ctxt()`
- `SpanRangeExt`
  - May want to be more specific with this one.
- `SourceFileRange` and its fields
- `is_present_in_source()`
  - This one feels a bit clunky to me; might want to rephrase.
- `snippet_block_with_context()`
- module
- fields of `StrIndex`
- `StrIndex::new()`
- fields of `StrCount`
- `StrCount::new()`

`StrIndex` and `StrCount` don't seem to guarantee that their fields are
internally consistent; should this be documented?
- `Sugg::into_string()`

I'm not sure that this is correct, but it does seem to be how its used
elsewhere.
- module
- `Descend::{Yes, No}`
- `find_all_ret_expressions()`
- `for_each_unconsumed_temporary()`
  - this one was mostly just reformatting the non-doc comment that was
    already there
- `any_temporaries_need_ordered_drop()`
- `contains_break_or_continue()`
@rustbot
Copy link
Collaborator

rustbot commented Oct 12, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@SignalWalker SignalWalker changed the title Document pub items in clippy_utils::usage Document pub items in multiple clippy_utils modules Oct 12, 2025
- `Constant::Adt`
- `Constant::partial_cmp()`
- `Constant::peel_refs()`
- `Constant::new_numeric_min()`
- `Constant::new_numeric_max()`
- `Constant::is_numeric_min()`
- `Constant::is_numeric_max()`
- `Constant::is_pos_infinity()`
- `Constant::is_neg_infinity()`
- `ConstantSource::is_local()`
- `FullInt` and its variants
- `ConstEvalCtxt::eval_pat_expr()`
- `mir_to_const()`
/// A `LitKind`-like enum to fold constant `Expr`s into.
#[derive(Debug, Clone)]
pub enum Constant {
/// A constant representing an algebraic data type
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if this is any ADT or just structs, because mir_to_const() seems only to convert struct ADTs.

}
}

/// Recursively consume [`Constant::Ref`] and return the innermost value.
Copy link
Author

@SignalWalker SignalWalker Oct 12, 2025

Choose a reason for hiding this comment

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

I don't think "Recursively" is technically the right word, but I couldn't think of anything better that would still be succinct.

}

/// An integer type (signed or unsigned) with enough bits to represent any integer that can be
/// represented in Rust.
Copy link
Author

Choose a reason for hiding this comment

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

I'm assuming this is what this is for, based on the name and from skimming over usage.

}
}

/// Attempts to evaluate the given [pattern expression](PatExpr) as a [`Constant`].
Copy link
Author

Choose a reason for hiding this comment

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

Not entirely sure this is accurate?

/// - If `val` is a [`ConstValue::Indirect`] and `ty` is an [`Array`](ty::Array) of
/// [`Float`](ty::Float), returns a [`Constant::Vec`]
///
/// Otherwise, returns `None`.
Copy link
Author

Choose a reason for hiding this comment

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

Is this function actually as particular as described or am I missing something

pub body: &'hir Expr<'hir>,
/// Span of the loop header
pub span: Span,
/// The loop's label, if present
Copy link
Author

Choose a reason for hiding this comment

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

pub let_expr: &'hir Expr<'hir>,
/// `while let` loop body
pub if_then: &'hir Expr<'hir>,
/// The loop's label, if present
Copy link
Author

Choose a reason for hiding this comment

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

matches!(name, sym::assert_macro | sym::debug_assert_macro)
}

/// A `panic!()` expression, which may contain arguments
Copy link
Author

Choose a reason for hiding this comment

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

not sure about this one

@@ -1,3 +1,5 @@
//! Utilities for node resolution.
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if this fully describes this module?


/// Conversion of a value into the range portion of a `Span`.
pub trait SpanRange: Sized {
/// Converts `self` into the range portion of a [`Span`].
Copy link
Author

Choose a reason for hiding this comment

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

not sure about this one

}
}

/// Extensions to [`SpanRange`].
Copy link
Author

Choose a reason for hiding this comment

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

could probably be more descriptive

/// Checks whether the code snippet referred to by the given [`Span`] is present in the source code.
///
/// For example, if the span refers to an attribute inserted during macro expansion, this will
/// return false.
Copy link
Author

Choose a reason for hiding this comment

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

Not sure this is accurate

}

/// Same as [`snippet_block_with_applicability()`], but first walks the span up to the given context
/// using [`snippet_with_context()`].
Copy link
Author

Choose a reason for hiding this comment

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

Should this be more descriptive?

//! Utilities for analyzing and transforming strings.
/// Dealing with string indices can be hard, this struct ensures that both the
/// character and byte index are provided for correct indexing.
Copy link
Author

@SignalWalker SignalWalker Oct 12, 2025

Choose a reason for hiding this comment

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

Should this mention that char_index and byte_index aren't guaranteed to agree with each other?

}

/// Dealing with string comparison can be complicated, this struct ensures that both the
/// character and byte count are provided for correct indexing.
Copy link
Author

Choose a reason for hiding this comment

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

Should this mention that char_count and byte_count aren't guaranteed to agree with each other?

}
}

/// Convert this suggestion into a [`String`] to be displayed to the user.
Copy link
Author

Choose a reason for hiding this comment

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

I'm assuming that this is this function's purpose based on how it's used elsewhere.

@@ -1,3 +1,5 @@
//! Contains utility functions for checking expression usage.
Copy link
Author

Choose a reason for hiding this comment

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

not sure about this one

mutated_variables(expr, cx).is_none_or(|mutated| mutated.contains(&variable))
}

/// Checks whether it is possible that the given [Place] refers to the given local.
Copy link
Author

Choose a reason for hiding this comment

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

"refers to"?

.is_some()
}

/// Checks whether the given expression contains `return`, `break`, `continue`, or a macro call.
Copy link
Author

Choose a reason for hiding this comment

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

Describe why it checks for macro calls?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants