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

Fix two missing_const_for_fn false positives #3844

Merged
merged 2 commits into from Mar 5, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+29 −3
Diff settings

Always

Just for now

@@ -1,8 +1,9 @@
use crate::utils::{is_entrypoint_fn, span_lint};
use if_chain::if_chain;
use rustc::hir;
use rustc::hir::intravisit::FnKind;
use rustc::hir::{Body, Constness, FnDecl, HirId};
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintPass};
use rustc::{declare_tool_lint, lint_array};
use rustc_mir::transform::qualify_min_const_fn::is_min_const_fn;
use syntax_pos::Span;
@@ -82,7 +83,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn {
) {
let def_id = cx.tcx.hir().local_def_id_from_hir_id(hir_id);

if is_entrypoint_fn(cx, def_id) {
if in_external_macro(cx.tcx.sess, span) || is_entrypoint_fn(cx, def_id) {
return;
}

@@ -95,7 +96,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn {
}
},
FnKind::Method(_, sig, ..) => {
if already_const(sig.header) {
if is_trait_method(cx, hir_id) || already_const(sig.header) {
return;
}
},
@@ -114,6 +115,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn {
}
}

fn is_trait_method(cx: &LateContext<'_, '_>, hir_id: HirId) -> bool {
This conversation was marked as resolved by oli-obk

This comment has been minimized.

Copy link
@oli-obk

oli-obk Mar 5, 2019

Collaborator

I think you may be able to use https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/fn.trait_of_item.html instead of this function. There's no docs on the function, so it might have panic cases, but as long as it's only used on methods, it should be fine

This comment has been minimized.

Copy link
@phansch

phansch Mar 5, 2019

Author Collaborator

I tried that and it only reported the traits of the default impl methods, not of normal trait methods like in the example :/

// Get the implemented trait for the current function
let parent_impl = cx.tcx.hir().get_parent_item(hir_id);
if_chain! {
if parent_impl != hir::CRATE_HIR_ID;
if let hir::Node::Item(item) = cx.tcx.hir().get_by_hir_id(parent_impl);
if let hir::ItemKind::Impl(_, _, _, _, Some(_trait_ref), _, _) = &item.node;
then { return true; }
}
false
}

// We don't have to lint on something that's already `const`
fn already_const(header: hir::FnHeader) -> bool {
header.constness == Constness::Const
@@ -55,3 +55,16 @@ trait Foo {
33
}
}

// Don't lint in external macros (derive)
#[derive(PartialEq, Eq)]
struct Point(isize, isize);

impl std::ops::Add for Point {
type Output = Self;

// Don't lint in trait impls of derived methods
fn add(self, other: Self) -> Self {
Point(self.0 + other.0, self.1 + other.1)
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.