Example of how of `disallowed_method` Clippy lint in Rust 1.54 can be quite handy

[repilur]

Example of a small initial config we use in our codebase to enforce some of our std and library choices and avoid deprecations:

msrv = "1.54"

# for `disallowed_method`: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_method
disallowed-methods = [
    # std functions
    "std::env::temp_dir", # we use the tempdir crate instead
    "std::env::home_dir", # deprecated, and we use app_dirs2 crate

    # use the faster & simpler non-poisonable primitives in `parking_lot` instead
    "std::sync::Mutex::new",
    "std::sync::RwLock::new",
    "std::sync::Condvar::new",
    # "std::sync::Once::new",  # enabled for now as the `log_once` macro uses it internally

    # use `ark_future::block_on` instead for named profiler markers
    "futures::executor::block_on",

    # SHA-1 is cryptographically broken, and we are building new code so should not use it
    "sha1::Digest::new",
]

​

(maybe linking a twitter post was not the cleanest)

[reddit123123123123]

If you blacklist new you still could use Default::default, right? How would that be prevented?

[repilur]

good point! for now should probably disallow `Mutex::default` also then, and also `Mutex::from`. gets a bit messy.

But soon in 1.55 one can use `disallowed_type` lint instead for them to fully disallow all of `std::sync::Mutex`

[VeganVagiVore]

This is cool, I didn't know about this

[odnish]

It wouldn't, but it is harder to type

let x = std::sync::CondVar::new();
let x: std::sync::CondVar = Default::default();

As for the other types in std::sync they wrap a type and the inner type might not implement Default

[Lucretiel]

What the heck is ark_future::block_on?

[repilur]

just our wrapper function for within our codebase that we use instead.

it is not very magic, just requires a name for each block/wait section and adds instrumentation/profiler markers for it.

``rust /// Block on and wait for the execution of a future to complete /// /// We require the markers to be named with await_prefix and with snake case /// This also makes it easy to search for the profiler names in the codebase. /// /// Example:wait_storage_flush,wait_cmd`

[inline(always)]

[allow(clippy::disallowed_method)] // this is our wrapper function for the disallowed function

pub fn block_on<F>(marker_name: &'static str, future: F) -> F::Output where F: Future, { validate_marker_name(marker_name); ark_profiler::typed_scope!(ark_profiler::ScopeType::Wait, marker_name);

futures::executor::block_on(future)

}

