Defensive programming clippy lint category

tools and infrastructure
1

Continuing the discussion from Linting and match ergonomics:

The very condensed version of all the match ergonomics discussions lately is that we want a way to enforce that a project’s code contains little comfort code and favours explicitness. This simplifies reviews e.g. in the (human!) safety critical sectors, because one does not need to know much about inference, autoderef, autoref, method resolution, … to figure out what a single line of code is doing.

Nonexhaustive list of lints that this category could contain:

  • prefer Type::method(x, y, z) <Type as Trait>::method(x, y, z) over x.method(y, z)
  • prefer let y: &u32 = &***x; over let y: &u32 = x; for some x which can be autodereferenced to an &u32
  • prefer let &S(ref foo) = bar; over let S(foo) = bar; for reference type bar
    • except if explicitly requested like in let S(foo) = &bar;
  • prefer let x: Type = y; over let x = y;. This can be fully automatically suggested and applied by rustfix. So the user can code with inference, and the clippy suggests inserting type annotations with the inferred types.
    • let x = |a| a + 1; and similar unnameable types are excluded of course. This might be fixed later by requiring let x: impl FnOnce(i32) -> i32 = |a| a + 1; or by requiring type annotations for closure arguments. We should probably not require both, as that is very verbose.
  • prefer foo::<T, U>(args) over foo(args). Inferring generic arguments can cause subtle changes. E.g. transmute(foo) will still work after foo's type changes as long as the size doesn’t change.
12 Likes
2

Thank you for starting this! The point about explicit generic arguments is a good one I hadn’t thought of yet, especially with transmute since I had struggled with that exact problem a bit ago.

Some others I could think of:

  • Requiring lifetime annotations. Although it might be better here to wait for the outcome of the current lifetime change discussions. Things like in-band lifetimes could increase a need to be explicit in certain context, while an addition like the '_ marker might actually decrease it and give more situations where things are quite clear.
  • Explicit (). This would probably need a lot of thinking. I only tend to do explicit () instead of relying on ; when I’m in generic context, and the ; looks more like a typo than an intended result. It’s basically a situation where the () replaces a comment explaining that I do want () there. But that hasn’t come up in a while, so I’m unsure about a good set of activating circumstances.
  • For some types like (for example) trees and graphs it would be nice if there was a solution to catch not-considered-enough &mut references leaking, where actually a wrapper type providing mutation should be given. This would catch chances for things like replacing or swapping nodes.
  • I’m wondering if some of these could go hand-in-hand with adding defensive abilities to std. For example: I’ve written traits providing things like borrow_scoped for RefCell a couple times now, that allow explicitly delimiting the scope for which a lock is active. Having a lint requiring those would allow one to use lint deactivation and use of the common borrow functions to easily discern between the uses where a lock is only used in a scoped context and where it has to escape.
1 Like
3

This is great – especially because RFC 1105 assumes that breaking changes are only those that break your defensively written code.

1 Like
4

One more:

for x in y {}

should specify if the y is a value, reference or mutable reference by using the appropriate method via UFC

1 Like
5

Does this include let ref x = &1;? (This discussion made me realize that Rust allows explicitly marking x as being a reference, which I didn't even know.)

Shouldn't this be let S(foo) = *bar;? Granted you never said what the type of bar is. I'm guessing &S(_)?

6

what about match v { x => ... } vs match v { A::x => ... } vs match v { x @ _ => ... }?

7

let ref x = &1 makes x be &&i32. Not sure what your question is.

Granted you never said what the type of bar is. I’m guessing &S(_) ?

The type of bar is irrelevant. the & right in front of the pattern is an explicit request for match ergonomics in my book.

Not sure what you are asking. Can you elaborate what each of your examples is weighing against the other examples?

8

Sorry, I must've made a mistake in testing this out, just ignore that. It doesn't work the way I thought it did.

Perhaps you could entertain me a little and explain your reasoning (just trying to understand)? Examples might help.

9

Well… there is no reason to ever write match &foo without match ergonomics. Any other use case would require you to also annotate all patterns. Explicitly matching on a reference that is created at the match site feels like using the underscore in _foo to silence the unused binding lint. In this case it would silence the explicit patterns lint.

10 
#[allow(whatever)]
const x: i32 = 0;

let v = 0;
match v {
    x => {},
    _ => {}
}
11

That is already linted by the rust compiler.

2 Likes
12

look, unless you can look at it and instantly tell what it means, it sucks.

you could require all patterns to habe either :: or @ and solve the ambiguity problem.

13

You specifically allow a non SCREAMING_SNAKE_CASE name that would normally be linted so I don’t think it is weard when this makes your code less readable. Requiring :: or @ in all patterns seems like an extreme solution to a nonexistent problem.

3 Likes
14

if it’s all about being explicit about your intentions, I feel like requiring @ or :: does a pretty good job of that. thus there should be lints about it, in the name of explicitness.

15

Not only did you allow a lint to do that, but if it wasn’t a const match, then you’d get a dead_code warning since the _ arm would never match.

I think that case is well-known enough that it already has sufficient lints about it, and they’re even on-by-default.

16

make it so humans can look at it and instantly be able to tell what it's doing, no need to compile!

17

That’s already the case if you name your constants with SCREAMING_SNAKE_CASE instead of silencing the lint that forces you to do so.

1 Like
18

const en_US: Language = ...;

my style guide prioritizes working around language ambiguities, not making assumptions about constants or w/e.

19

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.