Why doesn't rust warn about this missing match / typo?

1

This bug caused data corruption for me. It's really nasty, and I can not believe it compiles. Even asking for "help" on SO about this has given me no answers as to why it even compiles.

#![allow(non_camel_case_types)]
#![allow(non_snake_case)]
#![allow(unused)]

use PlayerItemTypes::*;

pub enum PlayerItemTypes {
    MAGIC_LAMP,
    DEATH_DOLL,
    SCROLL_OF_EXIT,
}

impl PlayerItemTypes {
    pub fn GetDisplayName(&self) -> &str {
        return match (self) {
            //MAGIC_LAMP     => "Magic Lamp", // should warn about non-exhaustive
            DEATH_DOLL     => "Death Doll",
            SCROLL_EXIT    => "Scroll of Exit",
        }
    }
}

fn main() {

}
2

See e.g. this previous topic:

3 Likes
3

all these lints would give you some clue about something was wrong. if you really understand why the compiler warns about them, it should not be surprising at all that it compiles.

8 Likes
4

Thanks, I looked through that thread.

" but I do agree that in a match expression, Ident => { stuff } binding the ident is quite counterintuitive. I would have much preferred requiring the @ binding, as in Ident @ _ => { stuff }"

??? I'm still baffled. Where is the doc on match expressions that say it can just make up symbols like that.

5

I have unused off because warnings about unused usings are the LAST thing I want to see when working on code. Especially at the top of my error window (which I already have to scroll through enough). I don't frigging care if I left a use std::fmt in or not.

6

Meant to reply to you. Thanks for the thread. As you can see, all I'm trying to do is use a plain old C-Style Enum as a bunch of const ints that I want to match on exhaustively. And rust is completely failing at doing that. I don't actually see any solutions in that thread for my pain, but only solidarity with the sufferings of others.

7

You can make the match pattern not just a plain identifier to work around this for now.

#![allow(non_camel_case_types)]
#![allow(non_snake_case)]

pub enum PlayerItemTypes {
    MAGIC_LAMP,
    DEATH_DOLL,
    SCROLL_OF_EXIT,
}

impl PlayerItemTypes {
    pub fn GetDisplayName(&self) -> &str {
        use PlayerItemTypes as P;
        return match self {
            P::MAGIC_LAMP     => "Magic Lamp", // should warn about non-exhaustive
            P::DEATH_DOLL     => "Death Doll",
            P::SCROLL_OF_EXIT => "Scroll of Exit",
        }
    }
}

fn main() {}
6 Likes
8

These are called identifier patterns.

I personally consider the answer marked as a solution as the best way to guard against typos, if you don't want to use prefixed variant paths:

#![allow(non_camel_case_types)]
#![allow(non_snake_case)]
#![allow(unused)]

use PlayerItemTypes::*;

pub enum PlayerItemTypes {
    MAGIC_LAMP,
    DEATH_DOLL,
    SCROLL_OF_EXIT,
}

impl PlayerItemTypes {
    pub fn GetDisplayName(&self) -> &str {
        #[deny(unused_variables)]
        return match (self) {
            MAGIC_LAMP     => "Magic Lamp", // should warn about non-exhaustive
            DEATH_DOLL     => "Death Doll",
            SCROLL_EXIT    => "Scroll of Exit",
        }
    }
}

Playground.

3 Likes
9

I tried setting #![deny(unused_variables)] in lib.rs. Sadly it causes a bunch of errors on unused function parameters too, and I have a lot of callbacks that sometimes don't need all the params they're passed.

It would be really great of those were separable warnings.

But I think I will go through and _ all the unused params, just so I can be sure that I'm not missing a match guard from this "feature".

10

This is a pretty brave statement. Maybe check https://doc.rust-lang.org/rustc/lints/groups.html how many lints get knocked out by this (scroll to the bottom).

So you can use this:

#![allow(unused_imports)]`

Just add an initial underscore to unused variables:

fn some_callback(x: u32, _y: u32) -> u32 {
    x + 1
}

Hmm, so Rust is not C. It's a different language, different style, different ideas, different solutions.

For experienced programmers in one language like you, it's not easy to unlearn some known patterns and to sink into the new language.

Maybe, some idea:

Is it really necessary, to use upper case? Why not stick to Rust conventions?

Dump the unnecessary Types postfix, qualify enum members, use Self, remove unnecessary return, and omit the parenthesis in the match argument:

pub enum PlayerItem {
    MagicLamp,
    DeathDoll,
    ScrollOfExit,
}

impl PlayerItem {
    pub fn get_display_name(&self) -> &str {
        match self {
            Self::MagicLamp    => "Magic Lamp",
            Self::DeathDoll    => "Death Doll",
            Self::ScrollOfExit => "Scroll of Exit",
        }
    }
}
6 Likes
11

