Useless vec false positive

[ericwu17]

changelog: [useless_vec]: fix false positive in macros.

fixes #11861

We delay the emission of useless_vec lints to the check_crate_post stage, which allows us to effectively undo lints if we find that a vec![] expression is being used multiple times after macro expansion.

[rustbot]

r? @blyxyas

(rustbot has picked a reviewer for you, use r? to override)

[y21]

Is this extra set needed? It seems to me like the map on its own would be enough for determining uses

[ericwu17]

I have removed this set, now there's only the span_to_lint_map.

[y21]

Is the logic here that if all uses of the variable let x = vec![]; adjust to a slice, then this is guaranteed to not be from a macro argument and we can always lint?

That would break down here I think:

macro_rules! m {
  ($x:expr) => {
    fn f() { let _x = $x; $x.starts_with(&[]); }
    fn f2() { let _x: Vec<i32> = $x; }
  }
}
m!(vec![1]);

If I'm reading this correctly, it would lint here.
All uses of _x in f adjust to a slice, but it's still expanded twice.

[ericwu17]

Oh, thanks for the catch. You are correct that the logic is broken here. My intention of adding the spans_sure_to_lint set was that when sometimes we might want to lint for _ in vec![...], but since there the vec![...] does not adjust to a slice, subseqeunt check_expr calls would insert None into the span_to_lint_map.

I think I will change the logic to store a set of HirIds that are ok to lint, since expressions after macro expansion will have distinct HirIds.

[blyxyas]

Meow :3

[blyxyas]

The first check of this method is already searching for VecArgs::hir(cx, expr), is this check necessary again?

[ericwu17]

The first check of this method was specifically looking for cases of let x = vec![...] where every subsequent use of x adjusts to a slice, whereas this check looks for cases where vec![] is used in a context where the expression's type adjusts to a slice.

The original lint was also set up in this manner, but I readjusted the order of the different checks and also changed subsequent checks to use "else if" instead of "if".

[blyxyas]

But I don't understand why you chose to create a new branch checking if let Some(vec_args) = higher::VecArgs::hir(cx, expr.peel_borrows()), instead of adding this logic to the previous that checks the same thing.

[blyxyas]

This if statement means that in cases like that self.span_to_lint_map.insert(span, None); in line 129, it will not lint. In that case, why insert it in the first case?

If not linting is intended, both that line and the whole lintable_exprs field can be removed.

[ericwu17]

The reason to insert None when we don't want to lint is to mark the specific span as non-lintable. There are cases where the same span expands to multiple expressions (when a macro expands the vec!) and we want to lint only when all of the expanded expressions are ok for linting.

The reason for lintable_exprs is because the first two checks of the lint will check cases where we do want to emit the lint even when the vec![...] expression doesn't adjust to a slice. I chose to keep track of those expressions separately, although I could have also added a parameter to check_vec_macro indicating whether we want to lint immediately, or wait until the check_crate_post stage to emit lints.

[blyxyas]

There's no need to gather all lintable expressions in a single collection and then do a crate pass, you can just lint in place.

The infrastructure needed to collect all lintable expressions costs performance and memory, some things that we're trying to improve right now. If you're trying to improve code cleanliness, you can just refactor the span_lint... function into it's own function that takes less arguments. Like a report function or emit. It's done in other lints to avoid typing the same description / notes multiple times.

[ericwu17]

Thanks for the suggestion. I'll look into refactoring this weekend, since I am busy with school this week.

[blyxyas]

Welp ｡＾･ｪ･＾｡

I pulled this PR on my personal computer to check if I could remove that new field, to no avail. I think that the current state of the PR is good to be merged, although I'd love to not have to add a new field / have to mark some spans as unlintable.

@y21 Do you find a way to remove that new field, or do you approve?
I'll approve from my part, let's see if someone comes before this Wednesday that has some ideas.

[blyxyas]

Eric, it seems like it will continue as-is, this is just the best solution we could find. Could you squash these commits into a single one?

[blyxyas]

@bors r+

[bors]

📌 Commit 884bec3 has been approved by blyxyas

It is now in the queue for this repository.

[bors]

⌛ Testing commit 884bec3 with merge c19508b...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing c19508b to master...