Skip to content
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.

Sign up for GitHub

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

Disable some flags on issue-71363 test #97750

Closed
wants to merge 1 commit into from
Closed

Disable some flags on issue-71363 test #97750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/test/ui/span/issue-71363.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// compile-flags: -Z simulate-remapped-rust-src-base=/rustc/xyz -Z ui-testing=no
// only-x86_64-unknown-linux-gnu
//---^ Limiting target as the above unstable flags don't play well on some environment.
// ignore-test
// FIXME(#97682):
// The `-Z simulate-remapped-rust-src-base=/rustc/xyz -Z ui-testing=no` flags
// don't work well and make UI test fail on some env.
// Once it starts to work fine, we could re-enable them here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test isn't disabled, right? I think we probably want to mark it as ignore for now, so that we can more easily tell that the test is not actually testing the right behavior and needs fixing.

Unless these flags are added for unrelated reasons? I don't see an obvious mention of them in the original issue (#71363)....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this PR disabled the flags, but didn't disable the test itself.

This test isn't disabled, right? I think we probably want to mark it as ignore for now, so that we can more easily tell that the test is not actually testing the right behavior and needs fixing.

Unless these flags are added for unrelated reasons? I don't see an obvious mention of them in the original issue (#71363)....

I just copy-pasted them from the previous PR so I cannot explain it correctly, I guess this comment has some context?: #71363 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added // ignore-test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the flag would indeed make the test incorrect.

This test ensures that when a file (in this case library/std/src/error.rs) is not included in the error message it will not affect the indentation of the error message itself (due to the line number length).

The test tried to rely on -Z simulate-remapped-rust-src-base to exclude the library/std/src/error.rs file from the error message, but that flag is buggy. The flag simulated remap-debuginfo = true only partially, and crucially didn't simulate mapping a remapped path to a local path when possible. Because of this, when remap-debuginfo = false the file wasn't mapped to a local path (excluding it from the error), while when remap-debuginfo = true it was mapped to a local file, causing the test to fail.

I opened #97786 to fix the behavior of that flag, but it also makes the test incorrect (as it always maps the remapped path to a local file).

If we'll want to fix this test we need to either find a path that will not be included in the error message regardless of remap-debuginfo, or add a new -Z flag to skip this piece of code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, does your PR fix the different behavior of this test described in #97682? If so, then we could close this and reopen #71363. This PR's motivation was to make UI test pass on @japaric's env and ignoring would be sufficient for that purpose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'm also working on a PR to try and add a -Z flag to make the test work as expected.


struct MyError;
impl std::error::Error for MyError {}
Expand Down
42 changes: 25 additions & 17 deletions src/test/ui/span/issue-71363.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,34 @@
error[E0277]: `MyError` doesn't implement `std::fmt::Display`
--> $DIR/issue-71363.rs:6:6
|
6 | impl std::error::Error for MyError {}
| ^^^^^^^^^^^^^^^^^ `MyError` cannot be formatted with the default formatter
|
= help: the trait `std::fmt::Display` is not implemented for `MyError`
= note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
--> $DIR/issue-71363.rs:7:6
|
LL | impl std::error::Error for MyError {}
| ^^^^^^^^^^^^^^^^^ `MyError` cannot be formatted with the default formatter
|
= help: the trait `std::fmt::Display` is not implemented for `MyError`
= note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
note: required by a bound in `std::error::Error`
--> $SRC_DIR/std/src/error.rs:LL:COL
|
LL | pub trait Error: Debug + Display {
| ^^^^^^^ required by this bound in `std::error::Error`

error[E0277]: `MyError` doesn't implement `Debug`
--> $DIR/issue-71363.rs:6:6
|
6 | impl std::error::Error for MyError {}
| ^^^^^^^^^^^^^^^^^ `MyError` cannot be formatted using `{:?}`
|
= help: the trait `Debug` is not implemented for `MyError`
= note: add `#[derive(Debug)]` to `MyError` or manually `impl Debug for MyError`
--> $DIR/issue-71363.rs:7:6
|
LL | impl std::error::Error for MyError {}
| ^^^^^^^^^^^^^^^^^ `MyError` cannot be formatted using `{:?}`
|
= help: the trait `Debug` is not implemented for `MyError`
= note: add `#[derive(Debug)]` to `MyError` or manually `impl Debug for MyError`
note: required by a bound in `std::error::Error`
--> $SRC_DIR/std/src/error.rs:LL:COL
|
LL | pub trait Error: Debug + Display {
| ^^^^^ required by this bound in `std::error::Error`
help: consider annotating `MyError` with `#[derive(Debug)]`
|
5 | #[derive(Debug)]
|
|
LL | #[derive(Debug)]
|

error: aborting due to 2 previous errors

Expand Down