Detect multiple crate versions on method not found by estebank · Pull Request #128786 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation20 Commits6 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

estebank

When a type comes indirectly from one crate version but the imported trait comes from a separate crate version, the called method won't be found. We now show additional context:

error[E0599]: no method named `foo` found for struct `dep_2_reexport::Type` in the current scope
 --> multiple-dep-versions.rs:8:10
  |
8 |     Type.foo();
  |          ^^^ method not found in `Type`
  |
note: there are multiple different versions of crate `dependency` in the dependency graph
 --> multiple-dep-versions.rs:4:32
  |
4 | use dependency::{do_something, Trait};
  |                                ^^^^^ `dependency` imported here doesn't correspond to the right crate version
  |
 ::: ~/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-1.rs:4:1
  |
4 | pub trait Trait {
  | --------------- this is the trait that was imported
  |
 ::: ~/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-2.rs:4:1
  |
4 | pub trait Trait {
  | --------------- this is the trait that is needed
5 |     fn foo(&self);
  |        --- the method is available for `dep_2_reexport::Type` here

Fix #128569, fix #110926, fix #109161, fix #81659, fix #51458, fix #32611. Follow up to #124944.

@rustbot

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make

Area: port run-make Makefiles to rmake.rs

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Aug 7, 2024

@rustbot

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

compiler-errors

estebank

Comment on lines 4061 to 4062

if let Some(map) = self.tcx.in_scope_traits_map(owner) {
for trait_candidate in map.to_sorted_stable_ord().into_iter().flat_map(|v

Choose a reason for hiding this comment

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

This is the thing I'm slightly uneasy about: for every error of a method not found that triggers the prior conditions, we'll be doing a linear scan of all traits in scope. Don't have a good sense of whether that will be a significant slow-down for compiles with errors (not a good way of testing that difference).

Choose a reason for hiding this comment

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

Please take some time to fuzz this. Would it be possible to trigger this in some normal session by removing some imports or upgrading a crate to a newer version (where the trait method is removed in the newer version and the older version is still imported by some transitive dependencies)?

It's very possible that some large project trying to refactor something will hit this if this is a significant slowdown.

Choose a reason for hiding this comment

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

I think putting this behind a query that allocates a HashMap<crate name, Vec<trait DefId>> and returns it would reduce the amortized cost significantly and eliminate the risk of causing trouble.

Choose a reason for hiding this comment

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

Ok, I changed it to use in_scope_traits instead of in_scope_traits_map, which will be much cheaper (at the cost of only looking at traits in the current scope, which is fine).

Choose a reason for hiding this comment

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

@fee1-dead given the current approach (where we're looking at in-scope traits for a specific scope), which is something we're already doing for every method resolution, I feel more comfortable with it. I'm also not quite sure how to synthesize a big enough corpus of "wrong crate" to properly stress this.

@estebank

I think after landing this PR the only error that still needs to check for multiple crate versions is E0277. E0308 already has a check (even though it isn't great), and this addresses E0599 for every case I could get my hands on.

Edit: identified the reason most of the E0277 weren't triggering and fixed it in #128849. I think the only place left where we could do more is E0308.

fee1-dead

Comment on lines 4061 to 4062

if let Some(map) = self.tcx.in_scope_traits_map(owner) {
for trait_candidate in map.to_sorted_stable_ord().into_iter().flat_map(|v

Choose a reason for hiding this comment

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

Please take some time to fuzz this. Would it be possible to trigger this in some normal session by removing some imports or upgrading a crate to a newer version (where the trait method is removed in the newer version and the older version is still imported by some transitive dependencies)?

It's very possible that some large project trying to refactor something will hit this if this is a significant slowdown.

@rustbot rustbot 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

Aug 9, 2024

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@estebank

When a type comes indirectly from one crate version but the imported trait comes from a separate crate version, the called method won't be found. We now show additional context:

error[E0599]: no method named `foo` found for struct `dep_2_reexport::Type` in the current scope
 --> multiple-dep-versions.rs:8:10
  |
8 |     Type.foo();
  |          ^^^ method not found in `Type`
  |
note: you have multiple different versions of crate `dependency` in your dependency graph
 --> multiple-dep-versions.rs:4:32
  |
4 | use dependency::{do_something, Trait};
  |                                ^^^^^ `dependency` imported here doesn't correspond to the right crate version
  |
 ::: ~/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-1.rs:4:1
  |
4 | pub trait Trait {
  | --------------- this is the trait that was imported
  |
 ::: ~/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-2.rs:4:1
  |
4 | pub trait Trait {
  | --------------- this is the trait that is needed
5 |     fn foo(&self);
  |        --- the method is available for `dep_2_reexport::Type` here

@estebank

@estebank

When encountering the following, mention the precense of conflicting crates:

error[E0599]: no function or associated item named `get_decoded` found for struct `HpkeConfig` in the current scope
   --> src/main.rs:7:17
    |
7   |     HpkeConfig::get_decoded(&foo);
    |                 ^^^^^^^^^^^ function or associated item not found in `HpkeConfig`
    |
note: if you're trying to build a new `HpkeConfig`, consider using `HpkeConfig::new` which returns `HpkeConfig`
   --> ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/janus_messages-0.3.1/src/lib.rs:908:5
    |
908 | /     pub fn new(
909 | |         id: HpkeConfigId,
910 | |         kem_id: HpkeKemId,
911 | |         kdf_id: HpkeKdfId,
912 | |         aead_id: HpkeAeadId,
913 | |         public_key: HpkePublicKey,
914 | |     ) -> HpkeConfig {
    | |___________________^
note: there are multiple different versions of crate `prio` in the dependency graph
   --> src/main.rs:1:5
    |
1   | use prio::codec::Decode;
    |     ^^^^^^^^^^^^^^^^^^^ `prio` imported here doesn't correspond to the right crate version
    |
   ::: ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/prio-0.9.1/src/codec.rs:35:1
    |
35  | pub trait Decode: Sized {
    | ----------------------- this is the trait that was imported
    |
   ::: ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/prio-0.10.3/src/codec.rs:35:1
    |
35  | pub trait Decode: Sized {
    | ----------------------- this is the trait that is needed
...
43  |     fn get_decoded(bytes: &[u8]) -> Result<Self, CodecError> {
    |     -------------------------------------------------------- the method is available for `HpkeConfig` here
help: there is an associated function `decode` with a similar name
    |
7   |     HpkeConfig::decode(&foo);
    |                 ~~~~~~

@estebank

As per the case presented in rust-lang#128569, we should be showing the extra info even if auto-deref is involved.

@estebank

Make checking slightly cheaper (by restricting to the right item only).

Add tests.

@estebank

@estebank estebank 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

Aug 12, 2024

fee1-dead

Choose a reason for hiding this comment

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

Thanks!

@fee1-dead

@bors

📌 Commit 110b19b has been approved by fee1-dead

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

label

Aug 17, 2024

tgross35 added a commit to tgross35/rust that referenced this pull request

Aug 17, 2024

@tgross35

…r=fee1-dead

Detect multiple crate versions on method not found

When a type comes indirectly from one crate version but the imported trait comes from a separate crate version, the called method won't be found. We now show additional context:

error[E0599]: no method named `foo` found for struct `dep_2_reexport::Type` in the current scope
 --> multiple-dep-versions.rs:8:10
  |
8 |     Type.foo();
  |          ^^^ method not found in `Type`
  |
note: there are multiple different versions of crate `dependency` in the dependency graph
 --> multiple-dep-versions.rs:4:32
  |
4 | use dependency::{do_something, Trait};
  |                                ^^^^^ `dependency` imported here doesn't correspond to the right crate version
  |
 ::: ~/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-1.rs:4:1
  |
4 | pub trait Trait {
  | --------------- this is the trait that was imported
  |
 ::: ~/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-2.rs:4:1
  |
4 | pub trait Trait {
  | --------------- this is the trait that is needed
5 |     fn foo(&self);
  |        --- the method is available for `dep_2_reexport::Type` here

Fix rust-lang#128569, fix rust-lang#110926, fix rust-lang#109161, fix rust-lang#81659, fix rust-lang#51458, fix rust-lang#32611. Follow up to rust-lang#124944.

tgross35 added a commit to tgross35/rust that referenced this pull request

Aug 17, 2024

@tgross35

…r=fee1-dead

Detect multiple crate versions on method not found

When a type comes indirectly from one crate version but the imported trait comes from a separate crate version, the called method won't be found. We now show additional context:

error[E0599]: no method named `foo` found for struct `dep_2_reexport::Type` in the current scope
 --> multiple-dep-versions.rs:8:10
  |
8 |     Type.foo();
  |          ^^^ method not found in `Type`
  |
note: there are multiple different versions of crate `dependency` in the dependency graph
 --> multiple-dep-versions.rs:4:32
  |
4 | use dependency::{do_something, Trait};
  |                                ^^^^^ `dependency` imported here doesn't correspond to the right crate version
  |
 ::: ~/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-1.rs:4:1
  |
4 | pub trait Trait {
  | --------------- this is the trait that was imported
  |
 ::: ~/rust/build/x86_64-unknown-linux-gnu/test/run-make/crate-loading/rmake_out/multiple-dep-versions-2.rs:4:1
  |
4 | pub trait Trait {
  | --------------- this is the trait that is needed
5 |     fn foo(&self);
  |        --- the method is available for `dep_2_reexport::Type` here

Fix rust-lang#128569, fix rust-lang#110926, fix rust-lang#109161, fix rust-lang#81659, fix rust-lang#51458, fix rust-lang#32611. Follow up to rust-lang#124944.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Aug 17, 2024

@bors

Rollup of 9 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

@bors

@bors

This was referenced

Aug 17, 2024

@rust-timer

Finished benchmarking commit (9b318d2): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌ (primary) 2.1% [2.1%, 2.1%] 1
Regressions ❌ (secondary) - - 0
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) - - 0
All ❌✅ (primary) 2.1% [2.1%, 2.1%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 749.433s -> 750.64s (0.16%)
Artifact size: 339.08 MiB -> 339.13 MiB (0.01%)

estebank added a commit to estebank/rust that referenced this pull request

Aug 18, 2024

@estebank

estebank added a commit to estebank/rust that referenced this pull request

Aug 21, 2024

@estebank

tgross35 added a commit to tgross35/rust that referenced this pull request

Aug 21, 2024

@tgross35

…errors

Do not ICE on non-ADT rcvr type when looking for crate version collision

When looking for multiple versions of the same crate, do not blindly construct the receiver type.

Follow up to rust-lang#128786. Fix rust-lang#129205.

compiler-errors pushed a commit to estebank/rust that referenced this pull request

Aug 26, 2024

@estebank @compiler-errors

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Aug 26, 2024

@matthiaskrgr

…errors

Do not ICE on non-ADT rcvr type when looking for crate version collision

When looking for multiple versions of the same crate, do not blindly construct the receiver type.

Follow up to rust-lang#128786. Fixes rust-lang#129205 Fixes rust-lang#129216

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Aug 26, 2024

@matthiaskrgr

…errors

Do not ICE on non-ADT rcvr type when looking for crate version collision

When looking for multiple versions of the same crate, do not blindly construct the receiver type.

Follow up to rust-lang#128786. Fixes rust-lang#129205 Fixes rust-lang#129216

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Aug 27, 2024

@rust-timer

Rollup merge of rust-lang#129250 - estebank:issue-129205, r=compiler-errors

Do not ICE on non-ADT rcvr type when looking for crate version collision

When looking for multiple versions of the same crate, do not blindly construct the receiver type.

Follow up to rust-lang#128786. Fixes rust-lang#129205 Fixes rust-lang#129216

bvanjoi pushed a commit to bvanjoi/rust that referenced this pull request

Aug 28, 2024

@estebank @bvanjoi

Labels

A-diagnostics

Area: Messages for errors, warnings, and lints

A-run-make

Area: port run-make Makefiles to rmake.rs

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.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.