Option to disable trivially_copy_pass_by_ref in public functions

[mati865]

Running Clippy over Rust repo yields about 100 unique warnings from trivially_copy_pass_by_ref, almost all of them were tripped by public functions which are not going to change.

Having option to disable it for public functions would help if this and probably other cases.

EDIT: ptr_arg have similar issue (but there are only 13 occurrences). Maybe something more generic should be developed?

[flip1995]

Just realized, this is kind of complicated without mentoring to do, so here we go:

Case 1:

rust-clippy/clippy_lints/src/trivially_copy_pass_by_ref.rs

Lines 139 to 147 in 7810652

	fn check_fn(
	&mut self,
	cx: &LateContext<'a, 'tcx>,
	kind: FnKind<'tcx>,
	decl: &'tcx FnDecl,
	_body: &'tcx Body,
	span: Span,
	hir_id: HirId,
	) {

with cx.tcx.hir().get(hir_id) the Node of the function in the hir can be accessed. This should be a Node::Item(item), where the visibility can be accessed by item.vis

Case 2:

rust-clippy/clippy_lints/src/trivially_copy_pass_by_ref.rs

Lines 129 to 137 in 7810652

	fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::TraitItem) {
	if item.span.from_expansion() {
	return;
	}
	if let hir::TraitItemKind::Method(method_sig, _) = &item.node {
	self.check_poly_fn(cx, item.hir_id, &*method_sig.decl, None);
	}
	}

Here the visibility is dependent on the trait visibility. A trait in the hir is again an Item. This should be accessible by cx.tcx.hir().get_parent_item(item.hir_id), which results in a HirId, which then should be resolvable with cx.tcx.hir().get(_). This should be a Node::Item(trait). The visibility then again is accessible with trait.vis.

One may also add sanity checks for the ItemKinds of the Items returned by cx.tcx.hir().get(_).

---

After this is implemented, a configuration option needs to be added. There should be enough examples in the code on how to do this.

[flip1995]

If this doesn't exist already, a function visibility(hir_id) would be useful in utils.

[flip1995]

Just realized, that there is a function visibility, which should be used here 😄

[palash25]

Hi @flip1995 new to Rust and I wanted to take a crack at this. I saw your comment explaining what needs to be done but its marked outdated anad collapsed, any reason for that? Is that approach not to be followed anymore?

[flip1995]

Hey @palash25 See the comment (#4504 (comment)) above yours. This isn't necessary, since the rust compiler already provides a function for that.

[palash25]

Oh my bad 😅 got confused.

Just to be sure I understand your comments correctly, the desired approach is to use the visibility method to figure out whether the method is public or not and if it is based on the flag disable or enable the lint, right?

[flip1995]

Yes correct. visibility takes a DefId. In the current implementation you have a HirId, which you can convert with opt_local_def_id.

based on the flag disable or enable the lint

Yes, I'd suggest to enable/disable the visibility check:

if allow_public_flag && is_public() {
    return;
}

[flip1995]

@palash25 I just realized, that this is even easier:

You can just use the function cx.access_levels.is_exported() for visibility.

[palash25]

thanks for your help @flip1995 will try to take a stab at this in a day or two

[palash25]

@flip1995 need a little help with this part allow_public_flag I saw clippy help and it mentions the following flags

    -W --warn OPT       Set lint warnings
    -A --allow OPT      Set lint allowed
    -D --deny OPT       Set lint denied
    -F --forbid OPT     Set lint forbidden

Do I have to somehow reuse allow flag (because it seems lint specific so doesn't make much sense to me) for public functions or implement a new flag for it?

[palash25]

Actually the clippy help text isn't very helpful I am not able to find out what could be the possible values for OPT I tried running something like cargo clippy --allow mut_from_ref but got an error. Is this documented in detail somewhere somewhere, any examples of flag usage?

[mati865]

@palash25 it's explained in the readme: https://github.com/rust-lang/rust-clippy#allowingdenying-lints
TL;DR the syntax is as follows: cargo clippy -- -A clippy::lint_name.

[flip1995]

Also this is not the place to look for this kind of configuration.

See for example this lint, for how to use a configuration:

rust-clippy/clippy_lints/src/functions.rs

Lines 88 to 97 in edd9047

	pub struct Functions {
	threshold: u64,
	max_lines: u64,
	}
	impl Functions {
	pub fn new(threshold: u64, max_lines: u64) -> Self {
	Self { threshold, max_lines }
	}
	}

rust-clippy/clippy_lints/src/lib.rs

Line 538 in 0583181

reg.register_late_lint_pass(box functions::Functions::new(conf.too_many_arguments_threshold, conf.too_many_lines_threshold));

And this for how to add a configuration option:

rust-clippy/clippy_lints/src/utils/conf.rs

Lines 133 to 134 in db233b0

	/// Lint: TOO_MANY_ARGUMENTS. The maximum number of argument a function or method can have
	(too_many_arguments_threshold, "too_many_arguments_threshold", 7 => u64),

To test it, you have to create a ui-toml test, like this one:
https://github.com/rust-lang/rust-clippy/tree/master/tests/ui-toml/toml_trivially_copy