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

Jump to bottom

Include rmeta candidates in "multiple matching crates" error #89587

Merged
merged 6 commits into from
Oct 13, 2021
Merged

Include rmeta candidates in "multiple matching crates" error #89587

merged 6 commits into from
Oct 13, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Oct 6, 2021 •
edited
Loading

Only dylib and rlib candidates were included in the error. I think the
reason is that at the time this error was originally implemented, rmeta
crate sources were represented different from dylib and rlib sources.
I wrote up more detailed analysis in this comment.

The new version of the code is also a bit easier to read and should be
more robust to future changes since it uses CrateSources::paths().

I also changed the code to sort the candidates to make the output deterministic;
added full stderr tests for the error; and added a long error code explanation.

cc @Mark-Simulacrum @jyn514

@camelid camelid added A-diagnostics Area: Messages for errors, warnings, and lints A-metadata Area: Crate metadata labels Oct 6, 2021
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2021
@camelid camelid mentioned this pull request Oct 6, 2021
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
camelid
camelid commented Oct 6, 2021
View reviewed changes
compiler/rustc_metadata/src/locator.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/locator.rs
crate_name,
paths.next().unwrap().display()
);
let padding = 8 + crate_name.len();
Copy link
Member Author

Choose a reason for hiding this comment

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

This padding doesn't seem correct to me. It's the same as before, but crate ` is 7 chars and `: is 3 chars (backtick, colon, space). Together, that makes 10 characters, not 8. Additionally, I don't think the format specifier {:>padding$} is correct for paths shorter than padding.

Should I change it to push_str the padding, and then push the path, rather than using format specifiers? I think that would be more correct.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov self-assigned this Oct 6, 2021
@oli-obk oli-obk removed their assignment Oct 6, 2021
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2021
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 6, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Copy link
Member Author

camelid commented Oct 6, 2021

Looks like tests failed for two reasons: (1) #89587 (comment) and (2) inconsistent ordering of candidates. For (1), I just need to add normalize-stderr-test. For (2), I'm thinking I could just sort the candidate paths. Would that be reasonable?

@camelid
Copy link
Member Author

camelid commented Oct 6, 2021

(1) should be fixed now.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Copy link
Member Author

camelid commented Oct 6, 2021

Ok, (2) should be fixed as well now.

@camelid
Copy link
Member Author

camelid commented Oct 6, 2021 •
edited
Loading

I also reordered the commits so that the tests are added before the rmeta candidates fix so you can see the diff.

@camelid camelid mentioned this pull request Oct 6, 2021
Closed
100 tasks
@bors
Copy link
Contributor

bors commented Oct 11, 2021

📌 Commit 359a3fdfc2d7f425c29155959643f7a5d46d732e has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented Oct 11, 2021

⌛ Testing commit 359a3fdfc2d7f425c29155959643f7a5d46d732e with merge 9abf6d9699ae26eec2c5a48bc6e610f6f97cc410...

petrochenkov
petrochenkov reviewed Oct 11, 2021
View reviewed changes
src/test/ui/crate-loading/crateresolve1.rs
// error-pattern:multiple matching crates for `crateresolve1`

// normalize-stderr-test: "\.nll/" -> "/"
// normalize-stderr-test: "\\\?\\" -> ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't this issue appear with other tests printing $TEST_BUILD_DIR paths?
Do verbatim paths (\\?) appear because the path is fs::canonicalize-d in this case?
This normalization looks like a compiletest's job (src\tools\compiletest\src\runtest.rs -> fn normalize_output).

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it's a bit odd, but I figured it's because the crate loader is showing absolute paths. I don't have time to look into whether this is a compiletest issue unfortunately.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 11, 2021
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2021
@bors

This comment has been minimized.

Sign in to view
@camelid
Sort candidate libraries by source path in error
6792c6a 
This makes the error output deterministic and thus testable.
@camelid
Test crate loading error stderr
642a43a 
And remove E0464 from test-exemption list, since it now has a full test.
@camelid
Add test for showing rmeta candidates in error
ad49cb6
@camelid
Include rmeta candidates in "multiple matching crates" error
2e56b6f 
Only dylib and rlib candidates were included in the error. I think the
reason is that at the time this error was originally implemented, rmeta
crate sources were represented different from dylib and rlib sources.
I wrote up more detailed analysis in [this comment][1].

The new version of the code is also a bit easier to read and should be
more robust to future changes since it uses `CrateSources::paths()`.

[1]: rust-lang#88675 (comment)
@camelid
Add long explanation for E0464
cc6a090 
The test is copied from `src/test/ui/crate-loading/crateresolve1.rs` and
its auxiliary tests. I added it to the `compile_fail` code example check
exemption list since it's hard if not impossible to reproduce this error
in a standalone code example.
@camelid
Copy link
Member Author

camelid commented Oct 12, 2021

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Oct 12, 2021

📌 Commit bf2d2e5 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 12, 2021
@bors
Copy link
Contributor

bors commented Oct 13, 2021

⌛ Testing commit bf2d2e5 with merge 5728bd6...

@bors
Copy link
Contributor

bors commented Oct 13, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 5728bd6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2021
@bors bors merged commit 5728bd6 into rust-lang:master Oct 13, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 13, 2021
@camelid camelid deleted the all-candidates branch October 13, 2021 16:03
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5728bd6): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@pnkfelix pnkfelix mentioned this pull request Dec 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-metadata Area: Crate metadata merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.
10 participants
@camelid @rust-highfive @rust-log-analyzer @petrochenkov @bors @Manishearth @jyn514 @rust-timer @oli-obk @rustbot