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

tokio: remove wildcard in match pattern #5970

Merged
merged 9 commits into from
Sep 23, 2023

Conversation

wathenjiang
Copy link
Contributor

@wathenjiang wathenjiang commented Sep 1, 2023

When we use wildcard * in match pattern, it can lead to potential errors.

enum MyEnum{
    TypeA,
    TypeB,
    TypeC,
}

fn logic(e :MyEnum){
    use MyEnum::*;

    match e {
        TypeA =>  println!("A"),
        TypeB =>  println!("B"),
        TypeC =>  println!("C"),
    }
}

If we delete TypeC,add add TypeD

enum MyEnum{
    TypeA,
    TypeB,
    TypeD,
}

fn logic(e :MyEnum){
    use MyEnum::*;

    match e {
        TypeA =>  println!("A"),
        TypeB =>  println!("B"),
        TypeC =>  println!("C"),
    }
}

The Rust compiler takes TypeC as a named variables, not a variant of the enum, and the unexpected error occurs.

So we should never use such as use MyEnum::*; wildcard in Rust code.

@github-actions github-actions bot added the R-loom-sync Run loom sync tests on this PR label Sep 1, 2023
@Darksonn Darksonn added the A-tokio Area: The main tokio crate label Sep 1, 2023
@Darksonn
Copy link
Contributor

Darksonn commented Sep 1, 2023

That would trigger a warning because TypeC is not snake case, which would fail our build.

@wathenjiang wathenjiang changed the title tokio: remove wildcard in match tokio: remove wildcard in match pattern Sep 1, 2023
@wathenjiang
Copy link
Contributor Author

That would trigger a warning because TypeC is not snake case, which would fail our build.

Yes, the CI is useful for it. But if we do this, we can avoid such error before CI, and it may have better code smell.

Comment on lines 131 to 132
let worker_threads = match (flavor, self.worker_threads) {
(CurrentThread, Some((_, worker_threads_span))) => {
(RuntimeFlavor::CurrentThread, Some((_, worker_threads_span))) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind dropping the import for this case, but I don't want to drop it for Poll, Result, Ok, or Ordering. That's too much noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind dropping the import for this case, but I don't want to drop it for Poll, Result, Ok, or Ordering. That's too much noise.

Some kinds of enum of them, such as Result, Ordering might not be changed in the future, unless the std of rust make some broken changes. I reserve my opinion on these enums. But I believe some enums which is defiend in internal code of tokio might change in the future.

We can use use MyEnum as E to reduce the noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made some changes, allowing the wildcard in Result and Ordering for reduce the noise. And keep the other changes.

Comment on lines 607 to 610
use TryCurrentErrorKind::*;
use TryCurrentErrorKind as E;
match self {
NoContext => f.write_str("NoContext"),
ThreadLocalDestroyed => f.write_str("ThreadLocalDestroyed"),
E::NoContext => f.write_str("NoContext"),
E::ThreadLocalDestroyed => f.write_str("ThreadLocalDestroyed"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why E? Should this not be a K for kind?

I'm also okay with TryCurrentErrorKind:: here.

Copy link
Contributor Author

@wathenjiang wathenjiang Sep 3, 2023

Choose a reason for hiding this comment

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

E means Error, and I may have been influenced by golang :). I believe in Rust the error have a kind, which can refer to: https://doc.rust-lang.org/std/io/enum.ErrorKind.html, and the use of TryCurrentErrorKind :: here is not tedious, so let us just use TryCurrentErrorKind ::.

@wathenjiang
Copy link
Contributor Author

The CI has passed, it looked like the last CI gone wrong.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks.

@Darksonn Darksonn merged commit 707fb4d into tokio-rs:master Sep 23, 2023
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate R-loom-sync Run loom sync tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants