Warn about ignored generic bounds in for
#48326
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
RalfJung
force-pushed
the
generic-bounds
branch
from
42b8ad3 to
0c5bc12
Compare
It's okay.
Yes, hardcoded warnings should be avoided whenever possible (I don't think we should remove the mechanism itself though).
Everything looks covered.
Crater can answer this! cc @aidanhs |
src/test/ui/param-bounds-ignored.rs
Outdated
| @@ -9,12 +9,14 @@ | |||
| // except according to those terms. | |||
|
|
|||
| // must-compile-successfully | |||
| #![allow(dead_code, non_camel_case_types)] | |||
There was a problem hiding this comment.
Could you warn(ignored_generic_bounds) and add //~ WARN ... annotations to the file?
It's very hard to see what is tested, what should be reported, and what shouldn't be reported based on stderr file alone.
There was a problem hiding this comment.
Ah, I didn't even know //~ WARN was a thing in ui tests. How do the two interact?
| --> $DIR/param-bounds-ignored.rs:39:8 | ||
| | | ||
| 39 | f: for<'xa, 'xb: 'xa> fn(&'xa i32, &'xb i32) -> &'xa i32) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
Spans could be more precise, AST has spans for generics and for the ignored bounds themselves.
There was a problem hiding this comment.
I'm now changing this so the span points at the first ignored bound. Is there a way to "combine" spans? I found no span for all the bounds together, just the generic (which is the wrong place) and individual bounds.
petrochenkov
added
S-waiting-on-crater
The warning should be forbid-by-default for cratering though. |
So are you saying I should change this PR to make the warning err-by-default? |
|
Turns out that even some of the compile-fail tests have these bounds in |
Done. |
|
There's a strange failure in I have no idea how my changes could cause that. |
|
The backtrace points at which I did not see because it's JSON-ified: Since this did not happen on Travis, I assume it's a stage-1-only failure. I'm currently building stage 2 to verify. (EDIT: confirmed stage-1-only failure.) |
RalfJung
force-pushed
the
generic-bounds
branch
from
8657731 to
9410a67
Compare
I guess alternatively I could make it warn about every single bound, but that feels excessive? |
|
You can pass a |
src/test/compile-fail/issue-39122.rs
Outdated
|
|
||
| type Bar<T> where T: std::ops::Add = T; //~ WARNING E0122 | ||
| type Bar<T> where T: std::ops::Add = T; //~ WARNING where clauses are ignored |
There was a problem hiding this comment.
This test is now a strict subset of the new ui/param-bounds-ignored. Should I remove it?
There was a problem hiding this comment.
Yeah, the less code the better!
Wow, fancy. Done :) |
RalfJung
force-pushed
the
generic-bounds
branch
from
e26c863 to
c2a8e23
Compare
I made it Deny-by-default. (There are some existing Deny-by-default lint and no Forbid-by-default lint, so I figured I'd stay consistent with them.) |
petrochenkov
added
S-waiting-on-review
| @@ -10,15 +10,15 @@ | |||
|
|
|||
| pub enum Expr<'var, VAR> { | |||
| Let(Box<Expr<'var, VAR>>, | |||
| Box<for<'v: 'var> Fn(Expr<'v, VAR>) -> Expr<'v, VAR> + 'var>) | |||
There was a problem hiding this comment.
Looks like the use of bounds was intentional here (#23046).
There was a problem hiding this comment.
Need to check whether they are totally ignored during lowering to ty representation or just not enforced properly (and still may cause ICEs or something).
There was a problem hiding this comment.
Okay I will bring the bound back. So far I haven't observed anything that would react to these bounds, and AFAIK @eddyb also said they are ignored.
Are you saying I should check the lowering code whether they are ignored? I'm not familiar with that code at all, not even sure where it lives, but I can certainly try.^^
For running crater on existing lints forbid-by-default allows to still catch cases where the lint was |
|
Travis found some failing run-pass tests that need to be updated. Everything else looks good to me, only crater run is remaining. |
petrochenkov
removed
the
S-waiting-on-review
|
Crater needs to be fed with a try build so it'll need to get through travis, which is unfortunately probably not going to happen with test failures. (also, please ping @rust-lang/infra for crater runs rather than me directly!) |
|
@bors p=1 segfaulty, not including in rollup, should be tested on its own when there's a slot |
|
@Manishearth The segfault is likely introduced by the miri PR, not this, thus the retry. I agree this (+324, -108) PR shouldn't be rolled up though 😄. |
|
⌛ Testing commit 780b544 with merge 50f2d3d3b6c75b4be1f4f44f1cf7555d2f4dba05... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
@bors retry segfault on dist x86_64-apple-darwin xcode 7.3 |
bors
added
S-waiting-on-bors
|
Sent a PR against the only crate this should break (according to crater): rustgd/rhusics#62 |
62: Remove lifetime bounds in higher-ranked lifetimes r=Rhuagh a=RalfJung Those bounds are ignored by the compiler, and will trigger an error once rust-lang/rust#48326 lands. According to crater, this is the only crate using these bounds. This patch should make the crate work on nightly again. However, I have been unable to compile the crate even with nightly 2018-02-25, which does not include the change mentioned above. So, I couldn't compile-test this PR. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/rustgd/rhusics/62) <!-- Reviewable:end -->
Warn about ignored generic bounds in `for` This adds a new lint to fix #42181. For consistency and to avoid code duplication, I also moved the existing "bounds in type aliases are ignored" here. Questions to the reviewer: * Is it okay to just remove a diagnostic error code like this? Should I instead keep the warning about type aliases where it is? The old code provided a detailed explanation of what's going on when asked, that information is now lost. On the other hand, `span_warn!` seems deprecated (after this patch, it has exactly one user left!). * Did I miss any syntactic construct that can appear as `for` in the surface syntax? I covered function types (`for<'a> fn(...)`), generic traits (`for <'a> Fn(...)`, can appear both as bounds as as trait objects) and bounds (`for<'a> F: ...`). * For the sake of backwards compatibility, this adds a warning, not an error. @nikomatsakis suggested an error in #42181 (comment), but I feel that can only happen in a new epoch -- right? Cc @eddyb
|
☀️ Test successful - status-appveyor, status-travis |
…chenkov Improve lint for type alias bounds First of all, I learned just today that I was wrong assuming that the bounds in type aliases are entirely ignored: It turns out they are used to resolve associated types in type aliases. So: ```rust type T1<U: Bound> = U::Assoc; // compiles type T2<U> = U::Assoc; // fails type T3<U> = <U as Bound>::Assoc; // "correct" way to write this, maybe? ``` I am sorry for creating this mess. This PR changes the wording of the lint accordingly. Moreover, since just removing the bound is no longer always a possible fix, I tried to detect cases like `T1` above and show a helpful message to the user: ``` warning: bounds on generic parameters are not enforced in type aliases --> $DIR/type-alias-bounds.rs:57:12 | LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases | ^^^^^ | = help: the bound will not be checked when the type alias is used, and should be removed help: use absolute paths (i.e., <T as Trait>::Assoc) to refer to associated types in type aliases --> $DIR/type-alias-bounds.rs:57:21 | LL | type T1<U: Bound> = U::Assoc; //~ WARN not enforced in type aliases | ^^^^^^^^ ``` I am not sure if I got this entirely right. Ideally, we could provide a suggestion involving the correct trait and type name -- however, while I have access to the HIR in the lint, I do not know how to get access to the resolved name information, like which trait `Assoc` belongs to above. The lint does not even run if that resolution fails, so I assume that information is available *somewhere*... This is a follow-up for (parts of) rust-lang#48326. Also see rust-lang#21903. This changes the name of a lint, but that lint was just merged to master yesterday and has never even been on beta.
Generic param order restriction was changed in rust-lang/rust#58191. Generic argument order restriction was changed in rust-lang/rust#70261. `for` lifetime argument restriction was changed in rust-lang/rust#48326. Generic parameter parsing: https://github.com/rust-lang/rust/blob/206ee1eea3467fd1d7f1efdbeafe27880897bb2c/compiler/rustc_parse/src/parser/generics.rs#L83-L153 Generic argument parsing: https://github.com/rust-lang/rust/blob/17eec1433c69972844dd228b5fe801f218e118c3/compiler/rustc_parse/src/parser/path.rs#L395-L413 `for` argument parsing: https://github.com/rust-lang/rust/blob/206ee1eea3467fd1d7f1efdbeafe27880897bb2c/compiler/rustc_parse/src/parser/ty.rs#L724-L736 Fixes rust-lang#785
Generic param order restriction was changed in rust-lang/rust#58191. Generic argument order restriction was changed in rust-lang/rust#70261. `for` lifetime argument restriction was changed in rust-lang/rust#48326. Generic parameter parsing: https://github.com/rust-lang/rust/blob/206ee1eea3467fd1d7f1efdbeafe27880897bb2c/compiler/rustc_parse/src/parser/generics.rs#L83-L153 Generic argument parsing: https://github.com/rust-lang/rust/blob/17eec1433c69972844dd228b5fe801f218e118c3/compiler/rustc_parse/src/parser/path.rs#L395-L413 `for` argument parsing: https://github.com/rust-lang/rust/blob/206ee1eea3467fd1d7f1efdbeafe27880897bb2c/compiler/rustc_parse/src/parser/ty.rs#L724-L736 Fixes rust-lang#785
This adds a new lint to fix #42181. For consistency and to avoid code duplication, I also moved the existing "bounds in type aliases are ignored" here.
Questions to the reviewer:
span_warn!seems deprecated (after this patch, it has exactly one user left!).forin the surface syntax? I covered function types (for<'a> fn(...)), generic traits (for <'a> Fn(...), can appear both as bounds as as trait objects) and bounds (for<'a> F: ...).Cc @eddyb