Repository navigation
Inconsistent const_err on index access to array in polymorphic functions #65005
Description
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.
Activity
wesleywiser commentedon Oct 2, 2019
I think this is related to #64419 which made the
ConstProppass 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)]tobugs(), it should continue to compile.I believe this behavior is covered under RFC 1129 so this is expected behavior.
197g commentedon Oct 2, 2019
From the quoted RFC, emphasis mine:
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
constexpression. The function is not markedconst, so the expression in question it is not a trueconstexpression since it satisfies none of the five/six listed items.constitemconst fnis 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 commentedon Oct 3, 2019
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.
197g commentedon Oct 3, 2019
@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 commentedon Oct 3, 2019
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_errinto 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.197g commentedon Oct 3, 2019
@Centril True, it would add uncertain complexity. To aid cost/benefit analysis, we're talking about
image >= 0.19here (I suspect there was no crater run, but do you happen to know what the next one after or includingnightly-2019-09-26was?). Also placingwarn(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
rustchas to addwarn(_)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 turneddenyso 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 commentedon Oct 3, 2019
To me this seems like a loophole in
--cap-lints. Can--cap-lintson the crate where a macro is declared be propagated to macro-expanded spans in downstream crates?197g commentedon Oct 3, 2019
@sinkuu Just to clarify, the macro in question is declared in the
imagecrate as well, it only helps with redundant implementation details. But the lint failing the build does propagate to downstream consumers.sinkuu commentedon Oct 3, 2019
@HeroicKatora Oh, the failing macro is used only in
imagecrate? Then downstream consumers does not break, IIUC. Cargo sets--cap-lints allowto (non-path) dependencies, which silences evendenylints.197g commentedon Oct 3, 2019
@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 😄