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

[useless_vec]: use the source span for initializer #11081

Merged
merged 1 commit into from Jul 3, 2023

Conversation

y21
Copy link
Member

@y21 y21 commented Jul 3, 2023

Fixes #11075.

changelog: [useless_vec]: use the source span for the initializer expression when inside of a macro

@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2023

r? @Manishearth

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 3, 2023
@Alexendoo
Copy link
Member

We could do with some tests for some various other macro combinations, e.g.

macro_rules! in_macro {
    ($e:expr, $vec:expr) => {{
        for _ in vec![1] {}
        for _ in vec![$e] {}
        for _ in $vec {}
    }}
}

m!(1, vec![1]);

macro_rules! from_macro {
    () => { vec![1, 2, 3] }
}

for _ in from_macro!() {}

@y21
Copy link
Member Author

y21 commented Jul 3, 2023

@Alexendoo Regarding the from_macro one, what's the expected behavior? I assume it shouldn't lint, since it's coming from an expansion. It might not be something the user can just easily change inside the macro. And if we don't lint there, wouldn't that also mean that the first one wouldn't be linted either, since that's also from an expansion?

@Manishearth
Copy link
Member

@bors r+

r=me to fix the issue but i agree that we should add more tests

@bors
Copy link
Collaborator

bors commented Jul 3, 2023

📌 Commit 1f77f8c has been approved by Manishearth

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 3, 2023

⌛ Testing commit 1f77f8c with merge a959061...

@Alexendoo
Copy link
Member

for _ in vec![1] {} and for _ in vec![$e] {} could be linted if it's in a local macro (but it's fine if they aren't), the other two shouldn't be

@bors
Copy link
Collaborator

bors commented Jul 3, 2023

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

@bors bors merged commit a959061 into rust-lang:master Jul 3, 2023
5 checks passed
bors added a commit that referenced this pull request Jul 3, 2023
[`useless_vec`]: add more tests and don't lint inside of macros

Closes #11084.

I realized that the fix I added in #11081 itself also causes an error in a suggestion when inside of a macro. Example:
```rs
macro_rules! x {
  () => {
    for _ in vec![1, 2] {}
  }
}
x!();
```
Here it would suggest replacing `vec![1, 2]` with `[x!()]`, because that's what the source callsite is (reminder: it does this to get the correct span of `x!()` for code like `for _ in vec![x!()]`), but that's wrong when *inside* macros, so I decided to make it not lint if the whole loop construct is inside a macro to avoid this issue.

changelog: [`useless_vec`]: add more tests and don't lint inside of macros

r? `@Alexendoo` since these were your tests, I figured it makes most sense to assign you
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.

useless_vec suggestion copies irrelevant tokens from a macro definition
5 participants