-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add compiler error E0523 long description and test #100599
Add compiler error E0523 long description and test #100599
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jackh726 (or someone else) soon. Please see the contribution instructions for more information. |
src/test/ui/error-codes/E0523.rs
Outdated
/// ``` | ||
/// | ||
|
||
// FIXME: add code to reproduce the above error here |
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'm somewhat stuck here because I'm not sure how to create a small self-contained test that defines and pulls in two different crates with the same name. Is there a way to "stub" out a fake crate for use in a test like this?
(thanks in advance for the help -- I'm relatively new to rust and trying to learn a bit by working through some simple issues)
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.
So, in ui tests, there is a way to mimic extra crates. (I don't remember if 'auxillary' files are separate crates or not.
For the rustc_error_codes
file, I think it's just going to be "faking" it.
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.
there is a way to mimic extra crates
Would you be able to point me to an example or give me a keyword that I can search the repo for?
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.
Note: this PR is still an early draft -- it just adds some notes in the files that need to be changed, and provides a way for me to get some early feedback. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7cf2b6a
to
5037b73
Compare
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.
@jackh726 -- Made a bit more progress, but still having trouble reproducing E0523. I was able to get the auxiliary file thing working though, so that's neat.
I did a bit of sleuthing to figure out where the compiler actually generates this error message. It is configured in compiler/rustc_metadata/src/errors.rs
:
#[derive(Diagnostic)]
#[diag(metadata_symbol_conflicts_others, code = "E0523")]
pub struct SymbolConflictsOthers {
#[primary_span]
pub span: Span,
pub crate_name: Symbol,
}
This is the instruction that tells the compiler to match some set of conditions to an actual error string, but I don't know enough about the compiler to reverse engineer how to reproduce those conditions (or exactly what they mean). Some more sleuthing shows that the actual error message text is produced in compiler/rustc_error_messages/locales/en-US/metadata.ftl
:
metadata_symbol_conflicts_others =
found two different crates with name `{$crate_name}` that are not distinguished by differing `-C metadata`. This will result in symbol conflicts between the two.
Any good ideas on how to reproduce E0523 in the UI test?
src/test/ui/error-codes/E0523.rs
Outdated
// aux-build:e0523_aux_1.rs | ||
// aux-build:e0523_aux_2.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.
These two crates both have the same "crate-name", so I was hoping that including both would trigger E0523.
I've tried various combinations of file names and crate names, but so far no luck hitting E0523. As written here, the compiler cannot find the fifty()
function, which suggests to me that somehoe *_aux1
is overriding *_aux2
or something like that.
src/test/ui/error-codes/E0523.rs
Outdated
// aux-build:e0523_aux_1.rs | ||
// aux-build:e0523_aux_2.rs | ||
|
||
extern crate e0523_conflict; |
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.
This should be ambiguous, as two different crates have this name. Maybe it just accepts the first one that it finds and doesn't see the second?
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #105940) made this pull request unmergeable. Please resolve the merge conflicts. |
@MatthewPeterKelly Friendly ping, how are you going on this PR? Would you like some help? It'd be really nice to get this merged soon so that we can have all error codes documented and finally close #61137! |
It would be great to get some help! I'm stuck on not being able to reproduce the error message. I tried to reverse engineer it from the compiler code (#100599 (review)), but got stuck there too. Any chance that you know what I need to do to make the compiler produce E0523?
Wow - so many checked boxes on that list! Agreed. Let's get this PR merged. |
5f7dd0c
to
2f7e2d9
Compare
@MatthewPeterKelly Right so here are my thoughts:
fn verify_no_symbol_conflicts(&self, root: &CrateRoot) -> Result<(), CrateError> {
// Check for (potential) conflicts with the local crate
if self.sess.local_stable_crate_id() == root.stable_crate_id() {
return Err(CrateError::SymbolConflictsCurrent(root.name()));
}
// Check for conflicts with any crate loaded so far
for (_, other) in self.cstore.iter_crate_data() {
// Same stable crate id but different SVH
if other.stable_crate_id() == root.stable_crate_id() && other.hash() != root.hash() {
return Err(CrateError::SymbolConflictsOthers(root.name()));
}
}
Ok(())
} (
Thanks for the hard work :) |
This comment has been minimized.
This comment has been minimized.
I was wondering the same thing -- all of my attempts produced other error messages. I'll wait to hear back from @compiler-errors about whether E0523 is reachable, and if it isn't follow the steps you outlined above to "deprecate" this error. |
I also can't reproduce this error, but I know a lot less about the crate loading part of the compiler. Regardless, if we are going to remove the error from the codebase, please replace it with a |
Do you think we should go ahead with the "assume that E0523 is no longer emitted" plan (with your suggestion of the |
@MatthewPeterKelly Yup, make it a |
☔ The latest upstream changes (presumably #107220) made this pull request unmergeable. Please resolve the merge conflicts. |
2f7e2d9
to
f087023
Compare
@@ -359,7 +359,11 @@ impl<'a> CrateLoader<'a> { | |||
for (_, other) in self.cstore.iter_crate_data() { | |||
// Same stable crate id but different SVH | |||
if other.stable_crate_id() == root.stable_crate_id() && other.hash() != root.hash() { | |||
return Err(CrateError::SymbolConflictsOthers(root.name())); | |||
span_bug!( | |||
DUMMY_SP, //FIXME: pass in the correct span here |
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.
Any ideas for how to pass in the correct span here?
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.
actually this could probably just be a bug!
, there's no span associated with it.
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.
Thanks -- I swapped in a bug!()
and that worked.
#### Note: this error code is no longer emitted by the compiler. | ||
|
||
Found two different crates that are not distinguished by differing metadata. | ||
Code that previously produced this error will now produce E0464 instead. |
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.
It'd be great if we could have a code example to show some code which previously produced this error here. You can just copy it from the E0523.md file.
Also a small nitpick: when you add a code example, the note explaining that this error has been merged into E0464 could be below the code example (I think that's more in line with how it's normally formatted).
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.
It'd be great if we could have a code example to show some code which previously produced this error here. You can just copy it from the E0523.md file.
Sure. I'm going to assume that you meant the E0464.rs
file (based on an earlier comment), as E0523.md
is this file and E0464.md
doesn't include a code example.
when you add a code example, the note explaining that this error has been merged into E0464
Done! Let me know if I got the formatting right.
f182a7f
to
a91f542
Compare
@Ezrashaw -- I've updated the docs for Does it still make sense to have the |
@MatthewPeterKelly Sorry for the slow reply. It looks great, just one thing: could you move the code examples to be under the top-line text, without the paragraph in between. Other than that, and if @GuillaumeGomez is happy, then it's good to merge. |
One nit: Can you rebase and either squash this into a fewer number of commits, or at least drop that commit fb1ff0f696df1056e32991a6444ee969f93e5fd0 + its revert commit from the history? no need to keep it if it's being reverted later down the stack. It helps to declutter the history imo :) |
a91f542
to
5b6a367
Compare
Ok - I think this is ready. I moved the code example as suggested by @Ezrashaw and did a squash rebase onto |
☔ The latest upstream changes (presumably #107679) made this pull request unmergeable. Please resolve the merge conflicts. |
Adds the extended error documentation for E0523 to indicate that the error is no longer produced by the compiler. Update the E0464 documentation to include example code that produces the error. Remove the error message E0523 from the compiler and replace it with an internal compiler error.
6975cdc
to
2bcd4e2
Compare
Since @compiler-errors already approved the compiler changes, then we can approve the PR. Thanks for working on this! @bors r=compiler-errors,GuillaumeGomez rollup |
…ption-and-test, r=compiler-errors,GuillaumeGomez Add compiler error E0523 long description and test This PR is one step towards addressing: rust-lang#61137.
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#100599 (Add compiler error E0523 long description and test) - rust-lang#107471 (rustdoc: do not include empty default-settings tag in HTML) - rust-lang#107555 (Modify existing bounds if they exist) - rust-lang#107662 (Turn projections into copies in CopyProp.) - rust-lang#107695 (Add test for Future inflating arg size to 3x ) - rust-lang#107700 (Run the tools builder on all PRs) - rust-lang#107706 (Mark 'atomic_mut_ptr' methods const) - rust-lang#107709 (Fix problem noticed in PR106859 with char -> u8 suggestion) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
hey, I managed to hit error E0523 😄
Reproduction steps:
diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs
index 608b4c4d245..0bf7f245518 100644
--- a/src/bootstrap/builder.rs
+++ b/src/bootstrap/builder.rs
@@ -2202,6 +2202,17 @@ pub struct Cargo {
}
impl Cargo {
+ /// NOTE: avoid this in favor of `builder.cargo` where possible.
+ pub fn from_cmd(command: Command, target: TargetSelection) -> Cargo {
+ let rustflags = Rustflags::new(target);
+ Cargo {
+ command,
+ rustdocflags: rustflags.clone(),
+ rustflags,
+ allow_features: String::new(),
+ }
+ }
+
pub fn rustdocflag(&mut self, arg: &str) -> &mut Cargo {
self.rustdocflags.arg(arg);
self
diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs
index 9540adea189..7c7e6100424 100644
--- a/src/bootstrap/test.rs
+++ b/src/bootstrap/test.rs
@@ -2035,7 +2035,7 @@ fn run(self, builder: &Builder<'_>) {
///
/// Returns whether the test succeeded.
fn run_cargo_test(
- cargo: impl Into<Command>,
+ cargo: crate::builder::Cargo,
libtest_args: &[&str],
crates: &[Interned<String>],
primary_crate: &str,
@@ -2051,7 +2051,7 @@ fn run_cargo_test(
/// Given a `cargo test` subcommand, pass it the appropriate test flags given a `builder`.
fn prepare_cargo_test(
- cargo: impl Into<Command>,
+ mut cargo: crate::builder::Cargo,
libtest_args: &[&str],
crates: &[Interned<String>],
primary_crate: &str,
@@ -2059,8 +2059,6 @@ fn prepare_cargo_test(
target: TargetSelection,
builder: &Builder<'_>,
) -> Command {
- let mut cargo = cargo.into();
-
// Pass in some standard flags then iterate over the graph we've discovered
// in `cargo metadata` with the maps above and figure out what `-p`
// arguments need to get passed.
@@ -2078,6 +2076,9 @@ fn prepare_cargo_test(
.unwrap_or_else(|| panic!("missing crate {primary_crate}"));
if krate.has_lib {
cargo.arg("--lib");
+ if crates.iter().any(|c| c != "panic_abort") {
+ cargo.rustflag("-Zpanic_abort_tests");
+ }
}
cargo.args(&["--bins", "--examples", "--tests", "--benches"]);
}
@@ -2118,7 +2119,7 @@ fn prepare_cargo_test(
);
}
- cargo
+ cargo.into()
}
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -2518,7 +2519,7 @@ fn run(self, builder: &Builder<'_>) {
}
// rustbuild tests are racy on directory creation so just run them one at a time.
// Since there's not many this shouldn't be a problem.
- run_cargo_test(cmd, &["--test-threads=1"], &[], "bootstrap", compiler, host, builder);
+ run_cargo_test(crate::builder::Cargo::from_cmd(cmd, host), &["--test-threads=1"], &[], "bootstrap", compiler, host, builder);
}
fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
|
If it helps, I see |
Some relevant debug logging:
From glancing at the error description, maybe this is a cargo bug and |
@jyn514 Could you open an issue or a Zulip thread please (and ping me)? IIRC, the author did a lot of searching for cases and they had all moved to |
This PR is one step towards addressing: #61137.