ok mroth, since you asked for it and are trying to stay constructive.

I never found that "lint groups" page because I was searching google for warnings. The compiler calls them warnings And half of the warnings I was getting from rustc didn't show the way to shut them up. I ended up finding some reddit post to say to use rustc -W help.

I was very tired of getting yelled at for snake case, camel case, extra parens, unused imports, unnecessary mut, and must use from write! when writing to a string buffer -- all when I was struggling with the arcane borrow checker syntax, and doing my first (and probably last) hobby project with this language, much of which was porting code from c#.

I'm not experienced with one language, I'm experienced with many over the last 25 years. And they've all had "new style conventions" that don't make one flipping bit of difference in production code. How is not being able to use the 'use' feature of rust, and instead putting Self:: everywhere, actually a good thing. How would matching on Self be able to match anything besides a type of self in the first place?

These things aren't types. They're just const ints, that are related. This is important for serialization. I'm not doing some academic type theory here. These are just integers that are serialized to a game data save file.

I read (many) times that the way to match all is _. But it appears there are other magic ways to match _. And it appears I can't actually use 'use' when I want. All of this makes me sad.

To be blunt, I would have been just as productive with c++ and a checked iterators library, or especially c# AOT, but that would have made a bigger wasm download. The only reason I was experimenting with rust was for WASM download size vs c#.

Rust is an innovative language, but it is not fun to work with. Perhaps it should be called 'bikeshedc', if it cares SO MUCH About not having screaming case for const ints, like people have done for 50 years.

I have found that people who are really experienced do not have this religious mentality about names of variables, but instead, care about if the language has footguns and is it cohesive, and does it have principle of least surprise.

2 Likes
12

I think, this issue comes up often enough, and one can run into it easily enough, that I’d say we do need more/better lints here eventually. Two ways lints could become better IMHO:

  • transferring some aspects of the existing lints (about casing convention, and/or unreachable match arms, and/or unused variables introduced in contexts of (fallible) pattern matching) to dedicated lints that are less likely to be "annoying" in any circumstances irrelevant to this kind of match issue, and not easily disabled together with other lints
    • for instance, I wonder why non_snake_case, disabling casing conventions for all things that are usually snake-case, needs to be so coarse; it looks like OP’s example might be "only" needing this for UpperCamelCase function&method names; but if that use case could be specified more concretely, then an UPPER_CASE identifier (not UpperCamelCase) for a local variable should ideally never not have been linted in the first place
  • coming up with new lints that try to specifically detect cases where confusion (between identifier patterns and prefix-free path patterns) seems very likely
    • these could (eventually; perhaps partially) even become deny-by-default, for very clear cases; and include actionable feedback, such as e.g. rewriting SCROLL_EXIT into SCROLL_EXIT @ _, for avoiding the lint in a specific case (without needing to disable it more broadly)

I don’t think it would really fit the style/quality of rustc error messages to just conclude that it’s the user’s fault. Also, of course, it’s a given that developing experience can be degraded with allow(unused) – e.g. if you typo your variable name, and refer to a different variable (of the same type, further up in the code) instead, accidentally, a lint calling the correct variable out as unused might have saved you some debugging; but at least, the issue is more easy to debug, and also it’s a much more natural/plausible/immediate consequence of the kinds of lints that are being disabled here, so no-one should feel the need to be annoyed about the compiler or language from such an interaction.

4 Likes
13

