Inconsistent const_err on index access to array in polymorphic functions

[HeroicKatora]

Stable currently allows indexing an array with invalid constant indices in polymorphic functions without warnings. A recently nightly change has apparently changed this behaviour into an error. The following builds on stable without warnings but errors on nightly:

pub fn bugs(a: &mut [u8; 4], _: impl Fn(u8)) {
    a[4];
}

It should be noted that in monomorphic functions this is already an error in both version, though I do not know when this was introduced. That is, this is an error:

pub fn bugs(a: &mut [u8; 4]) {
    a[4];
    // error: index out of bounds: the len is 4 but the index is 4
}

It is also an error (in both versions) if the actual access is statically unreachable:

pub fn bugs(a: &mut [u8; 4]) {
    if false {
        a[4];
    }
}

Note: This breaks image and everything that depends on it, many versions.

The image crate makes use of the index access in a macro where the alpha components of a pixel are accessed (see here). It properly guards this with a check on the component number but, statically speaking, the access exists in the CFG even for types without alpha components where the index is of course invalid. This seems like a regression that would usually have an incompatibility warning period.

[wesleywiser]

I think this is related to #64419 which made the ConstProp pass a bit smarter. What should be happening is that you get a warning which, by default, is treated as an error. If you add #[allow(const_err)] to bugs(), it should continue to compile.

I believe this behavior is covered under RFC 1129 so this is expected behavior.

[HeroicKatora]

From the quoted RFC, emphasis mine:

If the constant evaluator encounters erronous code during the evaluation of an expression that is not part of a true constant evaluation context a warning must be emitted and the expression needs to be translated normally.

That RFC only seems to argue for not erroring. It remarks that this is a breaking change and one that would not be forseeable by the user since the compiler may get arbitrary additional powers of evaluating constant expression and goes on to specify that it should only warn if the context is not a true const expression. The function is not marked const, so the expression in question it is not a true const expression since it satisfies none of the five/six listed items.

• It's not an initializer of a const item
• It's not an array size expression
• Nor a repeat expression
• It doesn't specify an enum variant
• The context is not a pattern
• (const fn is surely a const expression now but the example given above is not)

Treating the warning as deny(const_err) by default seems to directly contradict translating the expression normally.

[Centril]

It is our normal policy that warnings may be turned into deny-by-default lints. The specific lint has been deny-by-default for a long time. What we cannot do is turn these into hard errors. That is, I think @wesleywiser is correct that this is expected behavior and I'm closing the issue accordingly.

[HeroicKatora]

@Centril Okay, so this was a misunderstanding on the exact wording, and you cleared up what the RFC was meant to express. However, this never showed up as a warning in the first place. Indeed, stable is silent even if turning this into an explicit warning. There was no way to diagnose that this would be an issue before the error in nightly right now. Under this aspect it does not behave like a warning having been turned into a deny-by-default lint.

[Centril]

It sometimes happens, such as in this case, that we extend existing lints to more cases. To avoid that we would need to split const_err into two lints: one that is warn-by-default, one that is deny-by-default. It is not clear to me that this would be an easy distinction to maintain but maybe @wesleywiser feels differently.

[HeroicKatora]

@Centril True, it would add uncertain complexity. To aid cost/benefit analysis, we're talking about image >= 0.19 here (I suspect there was no crater run, but do you happen to know what the next one after or including nightly-2019-09-26 was?). Also placing warn(const_err) on the macro block in question would be a suitable (shorterm) fix I suppose, right?

But more generally, doesn't continuing this train of though mean that any other crate which wants to stay long-term compatible with rustc has to add warn(_) for all deny-by-default lints since they could be expanded in scope? Only it can never know about future lints that may be added and turned deny so even that may not be enough.

At the end of the day, this can be solve like a library bug–publishing a new version. Those with necessarily old compilers stay unaffect, those with newer ones available can update. It's fine, thanks for consideration and time.

[sinkuu]

To me this seems like a loophole in --cap-lints. Can --cap-lints on the crate where a macro is declared be propagated to macro-expanded spans in downstream crates?

[HeroicKatora]

@sinkuu Just to clarify, the macro in question is declared in the image crate as well, it only helps with redundant implementation details. But the lint failing the build does propagate to downstream consumers.

[sinkuu]

@HeroicKatora Oh, the failing macro is used only in image crate? Then downstream consumers does not break, IIUC. Cargo sets --cap-lints allow to (non-path) dependencies, which silences even deny lints.

[HeroicKatora]

@sinkuu I wasn't aware of that and of course only tested by downloading and then adding a path replacement.. That's very calming to know 😄