check against alternative method syntax#253
Merged
BD103 merged 20 commits intoTheBevyFlock:mainfrom Feb 7, 2025
Merged
Conversation
Member
|
Thanks for starting this! Since many lints suffer from this issue, I adapted your code into a utility that we can use across the entire project: Show Code (It's pretty long c:)use rustc_hir::{
def::{DefKind, Res},
Expr, ExprKind, Path, PathSegment, QPath,
};
use rustc_lint::LateContext;
use rustc_span::{Span, Symbol};
/// An abstraction over method calls that supports both `receiver.method(args)` and
/// `Struct::method(&receiver, args)`.
///
/// # Examples
///
/// ```ignore
/// fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
/// // Don't use this, it doesn't match qualified method calls!
/// if let ExprKind::MethodCall(_, _, _, span) = expr.kind {
/// // ...
/// }
///
/// // Instead use:
/// if let Some(MethodCall { span, .. }) = MethodCall::try_from(cx, expr) {
/// // ...
/// }
/// }
/// ```
///
/// # Limitations
///
/// This does not support qualified method calls where the function is not a path. For example:
///
/// ```
/// struct Foo;
///
/// impl Foo {
/// fn bar(&self) {}
/// }
///
/// // A closure that returns a function.
/// let closure_closure = || Foo::bar;
///
/// // This will not be matched, because `closure_closure()` is an `ExprKind::Call` instead of an
/// // `ExprKind::Path`.
/// (closure_closure())(&Foo);
///
/// // This *will* be matched, though, because `Foo::bar` is an `ExprKind::Path`.
/// Foo::bar(&Foo);
/// ```
///
/// Furthermore, this does not support [language items]. If [`Self::try_from()`] encounters a
/// qualified method call that is a lang item, it will still return [`None`].
///
/// [language items]: https://rustc-dev-guide.rust-lang.org/lang-items.html
#[derive(Debug)]
pub struct MethodCall<'tcx> {
/// The path to the method.
///
/// This can be used to find the name of the method, its [`DefId`](rustc_hir::def_id::DefId),
/// and its generic arguments.
///
/// # Example
///
/// ```ignore
/// receiver.method(args);
/// // ^^^^^^
///
/// Struct::method(&receiver, args);
/// // ^^^^^^
/// ```
pub method_path: &'tcx PathSegment<'tcx>,
/// The receiver, or the object, of the method.
///
/// This can be used to find what type the method is implemented for.
///
/// TODO(BD103): Does this include the `&` reference? Should we suggest stripping it?
///
/// # Example
///
/// ```ignore
/// receiver.method(args);
/// //^^^^^^
///
/// Struct::method(&receiver, args);
/// // ^^^^^^^^^
/// ```
pub receiver: &'tcx Expr<'tcx>,
/// The arguments passed to the method.
///
/// # Example
///
/// ```ignore
/// receiver.method(args);
/// // ^^^^
///
/// Struct::method(&receiver, args);
/// // ^^^^
/// ```
pub args: &'tcx [Expr<'tcx>],
/// The span of the method and its arguments.
///
/// This will not include the receiver if this is not a qualified method call.
///
/// # Example
///
/// ```ignore
/// receiver.method(args);
/// // ^^^^^^^^^^^^
///
/// Struct::method(&receiver, args);
/// //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/// ```
pub span: Span,
}
impl<'tcx> MethodCall<'tcx> {
/// Tries to return a [`MethodCall`] from an [`Expr`].
///
/// Please see the [structure documentation](MethodCall) for examples and limitations.
pub fn try_from(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<Self> {
match expr.kind {
ExprKind::MethodCall(method_path, receiver, args, span) => Some(Self {
method_path,
receiver,
args,
span,
}),
ExprKind::Call(
// We only want function calls where the function is a path, so we can use
// `LateContext::qpath_res()`. This elimantes code where the function is the result
// of another function, such as:
//
// ```
// let closure_closure = || || {};
//
// // This entire expression will not be matched, though the inner
// // `closure_closure()` will because `closure_closure` is a path.
// (closure_closure())();
// ```
path @ Expr {
kind: ExprKind::Path(qpath),
..
},
args,
) => {
// Resolve the path, filtering out any paths that are not to associated functions.
// This eliminates prevents us from matching standalone functions, such as:
//
// ```
// fn function() {}
//
// // This will not be matched, since `function()`'s definition is not within an
// `impl` block.
// function();
// ```
if let Res::Def(DefKind::AssocFn, def_id) = cx.qpath_res(qpath, path.hir_id) {
// Retrieve the identifiers for all the arguments to this function.
let inputs = cx.tcx.fn_arg_names(def_id);
// If the name of the first argument is `self`, then it *must* be a method.
// `self` is a reserved keyword, and cannot be used as a general function
// argument name.
if inputs
.first()
.is_some_and(|ident| ident.name == Symbol::intern("self"))
{
let method_path = match *qpath {
// If the qualified path is resolved, the method path must be the final
// segment.
QPath::Resolved(
_,
Path {
// Match the final segment as the method path.
segments: [.., method_path],
..
},
) => method_path,
QPath::Resolved(_, path @ Path { segments: [], .. }) => unreachable!(
"found a function call path with no segments at {:?}",
path.span
),
QPath::TypeRelative(_, method_path) => method_path,
// Lang items are not supported.
QPath::LangItem(_, _) => return None,
};
// Match the first argument as `receiver`, then group the rest into the
// slice `args`.
let [receiver, args @ ..] = args else {
// This can only happen if `args == &[]`, which shouldn't be possible,
// since we previously ensured that the the first element to `args`
// existed and was `self`.
unreachable!("arguments to function call was empty, even though `self` was expected, at {:?}", expr.span);
};
return Some(Self {
method_path,
receiver,
args,
span: expr.span,
});
}
}
// If any of the above checks fail, return `None`, as it's not a qualified method
// call.
None
}
_ => None,
}
}
}With that, could you migrate all of our code to use it? The doc-comments I wrote should help! (If you don't have time, I can do this as well.) File that I used to test the above code, you can ignore this :)trait Trait {
fn trait_function();
fn trait_method(&self) {
Self::trait_function();
}
}
struct Struct {
_field: usize,
}
impl Struct {
fn inherent_function() {}
fn inherent_method(&self) {}
}
impl Trait for Struct {
fn trait_function() {}
fn trait_method(&self) {}
}
fn normal_function() {}
fn main() {
let closure = || {};
let closure_closure = || || {};
let struct_ = Struct { _field: 0 };
struct_.inherent_method();
struct_.trait_method();
// Type relative path, assosc-fn def, def assoc-fn res
Struct::inherent_function();
// Type relative path, assoc-fn def, def assoc-fn res
Struct::inherent_method(&struct_);
// Assoc-fn def
Struct::trait_function();
// Type relative path, assoc-fn def, def assoc-fn res
Struct::trait_method(&struct_);
// Assoc-fn def
<Struct as Trait>::trait_function();
// Resolved path, no def, def assoc-fn res
Trait::trait_method(&struct_);
// Resolved path, no def, def fn res
normal_function();
// Resolved path, no def, local res
(closure)();
// Call kind, assoc-fn def, error
(closure_closure)()();
let x = || Struct::inherent_method;
(x())(&struct_);
} |
Collaborator
Author
O neat! Will do, thanks for the help! |
panicking_query_methods
DaAlbrecht
commented
Feb 2, 2025
BD103
reviewed
Feb 4, 2025
Member
BD103
left a comment
There was a problem hiding this comment.
Looks good! I have some nits and a few requested changes, but nothing too difficult to fix :)
Updates to `openssl` v0.10.70, which fixes a use-after-free. https://github.com/TheBevyFlock/bevy_cli/security/dependabot/3
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
DaAlbrecht
commented
Feb 4, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #94
panicking_methodsmain_return_without_appexitinsert_event_resourceinsert_unit_bundle