The point of having a default formatting style that a default linter encourages is to make the code you write easier to read for others (and once you're used to it, others easier for you) - but yes they are only style lints, which is why it doesn't say anything in a default compile, you need to explicitly run the linter and you can easily disable them.

It's really not worth getting that worked up about, other than this is a known weakness that not using a standard style exacerbates.

I will specifically call out though, that Rust enums are not replacements for C or C# enums, and if you treat them as such you are making a lot more work for yourself, well into the "make one flipping bit of difference in production code" territory.

Rust does feel weird, even if you do have experience with many languages, especially if those languages are all in the C/Java family, as it is drawing from a lot of other language families in it's design. As you become more familiar with it, the weirdness goes away; for example the "identifiers match everything like _" behavior becomes quite natural when you connect together how pattern matching actually works: of course it does, otherwise how would let x = y; work? It's the same feature!

8 Likes
14

OP:

Also OP:

Gee, I wonder. It's almost like if you turn off the warnings, you don't get warnings :roll_eyes:

8 Likes
15

I think the main issue is the large disconnect between the kind of warnings being disabled, and the kind of issue that arose from it. I don't think there's anything obvious in how “disabling casing convention lints, and an unused items / dead code lints + some use Enum::*; being used = your match expressions become hard to use right”.

Ideally, there should be an warning relating more specifically to the topic of (fallible) pattern matching, which would need to be explicitly allowed separately, before the mechanisms fully break down that make this whole problem a non-issue for most experienced Rust users — something like a minimally sufficient subset of linting on casing, importing, and/or used variables, that still suffices to prevent the issue in all cases. When manually disabled anyway, it would be something obvious to quote as “here's the lint you disabled that was specifically designed to prevent this problem”. And additionally it would be nice-to-have if some particularly obvious cases of “there almost certainly was an actual confusion here” could even produce an error (denied lint) by default.

15 Likes
16

Right off the bat, sorry it happened to you - that experience is never fun.

As I'm sure your 25+ years of experience have shown you, every PL is different. Quite a few of them are / may appear to be fairly similar, I'll give you that - yet to come from a background X into a new environment Y while expecting your overall experience Z to remain roughly the same, isn't always the best idea. Having to force your brain into picking up new habits, instead of reverting to the "same ol' ways we've always done things 'ere" isn't that fun, either. Especially not when you're dead set on the end result (porting the code) instead of the process (figuring it all out). With that -

Every language has its fair share. async void and using HttpClient() in C#, coercions of true-y and false-y values in JS, the "01/02 03:04:05PM '06 -0700" reference time layout in Go, the bit-field std::vector<bool> in C++. Whether any of them are, or appear to be, cohesive and/or surprising is, mostly - a matter of an individual's own habit, experience, and expectations.

I could try pointing out that most of what comprises Rust nowadays is a little more than an amalgamation of the best practices from a whole bunch of other PL's, put together in a way that appeals to the sense of "fun" for quite a few - though I doubt it'd do either of us any favour.

You're upset about the (buggy) feature of a (unfamiliar) language that you (might) have inadvertently walked into yourself by explicitly #![allow(...)]'ing yourself a great deal than a newcomer (to this PL) would be expected to (regardless of prior experience). That would be neither your fault, necessarily - as relying on your prior 25+ years in the field have served you more than well up to this point, of that I'm certain. Nor of the language/compiler/linter itself - the designers of which have had to make their own choices, each with its own pros and cons.

Everybody can learn something here. I just hope you'll be amongst those who do, by the end.

11 Likes
17

I think warnings are too configurable. How about making the naming conventions obligatory rather than just warnings? Then this pattern context-sensitivity would go away, and error messages would probably be better because it would be unambiguous what is supposed to be a variable and what isn't.

2 Likes
18

Because then you need to figure out how to case "iOS" as an enum variant, or how to document how your bindings library distinguishes between ReadFile and READ_FILE, or a bunch of other weird edge cases (also why you can use keywords as identifiers via r#foo).

If there's no actual immediate, unconditional problem disabling a diagnostic would cause, then it's really just a lint, not an error.

1 Like
19

Easy: IOS.

I think that maybe just the first letter would be enforced because it's sufficient to resolve the syntax ambiguity.

An alternative solution would be to require const { MAGIC_LAMP } in the pattern if it's a single identifier of a constant without a path.

You could have R#foo as the upper-case escape, r#foo as the lower case. So you could say: R#iOS.

By that rule a lot of language rules would be configurable. Accessing private members, calling unsafe functions outside of unsafe, mutating non-mut variables.

20

IOS violates most casing rules. If you're going to allow ARBITRARY yelling case, it's not much of an enforced rule, is it? But sure, a rule like "warn if a lowercase variable doesn't resolve to an identity pattern or vice versa" is likely what the suggested lint would end up looking like, but you do need an escape hatch for general compatibility and sanity reasons. As I understand it, there's issues with many of the options last time this came up, I haven't reread that thread.

In most languages, including Rust, visibility is a requirement to be able to make versioning guarantees, which I would say means breaking it is an immediate harm of a sort; and it many yet many languages yet let you bypass that privacy (through creating a proxy in the same package, reflection etc) - perhaps it's merely a very poorly designed lint in those languages? In Rust is even worse though, as module visibility boundaries are required to be able to provide safety guarantees; breaking such visibility would necessarily be unsafe, speaking of which...

Yes, people have asked for and it's been considered, for Rust to have some option to disable the need for unsafe blocks, at least at a larger scope than statements - consider FFI wrapper modules where it's mostly noise. Further, the lint unsafe_op_in_unsafe_fn is literally a lint that checks for a missing unsafe, so really I don't at all consider it strange to call requiring unsafe a lint; just an fairly strong one. Heck, what else is an unsafe block but temporarily disabling a diagnostic in the first place?