Stream: t-compiler

Topic: Uplift lints from clippy to rustc #53224


nagisa (Aug 10 2018 at 14:13, on Zulip):

I was writing a long list of concerns before I realised that most of these can be quickly answered instead over chat

nagisa (Aug 10 2018 at 14:14, on Zulip):

@rfcbot concern false positives in eq_op?

Checks for equal operands to comparison, logical and bitwise, difference and division binary operators (==, >, etc., &&, ||, &, |, ^, - and /).

x ^ x or x - x could be an easy way to get yourself a literal zero in generic code that otherwise already depends on Sub or BitXor, but does not have any other means of getting a zero yet.

nagisa (Aug 10 2018 at 14:15, on Zulip):

concern unclear what level lints are aiming for by default


deprecated_semver needs some help from cargo to weed out false positives for crates which are known to use non-semver versioning scheme


concern logic_bug ignores short circuiting

and related

concern while_immutable_condition

Does this work correctly with variables marked as mut and modified not by the body, but by the hardware?

E.g.

spi_peripheral_write(0x42);
// SPI_PERIPHERAL_STATE_REGISTER may be changed by the hardware.
while (SPI_PERIPHERAL_STATE_REGISTER & READ_READY == 0) {
}
let read_byte = spi_peripheral_read();
varkor (Aug 10 2018 at 14:15, on Zulip):

using x ^ x to get a "zero" where there isn't naturally a zero seems a bit hacky — there's no guarantee this has to be zero for custom traits

nagisa (Aug 10 2018 at 14:16, on Zulip):

mut_from_ref: This lint checks for functions that take immutable references and return mutable ones.

This is trivially unsound

Does this complain about (because this is not exactly unsound as long as ensure_exclusivity does what its name says.

unsafe fn foo(x: &AtomicUsize) -> &mut u32 {
    static mut MY_STATIC: u32 = 0;
    ensure_exclusivity(x);
    &mut MY_STATIC
}
varkor (Aug 10 2018 at 14:16, on Zulip):

at the same time, this makes x ^ x not something you want to warn about, exactly because you don't have any guarantees

nikomatsakis (Aug 10 2018 at 14:17, on Zulip):

it would be easy to check that the types are integers, I imagine

varkor (Aug 10 2018 at 14:19, on Zulip):

seems reasonable for specific built-in types

nagisa (Aug 10 2018 at 14:19, on Zulip):

at the same time, this makes x ^ x not something you want to warn about, exactly because you don't have any guarantees

Yeah, you don’t want to lint against any of those in generic code, but it is unclear whether clippy takes care to check that, like for many other lints

nagisa (Aug 10 2018 at 14:20, on Zulip):

the clippy descriptions are very concise and not at all appropriate for making decisions like whether the lint should be included into the compiler

nagisa (Aug 10 2018 at 14:20, on Zulip):

cc @Oli ^

varkor (Aug 10 2018 at 14:23, on Zulip):

it'd be nice to have clippy support on the playground to test these things out directly

nagisa (Aug 10 2018 at 14:24, on Zulip):

it'd be nice to have clippy support on the playground to test these things out directly

Oh there is!

nagisa (Aug 10 2018 at 14:24, on Zulip):

on the right hand side there’s a "Tools" dropdown

nagisa (Aug 10 2018 at 14:24, on Zulip):

and in it there’s clippy. I absolutely forgot about that

nagisa (Aug 10 2018 at 14:26, on Zulip):

Well, the mut_from_ref definitely fails my test.

nikomatsakis (Aug 10 2018 at 14:27, on Zulip):

I do feel like some very minimal amount of false positives are ok —

nikomatsakis (Aug 10 2018 at 14:27, on Zulip):

not sure which of your examples meets that test

nikomatsakis (Aug 10 2018 at 14:27, on Zulip):

but if you are doing something very subtle

nikomatsakis (Aug 10 2018 at 14:27, on Zulip):

to my mind, it is ok to #[allow()] a lint — good place to leave a comment :)

nikomatsakis (Aug 10 2018 at 14:28, on Zulip):

basically that's kind of my rule

nikomatsakis (Aug 10 2018 at 14:28, on Zulip):

if I wrote that code, would I also write a comment like // Subtle: ...?

nikomatsakis (Aug 10 2018 at 14:28, on Zulip):

if so, a lint makes sense, since it reminds me to write the comment :)

nagisa (Aug 10 2018 at 14:30, on Zulip):

well, neither of my examples are subtle, but rather something that you wouldn’t really do on your usual hosted platform

nagisa (Aug 10 2018 at 14:31, on Zulip):

some of the lints seem to be fairly opionated on how exactly a platform should work. We could allow-by-default these lints on platforms which do not adhere to the opinions though.

nikomatsakis (Aug 10 2018 at 14:32, on Zulip):

I consider

unsafe fn foo(x: &AtomicUsize) -> &mut u32 {
    static mut MY_STATIC: u32 = 0;
    ensure_exclusivity(x);
    &mut MY_STATIC
}

somewhat subtle.

nikomatsakis (Aug 10 2018 at 14:32, on Zulip):

I'm not convinced it even makes sense -- when/how does the "lock" from ensure_exclusivity get released?

nagisa (Aug 10 2018 at 14:33, on Zulip):

I would write such code (well, with some additional linkage attributes) on a bare metal program in a blink of an eye

nikomatsakis (Aug 10 2018 at 14:33, on Zulip):

and you ought to comment it =)

nikomatsakis (Aug 10 2018 at 14:33, on Zulip):

but yes I might be overlooking the platform specifics or something, I don't know

nikomatsakis (Aug 10 2018 at 14:34, on Zulip):

in general I agree that compiler should be conservative when it comes to false positive..

oli (Aug 13 2018 at 09:10, on Zulip):

I'm absolutely fine with making this a hosted-platform-only lint. I didn't consider embedded at all for this.

oli (Aug 13 2018 at 09:16, on Zulip):

wrt eq_op: we can totally ignore it if the args are of a generic type for value computing operators. I don't think comparison operators are problematic like this. While x == x can be false (for e.g. floats), there are definitely better ways to check constraints

Last update: Nov 16 2019 at 01:10UTC