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 some #[expect] lint interaction #8976

Merged

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Jun 9, 2022

Fixing the first few lints that aren't caught by #[expect]. The root cause of these examples was, that the lint was emitted at the wrong location.


changelog: none

r? @Jarcho

cc: rust-lang/rust#97660

@xFrednet
Copy link
Member Author

xFrednet commented Jun 9, 2022

Hey @Jarcho, could you review this. You can r+ this directly if you agree with the changes 🙃

@bors delegate=Jarcho

@bors
Copy link
Contributor

bors commented Jun 9, 2022

✌️ @Jarcho can now approve this pull request

@Jarcho
Copy link
Contributor

Jarcho commented Jun 9, 2022

Taking a second look at async_yields_async I don't think anything needs to change. It's basically linting the signature of the body. I originally thought it might have an issue because it did something with the span of the final expression.

@Jarcho
Copy link
Contributor

Jarcho commented Jun 9, 2022

Everything else LGTM.

@xFrednet
Copy link
Member Author

xFrednet commented Jun 9, 2022

Well, while async_yields_async lints on the entire body, for the user it would be nice, if expect/allow also works on the statement that's shown in the lint message. Every problem you found with the expectation not working was also affecting other lint attributes, from what I can tell 🙃. Btw, thank you very much for checking these lints, that has been super valuable!

So far, I haven't got any conflicts (Made these changes last weekend or so). Should we merge it as it is, or should I add the other lint changes to this PR as well? I'm fine either way :)

@Jarcho
Copy link
Contributor

Jarcho commented Jun 9, 2022

I question whether that's a good span to lint on in general, but that can be a different discussion.

May as well merge now. Waiting just increases the chances of conflicts coming up. @bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2022

📌 Commit 9d201d6 has been approved by Jarcho

@bors
Copy link
Contributor

bors commented Jun 9, 2022

⌛ Testing commit 9d201d6 with merge b3c94c0...

@bors
Copy link
Contributor

bors commented Jun 9, 2022

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

@bors bors merged commit b3c94c0 into rust-lang:master Jun 9, 2022
@xFrednet xFrednet deleted the rust-97660-catch-emissions-with-expect branch June 23, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants