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

Add empty closure lint #5355

Closed
wants to merge 1 commit into from
Closed

Conversation

ThibsG
Copy link
Contributor

@ThibsG ThibsG commented Mar 22, 2020

This PR adds a lint that triggers spawned empty closures.

Fixes: #625

changelog: Add empty closure lint

@ThibsG ThibsG changed the title Add empty closure lint WIP: Add empty closure lint Mar 23, 2020
@bors
Copy link
Contributor

bors commented Mar 23, 2020

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

@ThibsG ThibsG changed the title WIP: Add empty closure lint Add empty closure lint Mar 27, 2020
@bors
Copy link
Contributor

bors commented Mar 30, 2020

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

@Manishearth
Copy link
Member

I'm not 100% convinced this is a necessary lint, do people ever write this?

/// ```rust
/// std::thread::spawn(|| {});
/// ```
pub EMPTY_CLOSURE,
Copy link
Member

Choose a reason for hiding this comment

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

the name should probably be spawn_empty_closure

/// ```
pub EMPTY_CLOSURE,
style,
"closure with empty body"
Copy link
Member

Choose a reason for hiding this comment

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

this should probably mention spawning

@ThibsG
Copy link
Contributor Author

ThibsG commented Mar 30, 2020

I'm not 100% convinced this is a necessary lint, do people ever write this?

Me neither. I guess people don't (or they don't read their code after refactoring).

Anyway if people are not convinced, we should just close the issue and this PR, it's totally fine.

@Manishearth
Copy link
Member

Sounds good. Let me know if you want to find something else to work on. We do have a couple issues like this where we filed stuff but weren't yet sure if we wanted them, sorry about that.

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.

lint: spawn(|| ()) (spawn empty)
4 participants