Statics shadow local variables causing "refutable pattern error", and non-obvious bugs.

[huonw]

rusti> static a: uint = 1;
rusti> let a = 2;
<anon>:29:4: 29:5 error: only refutable patterns allowed here
<anon>:29 let a = 2;
              ^

A particularly insidious example is:

static a: uint = 1;
// ... hundreds of lines of code/several modules later ...
match (1u,2u) {
   (1, a) => println(a.to_str()),
   (x, y) => println((x+y).to_str())
}

since one could easily imagine not noticing that the first arm doesn't run, and/or spending hours debugging it.

(#7523 added an lint enforcing statics as uppercase, warn'd by default, which makes this less likely to occur. There was some discussion about turning this to allow by default if/when the error messages are improved. Update 2013-7-22: has been turned to allow but there is no improvement in error messages.)

[pnkfelix]

visiting for triage, email from 2013 sep 30.

still relevant. The lint was turned to allow in SHA: 23fbe93

[pnkfelix]

(maybe there should be a separate lint for this match pattern issue, where if there is a static in scope of a pattern with the same name, then this more specific lint fires. I'm not sure @alexcrichton was aware of this bad behavior when he set the lint to default to allow...)

((my initial idea above was dumb: Its doubtlessly by design that statics can appear as constants in patterns. The issue is that they are easily mistaken for binding identifiers. So the lint could instead warn about lowercase static constants that are occurring as pattern bindings; its again a refinement of the aforementioned non-uppercase-statics lint, but one that we can probably stand behind turning on by default.))

[pnkfelix]

(I think adding such a lint for non-uppercase statics in match arms may not be so hard. Giving it a shot now.)

[huonw]

@pnkfelix checking function/closure arguments and let variables would be good, since they're pattern contexts too. (Although they "just" give weird error messages about refutable patterns, not silently changing program behaviour, so it isn't as important.)

[pnkfelix]

@huonw yeah I'm more concerned about the bug you outlined in the description. From what I saw in the code, its not necessarily "natural" to put this check into a place that would cover all of the cases you listed.

(oops, hit close-and-comment by accident.)

[pnkfelix]

cc #3070

[pnkfelix]

(and likewise, it would be good if the new lint, or another new one, also warned upon encountering all-uppercase identifier patterns that are not references to a static, but instead are binding patterns, since that is probably a sign of bug such as an alpha-renaming gone wrong.)

[alexcrichton]

I understand now that it's a bit more of a problem, but I still think that by-default all-caps statics may not be the best solution. I agree that a smarter lint mode would be a good candidate for warn-by-default.

[pnkfelix]

This is pretty close to being closeable now that PR #9638 has landed. The main thing left would be to improve the error message for the "refutable pattern error" (sic) to point at the span for the static that is causing the rebinding attempt to fail.

(I wrote "(sic)" above because I'm pretty sure we should be saying that only irrefutable patterns are allowed in e.g. let x = 3;, based on my understanding of what the words "refutable" and "irrefutable" mean. A "refutable" pattern is one that could be proven false, which I take to mean "one that is allowed to fail to match up", and an "irrefutable pattern" would be one that cannot fail to match, which sounds like the kind of pattern that fits with let PAT = EXPR.)

[pnkfelix]

(also, I think my interpretation of "refutable" and "irrefutable" is consistent with the documentation in rust.md. So I think I'll probably change the text here to say "irrefutable" in the error message.)

[pnkfelix]

(PR #9683 needed a rebase, so I closed it. Either I'll get back to it in the future and rebase it, or someone else can pick it up. But this ticket (#7526) should probably not be closed until the error message issues have been addressed.)

[Ape]

There's also the issue with shadowing even when the name shouldn't be visible. For example this case:

mod submod {
    // 'number' shouldn't be visible here (only with 'super::number')
    pub fn subfn(number: int) -> int {
        2 * number
    }
}

static number: int = 1;

fn main() {
    submod::subfn(3);
}

Compiler fails like this:

modnameleak.rs:3:14: 3:20 error: only irrefutable patterns allowed here
modnameleak.rs:3    pub fn subfn(number: int) -> int {
                                 ^~~~~~

[nstoddard]

I hit a variant of this today:

const x: int = 5;

//possibly thousands of lines later in a different module
fn main() {
  let x = 10;
  println!("{}", x);
}

This gives the following error:

test.rs:4:7: 4:8 error: only irrefutable patterns allowed here
test.rs:4   let x = 10;
                ^

Which is a very confusing error message.

[steveklabnik]

triage: same irreffutable pattern error, which is understandable but sad.

[steveklabnik]

I'm going to add the 'lint' tag in case this is something we want to lint, but it may just need a better error.

[Manishearth]

Triage: no longer happens for statics. Still happens for consts.

[Manishearth]

#29777