Note different versions of same crate when absolute paths of different types match. #42826
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
alexcrichton
added
the
S-waiting-on-review
|
friendly ping @nikomatsakis, pinging you in IRC too! |
|
@bors r+ |
|
📌 Commit 8205c34 has been approved by |
|
@bors rollup |
…paths, r=arielb1 Note different versions of same crate when absolute paths of different types match. The current check to address rust-lang#22750 only works when the paths of the mismatched types relative to the current crate are equal, but this does not always work if one of the types is only included through an indirect dependency. If reexports are involved, the indirectly included path can e.g. [contain private modules](rust-lang#22750 (comment)). This PR takes care of these cases by also comparing the *absolute* path, which is equal if the type hasn't moved in the module hierarchy between versions. A more coarse check would be to compare only the crate names instead of full paths, but that might lead to too many false positives. Additionally, I believe it would be helpful to show where the differing crates came from, i.e. the information in `rustc::middle::cstore::CrateSource`, but I'm not sure yet how to nicely display all of that, so I'm leaving it to a future PR.
…paths, r=arielb1 Note different versions of same crate when absolute paths of different types match. The current check to address rust-lang#22750 only works when the paths of the mismatched types relative to the current crate are equal, but this does not always work if one of the types is only included through an indirect dependency. If reexports are involved, the indirectly included path can e.g. [contain private modules](rust-lang#22750 (comment)). This PR takes care of these cases by also comparing the *absolute* path, which is equal if the type hasn't moved in the module hierarchy between versions. A more coarse check would be to compare only the crate names instead of full paths, but that might lead to too many false positives. Additionally, I believe it would be helpful to show where the differing crates came from, i.e. the information in `rustc::middle::cstore::CrateSource`, but I'm not sure yet how to nicely display all of that, so I'm leaving it to a future PR.
| $(RUSTC) --crate-type=rlib crateB.rs --extern crateA=$(TMPDIR)/libcrateA-1.rlib | ||
| # make crateC depend on version 2 of crateA | ||
| $(RUSTC) crateC.rs --extern crateA=$(TMPDIR)/libcrateA-2.rlib 2>&1 | \ | ||
| grep -z \ |
There was a problem hiding this comment.
grep -z does not exist on macOS. See #42961 (comment).
There was a problem hiding this comment.
Is there a blessed, cross-platform way to check the output of some command if it is expected to span multiple lines? Would | tr -d '\r\n' | grep ... work?
There was a problem hiding this comment.
@Yorwba This "works" as in all lines are collapsed into the single line.
Is it possible to rewrite this as a UI test? Something similar to src/test/ui/cross-crate-macro-backtrace.
There was a problem hiding this comment.
The aux-build system is not powerful enough to set up the necessary crate conflicts. Basically, rustc needs to be passed --extern crateA=... for two different values of crateA when building two crates that end up linked together. (This actually happens when using cargo.)
I considered using compile-flags to set this up, but there is no way to get the necessary paths, since they depend on the current target platform and compilation stage. It ended up being easier just to write a makefile for this.
As for tr -d '\r\n' collapsing everything into a single line, this is mostly fine, since the line endings are not as important as the error being reported at all. (My regex for grep -z just skips over the intermediate parts anyway.) So if tr -d '\r\n' can be used on all platforms that makefile tests need to work on, I think this would be the best solution.
There was a problem hiding this comment.
Unless of course there is some way to reuse parts of the UI test machinery from the makefile.
|
@bors r- |
|
The test should now also work on macOS by relying on the standard |
|
r? @arielb1 |
|
friendly ping @arielb1! Note that travis has completed, despite github saying it hasn't... |
|
@bors r+ |
|
📌 Commit 21bb60a has been approved by |
arielb1
added
S-waiting-on-bors
…paths, r=arielb1 Note different versions of same crate when absolute paths of different types match. The current check to address rust-lang#22750 only works when the paths of the mismatched types relative to the current crate are equal, but this does not always work if one of the types is only included through an indirect dependency. If reexports are involved, the indirectly included path can e.g. [contain private modules](rust-lang#22750 (comment)). This PR takes care of these cases by also comparing the *absolute* path, which is equal if the type hasn't moved in the module hierarchy between versions. A more coarse check would be to compare only the crate names instead of full paths, but that might lead to too many false positives. Additionally, I believe it would be helpful to show where the differing crates came from, i.e. the information in `rustc::middle::cstore::CrateSource`, but I'm not sure yet how to nicely display all of that, so I'm leaving it to a future PR.
The logic was added in rust-lang#42826. The test for it (`run-make/type-mismatch-same-crate-name`) passes when just using the macros to control pretty printing. Note that the test does fail unless both of the macros are used, indicating that we're doing something right.
The logic was added in rust-lang#42826. The test for it (`run-make/type-mismatch-same-crate-name`) passes when just using the macros to control pretty printing. Note that the test does fail unless both of the macros are used, indicating that we're doing something right.
The logic was added in rust-lang#42826. The tests for it (`run-make/type-mismatch-same-crate-name` and `ui/type/type/type-mismatch-same-crate-name.rs) pass when being replaced by this, which makes sense, as `no_trimmed!(no_visible!())` should be equivalent to the custom absolute printer.
The current check to address #22750 only works when the paths of the mismatched types relative to the current crate are equal, but this does not always work if one of the types is only included through an indirect dependency. If reexports are involved, the indirectly included path can e.g. contain private modules.
This PR takes care of these cases by also comparing the absolute path, which is equal if the type hasn't moved in the module hierarchy between versions. A more coarse check would be to compare only the crate names instead of full paths, but that might lead to too many false positives.
Additionally, I believe it would be helpful to show where the differing crates came from, i.e. the information in
rustc::middle::cstore::CrateSource, but I'm not sure yet how to nicely display all of that, so I'm leaving it to a future PR.