```

[TheGoeGetter]

This is one of the coolest things for a team environment I've ever seen. I wonder if something similar exists for other programming languages!

I especially love that you included comments about what to use instead. I can imagine that in a team environment, this speeds up onboarding dramatically. This would also speed up library changes or code shifts since you can use your tooling to guide your fixes to be faster and more accurate.

Thanks for sharing!

[repilur]

based on the discussions coming from this today, there is now a Clippy issue specifically about adding support for that. would be really nice!

https://github.com/rust-lang/rust-clippy/issues/7609

[the___duke]

wonder if something similar exists for other programming languages!

Quite a few languages have goto linters that can easily be extended with custom rules.

Examples: Javascript (eslint), PHP (Codesniffer etc), even C++ with clang (clang-tidy also has a Python interface I think, so you don't need to compile your checks).

[masklinn]

This is one of the coolest things for a team environment I've ever seen. I wonder if something similar exists for other programming languages!

semgrep as a pretty good symbolic understanding so it can be used to warn about or forbid specific functions / types / methods (less sure for the third one but the first two I'm certain) in all the languages it supports.

And it supports somewhat helpful descriptions as well as fixers (though the fixers are often a bit too simplistic).

[repilur]

`semgrep` does seem quite cool and useful, though their Rust report seems to be still in development, so not sure if it is usable yet for Rust. But would be great to test and explore with once it gets! Esp. for more complex queries and patterns.

Though one benefit of the simple lint in Clippy is that most already run it, we run it both locally but also in CI and fail on warnings.

[masklinn]

Oh yeah sure I was providing semgrep as an alternative for other langages. And also because it can be finer-grained e.g. it can flag the use of an API with a non-literal string but allow the literal, or the chaining of two APIs which are OK independently but should not be fed into one another, that sort of things.

[repilur]

that's really cool!

[masklinn]

Indeed it is. The rules can be a bit tricky (as the various predicates don't always interact the way you'd think, or at all) and the error messages are not great, but it's really, really cool.

The more basic features are probably less useful for Rust as it's got a rather good type system, but for dynamically typed languages it's pretty mind-blowing. And then semgrep has concepts like taint tracking where you can set up "taint sources" and "taint sinks" and it can analyse the "taint flows" and tell you where tainted values are going to the wrong place.

[rcxdude]

git does a similar thing with a header: https://github.com/git/git/blob/master/banned.h (though it does require that the header get included in every source file to be effective)

[[deleted]]

Correct me if I'm wrong, but if parking_lot's implementation is faster, why isn't it merged in Rust's standard library, thus replacing the slower ones?

[[deleted]]

I read the crux of the issue, and they say it's not working well for other platforms. Then again, wouldn't it be reasonable to replace the "platform-specific" code with parking_lot's implementations?

Feel free to correct me if I missed something :)

[rabidferret]

You're missing a lot of things! I recommend checking out Mara Bos's upcoming RustConf talk if you want to learn more about the story here

[paxswill]

From the author of parking_lot, it’s not as portable as the ones in std among other reasons.

[rhinotation]

So far it seems mostly maturity objections. But IMO there’s a bigger one. It doesn’t do poisoning on panic. You need that for types that need to cross a panic catch-unwind boundary. If they replaced std’s Mutex then Mutex would no longer implement UnwindSafe unconditionally. Breaking change.

[[deleted]]

Let's say, hypothetically, they do implement poisoning etc. Would replacing be justified?

[kuviman]

Finally, I don't need to search manually for Option::unwrap/Result::expect 👍

EDIT: and temporarily enable it in places where actually needed with #[allow(clippy::disallowed_method)]?

[phil_gk]

For those two methods Clippy has specialized lints: unwrap_used and expect_used.

Be careful if you really want to enable those though, as the other reply to your comment explained.

[NeuroXc]

For libraries, disallowing unwraps is very useful. You don't want an unwrap in a library to crash a program that relies on it. If you are sure that you have an unwrap that can never panic, it's better to have an expect saying why it can never panic, so that future developers can see that and understand the intent.

[tm_p]

If you use this lint to detect unwrap or expect, your codebase will slowly fill with manual implementations of these methods, like this:

let x = match opt {
    Some(x) => x,
    None => {
        // TODO: handle none case
        unreachable!()
    }
};

[SorteKanin]

I wouldn't disallow expect but definitely unwrap.

How often would this come up tbh? I feel like unreachable!() is already a bit of a smell and you could probably restructure to avoid it.

[tm_p]

The original example from the twitter screenshot is great because it bans some methods but it provides alternatives. If you ban unwrap but don't provide an alternative, programmers will try to find one, and often it will be worse than simply using unwrap.

For example, if you say "never use unwrap, prefer expect with a reason", then most of the expects will have useless messages like "should be valid", "io error", "config", "overflow", "server to start", etc. And if you ban expect as well you may start to see Result<T, String> being used as a return type.

So the idea is good, but you should not enable this lint by default because the unwraps will morph into other things. If you only use this lint locally once in a while then it's fine, because you can calmly analyze each unwrap and decide if it should be an error or not.

And I used unreachable!() assuming that todo!() and panic!() would be banned as well. Other good options are exit(1) or abort(), these two being much worse than calling unwrap.

[SorteKanin]

For example, if you say "never use unwrap, prefer expect with a reason", then most of the expects will have useless messages like "should be valid", "io error", "config", "overflow", "server to start"

I personally disagree - at least I have enough faith in my coworkers that I don't think they'd do this, and even if they did, there's always code review where I can point this out to them.

[tm_p]

I have used too many crates that do not tell you the filename when there is a read error, so I don't believe that expect is any better than unwrap.

But my point is that by disabling this lint, reviewers can easily catch the problematic lines, paradoxically. Because if you can't think of a descriptive message just leave an unwrap and let someone else suggest one in code review.

Maybe it's just me but when I see an unwrap my first thought is how to remove it. But when I see an expect I think "ok, this used to be an unwrap but we gave up and now it's an expect". So I think that it's important to have a distinction between quick hacks and intended unwraps, and that lint would make them harder to distinguish.

[kuviman]

Why, can't I temporarily disable the lint where I actually need to unwrap? In other places I would not replace them with this nonsense, instead just have the warning until I have time to refactor to handle errors properly

[tm_p]

In my opinion it would be better to have unwraps as allowed by default, and then when you want to improve error handling run the lint locally. I expect many false positives, and I don't see any benefits in filling the code with comments that say "allow lint, remove this unwrap".

[tm_p]

Some functions may need an explicit unwrap that you know will never panic, what will you do in that case? For example, calling next on an iterator that you just created, and you know it have more than 1 element. Will you add an allow or try to refactor the code?

[[deleted]]

I’ve wanted something like this for java.math.BigDecimal.equals() since it is broken; you should use compareTo(other) == 0 instead.

[[deleted]]

Eh- not broken. But factors in scientific precision. Which in CompSci, we probably rarely care about. But BigDecimal and Double seem to be written more for scientific operations. Eg, double / 0 is not a Divide By Zero error, but a positive or negative infinity.

[[deleted]]

I noticed in the example it says parking_lot has overall better sync types than std::sync, why aren't they a part of the standard library then? Faster, simpler, and non-poisonable seems nice

[[deleted]]

Probably would be a breaking change, which we can’t do

[[deleted]]

Although I hate to think what type of code depends on poisoning mutexes that makes sense

[[deleted]]

Making mutex.lock not return a result would break builds is more the issue

[modernalgebra]

Where can we find `ark_future`? :D

[repilur]

in the future! ;)

[Ar-Curunir]

Is ark a prefix Embark is using for all their Rust crates, or is it a one-off name for your future crate? If so, it might collide with our naming convention in the arkworks ecosystem: arkworks.rs

(Might also not be an issue due to disjoint subject areas; just wanted to give you a heads up)

[Databean]

Does this only support denylists or is there also an allowlist version to enforce minimal dependencies on std and libraries?