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.

Sign up for GitHub

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

Jump to bottom

Add lint for assertions in functions returning Result #6280

Merged
merged 3 commits into from
Dec 7, 2020
Merged

Add lint for assertions in functions returning Result #6280

merged 3 commits into from
Dec 7, 2020

Conversation

dp304
Copy link
Contributor

@dp304 dp304 commented Oct 31, 2020

changelog: none
fixes #6082

@rust-highfive
Copy link

r? @ebroto

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 31, 2020
ebroto
ebroto suggested changes Nov 10, 2020
View reviewed changes
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the lint documentation of panic_in_result_fn:

For some codebases, it is desirable for functions of type result to return an error instead of crashing`

So I think this should not be a separate lint, but an enhancement of panic_in_result_fn, otherwise that lint would miss some panic cases.

@thefallentree do you have any other reason (besides avoiding panicking) for having this implemented separately? In case separate lints are desired, I think we should fix panic_in_result_fn anyway.

@ebroto ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 10, 2020
@dp304 dp304 force-pushed the assert_in_result_fn branch 2 times, most recently from 874bec6 to 684f407 Compare November 11, 2020 03:03
@ebroto ebroto added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Nov 23, 2020
@bors
Copy link
Contributor

bors commented Nov 27, 2020

☔ The latest upstream changes (presumably #6389) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

ebroto
ebroto suggested changes Nov 28, 2020
View reviewed changes
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I wanted to give @thefallentree the opportunity to raise a concern.

Would you mind adding your changes as an enhancement to panic_in_result_fn instead of making it a new lint?

@ebroto ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 28, 2020
@dp304 dp304 force-pushed the assert_in_result_fn branch 2 times, most recently from 1665d88 to 69f83dd Compare November 30, 2020 15:14
@dp304
Copy link
Contributor Author

dp304 commented Nov 30, 2020

Done as requested.
Also rebased to current master, resolved conflicts, squashed & added an unrelated commit to make automated tests work.

ebroto
ebroto suggested changes Dec 3, 2020
View reviewed changes
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, just some nits.

BTW the latest commit is not necessary, it will be removed if you rebase on top of the latest master

@dp304 dp304 force-pushed the assert_in_result_fn branch from 69f69bc to 6544a63 Compare December 3, 2020 22:33
@dp304 dp304 requested a review from ebroto December 4, 2020 21:31
@ebroto
Copy link
Member

ebroto commented Dec 7, 2020

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Dec 7, 2020

📌 Commit 6544a63 has been approved by ebroto

@bors
Copy link
Contributor

bors commented Dec 7, 2020

⌛ Testing commit 6544a63 with merge c5096e6...

bors added a commit that referenced this pull request Dec 7, 2020
@bors
Auto merge of #6280 - dp304:assert_in_result_fn, r=ebroto
c5096e6 
Add lint for assertions in functions returning Result

changelog: none
fixes #6082
@ebroto ebroto removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Dec 7, 2020
@bors
Copy link
Contributor

bors commented Dec 7, 2020

💔 Test failed - checks-action_test

dp304 and others added 3 commits December 8, 2020 00:10
@dp304 @ebroto
panic_in_result_fn: Extend to also check usages of [debug_]assert* ma…
e58c7dd 
…cros

Also, the macro-finding logic has been moved to the util module, for
use by future lints.
@dp304 @ebroto
Apply suggestions from code review
bdad790 
Use array slice instead of `Vec` in `find_macro_calls` as suggested by @ebroto

Co-authored-by: Eduardo Broto <ebroto@tutanota.com>
@ebroto
Update reference file
16d0e56
@ebroto ebroto force-pushed the assert_in_result_fn branch from 6544a63 to 16d0e56 Compare December 7, 2020 23:14
@ebroto
Copy link
Member

ebroto commented Dec 7, 2020

@bors r+

The reference file needed to be updated. The span looks weird with an extra whitespace, but since it comes directly from LateLintPass::check_fn, I guess there's nothing we can do.

@bors
Copy link
Contributor

bors commented Dec 7, 2020

📌 Commit 16d0e56 has been approved by ebroto

@bors
Copy link
Contributor

bors commented Dec 7, 2020

⌛ Testing commit 16d0e56 with merge 856f4f3...

@bors
Copy link
Contributor

bors commented Dec 7, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 856f4f3 to master...

@bors bors merged commit 856f4f3 into rust-lang:master Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebroto ebroto ebroto approved these changes

Assignees

@ebroto ebroto

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

catch assert! assert_eq! assert_ne! and variants in ResultFN
4 participants
@dp304 @rust-highfive @bors @ebroto