-
Notifications
You must be signed in to change notification settings - Fork 13.4k
tests/ui
: A New Order [6/N]
#142132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
tests/ui
: A New Order [6/N]
#142132
Conversation
|
This comment has been minimized.
This comment has been minimized.
btw, i don't like this complex.rs test, it feels like it tests nothing, so if you feels the same way we can remove it i guess |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you leave a comment that points people back to ../cross-crate-method-reexport.rs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not sure how exactly this comment should looks like, because i checked other auxiliary files in this directory on none of this files contains something like this
something like?
Original test is located in `tests/ui/cross-crate/cross-crate-method-reexport.rs`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, "used by" is also fine
//! treated as early bound, similar to associated items, rather than late bound as in manual | ||
//! constructors. | ||
//! | ||
//! Related to issue #30904. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
howso? pretend I am too lazy to go look at that issue (because I am)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess your question is "how this test actually validates the early-bound behavior" so answer is the test proves early-bound behavior by confirming that partial specification fails with the right error message, if these were late-bound, the error messages might be different
it may be not the best way surely but what we have is what we have
(if that's not what your question was about, you could clarify what you meant)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I meant "how does it relate to that issue"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as i concerned (cuz issue is still open and problem was not fixed yet): the test is mainly to make sure the lifetime mechanism works right, which we'll need for a fix later on (since person who created this test said in their pr that issue was actually fixed, but in reality it doesnt, so this test more indirect relation with this issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//! Related to issue #30904. | |
//! May become a non-error if we fix https://github.com/rust-lang/rust/issues/30904 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the errors will still remain after fix, because arguments number does not match the expected one, like here
S::<'static>(&0, &0);
//~^ ERROR struct takes 2 lifetime arguments
so i even think about to remove this entire mention of this issue, because it tests slightly different thing
fun part that we had a test for #30904, but it was removed and i cant find this, maybe it was renamed and fully rewritten but i find it hard to believe, here:
https://github.com/JohnTitor/rust/blob/74d45afbf5473d1b255629e786e074060dcc7ec2/src/test/ui/unboxed-closures/issue-30904.rs
if test was really deleted i guess i can add it, because this still true and what issue was about
let _: for<'a> fn(&'a str) -> Str<'a> = Str;
//~^ ERROR: mismatched types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, hm.
Yes I think removing the mention of the issue if it's that unrelated is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, yeah, it was deleted in this commit
564c78a#diff-85d65712084246fc61f287664eef63b0b25ba0a5c8b69a4a59a9454b6a3ebac4
but then issue was reopened and test was not brought back, i guess i can add it in separate pr
also if you want and agree with changes, i guess, i can squash it so you could r+ |
Yes, I'll sign off on this one. |
okie, i will ping you as soon as ci passed then |
Thanks! @bors r+ rollup |
This comment has been minimized.
This comment has been minimized.
fixed the last error with stderr, not actually sure if it should be readded to bors as new commit |
`tests/ui`: A New Order [6/N] > [!NOTE] > > Intermediate commits are intended to help review, but will be squashed prior to merge. Some `tests/ui/` housekeeping, to trim down number of tests directly under `tests/ui/`. Part of rust-lang#133895. r? `@jieyouxu` auxiliary tag means some changes in realted auxiliary file for test
Rollup of 7 pull requests Successful merges: - #141001 (Make NonZero<char> possible) - #141700 (Atomic intrinsics : use const generic ordering, part 2) - #141993 (Use the in-tree `compiler-builtins` for the sysroot) - #142008 (const-eval error: always say in which item the error occurred) - #142053 (Add new Tier-3 targets: `loongarch32-unknown-none*`) - #142132 (`tests/ui`: A New Order [6/N]) - #142179 (store `target.min_global_align` as an `Align`) r? `@ghost` `@rustbot` modify labels: rollup
`tests/ui`: A New Order [6/N] > [!NOTE] > > Intermediate commits are intended to help review, but will be squashed prior to merge. Some `tests/ui/` housekeeping, to trim down number of tests directly under `tests/ui/`. Part of rust-lang#133895. r? ``@jieyouxu`` auxiliary tag means some changes in realted auxiliary file for test
Rollup of 6 pull requests Successful merges: - #141001 (Make NonZero<char> possible) - #141700 (Atomic intrinsics : use const generic ordering, part 2) - #142008 (const-eval error: always say in which item the error occurred) - #142053 (Add new Tier-3 targets: `loongarch32-unknown-none*`) - #142132 (`tests/ui`: A New Order [6/N]) - #142179 (store `target.min_global_align` as an `Align`) r? `@ghost` `@rustbot` modify labels: rollup
Note
Intermediate commits are intended to help review, but will be squashed prior to merge.
Some
tests/ui/
housekeeping, to trim down number of tests directly undertests/ui/
. Part of #133895.r? @jieyouxu
auxiliary tag means some changes in realted auxiliary file for test