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.

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

Make required dependency as future an error, remove RcList #6860

Merged
merged 9 commits into from Apr 25, 2019

Conversation

Projects
None yet
4 participants
@Eh2406
Copy link
Contributor

commented Apr 17, 2019

This makes it an error to set a feature on a dependency if it is the name of a required dependency not a feature. This has been a warning for a long time. The resolver will backtrack to find a version that works, if one is available. So I think it is safe to make the change.

If you need to make a optional dependency into a required dependency without a semver breaking change this can be done with rename dependencies.

The result is that we can remove 3 data structures that get cloned once per tick of the resolver.

@rust-highfive

This comment has been minimized.

Copy link

commented Apr 17, 2019

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@Eh2406 Eh2406 force-pushed the Eh2406:no-RcList branch from 3dff5ba to 8cd9b0c Apr 18, 2019

@alexcrichton
Copy link
Member

left a comment

Looks reasonable to me! I'm a little worried in a few places about reconstructing resolver state which may accidentally produce subtle differences with overriding, so I think it may be worthwhile to add a few more asserts here and there to ensure everything is as we expect and we're not accidentally overriding something.

Show resolved Hide resolved src/cargo/core/resolver/context.rs Outdated
Show resolved Hide resolved src/cargo/core/resolver/types.rs Outdated
Show resolved Hide resolved src/cargo/core/resolver/context.rs Outdated
Show resolved Hide resolved src/cargo/core/resolver/mod.rs Outdated
Show resolved Hide resolved src/cargo/core/resolver/mod.rs Outdated
Show resolved Hide resolved src/cargo/core/resolver/mod.rs Outdated
uses_default_features: dep.uses_default_features(),
};
for (dep, features) in new_cx
.resolve_features(None, &summery, &method, config)

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 19, 2019

Member

Would it be possible to perhaps avoid re-calling resolve_features here? I found this somewhat complicated to follow, and figured it might be simpler if we simply walk over the graph after resolution, check the list of activated features for each package, and then present a warning if the feature enables a required dependency.

We may produce a slightly worse error message since we don't have why a feature is activated, but that seems reasonable given the age of this warning perhaps?

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Apr 19, 2019

Author Contributor

The original commit f0f8565, did that. It was a lot cleaner. But it did not work. resolve_features is only adds a canonicalized feature to cx.resolve_features. So I did not find a way to reconstruct the warnings from that. (The hard cases are "invalid9", "dep_feature_in_cmd_line") Suggestions are welcome.

I also don't think we can extend that strategy to only the features transitively enabled by some deps. But I would love to be proven wrong.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 22, 2019

Member

I wonder if we could build up a side table or something like that during resolution to facilitate this strategy of emitting warnings? I suppose that's sort of like what happens today, but I think it'd be best to decouple the warning emission as much as we can from the exact state of the resolver today, because it may make future changes to resolve_features more complicated in the future

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Apr 22, 2019

Author Contributor

True, this new code that redos the work of the resolver is going to be a pane to keep in sink.

I gave up on making it work, and the future opportunities of filtering dependencies, and just made this case a resolver bactrackable error.

self.activations
.values()
.for_each(|(r, _)| graph.add(r.package_id()));
for i in self.parents.iter() {
graph.add(*i);
for (o, e) in self.parents.edges(i) {
*graph.link(*o, *i) = e.to_vec();
let old_link = graph.link(*o, *i);
debug_assert!(old_link.is_empty());

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 22, 2019

Member

Oh FWIW I'd be fine with just a plain assert!, I think we should only use debug_assert if there's a runtime issue

@Eh2406 Eh2406 force-pushed the Eh2406:no-RcList branch from 8799362 to a473716 Apr 22, 2019

@Eh2406 Eh2406 changed the title No RcList Make required dependency as future an error, remove RcList Apr 22, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@bors: r+

That also works I think :)

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

📌 Commit 35ff555 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

⌛️ Testing commit 35ff555 with merge 66817c5...

bors added a commit that referenced this pull request Apr 23, 2019

Auto merge of #6860 - Eh2406:no-RcList, r=alexcrichton
Make required dependency as future an error, remove RcList

This makes it an error to set a feature on a dependency if it is the name of a required dependency not a feature. This has been a warning for a long time. The resolver will backtrack to find a version that works, if one is available. So I think it is safe to make the change.

If you need to make a optional dependency into a required dependency without a semver breaking change this can be done with rename dependencies.

The result is that we can remove 3 data structures that get cloned once per tick of the resolver.
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

💔 Test failed - checks-travis

@Eh2406 Eh2406 added the relnotes label Apr 24, 2019

@Eh2406 Eh2406 closed this Apr 24, 2019

@Eh2406 Eh2406 deleted the Eh2406:no-RcList branch Apr 24, 2019

@Eh2406 Eh2406 restored the Eh2406:no-RcList branch Apr 24, 2019

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

I missed that this had not merged, sorry.

@Eh2406 Eh2406 reopened this Apr 24, 2019

@Eh2406 Eh2406 referenced this pull request Apr 24, 2019

Merged

Caching the dependencies #6853

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

📌 Commit 35ff555 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

⌛️ Testing commit 35ff555 with merge a507ae4...

bors added a commit that referenced this pull request Apr 25, 2019

Auto merge of #6860 - Eh2406:no-RcList, r=alexcrichton
Make required dependency as future an error, remove RcList

This makes it an error to set a feature on a dependency if it is the name of a required dependency not a feature. This has been a warning for a long time. The resolver will backtrack to find a version that works, if one is available. So I think it is safe to make the change.

If you need to make a optional dependency into a required dependency without a semver breaking change this can be done with rename dependencies.

The result is that we can remove 3 data structures that get cloned once per tick of the resolver.
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing a507ae4 to master...

@bors bors merged commit 35ff555 into rust-lang:master Apr 25, 2019

2 of 3 checks passed

Travis CI - Pull Request Build Errored
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@Eh2406 Eh2406 deleted the Eh2406:no-RcList branch Apr 25, 2019

@Eh2406 Eh2406 referenced this pull request May 15, 2019

Merged

Remove Candidate #6946

bors added a commit that referenced this pull request May 15, 2019

Auto merge of #6946 - Eh2406:remove_candidate, r=alexcrichton
Remove Candidate

`Candidate` was a type to record the possibility that a package was replaced using the replacements feature. However there were only two places that cared about this possibility. One was switched to used a lookup table in #6860. This PR switches the other to use that table as well. Making the entire `Candidate` type unnecessary.

The main benefit of this change is a reduction in the cognitive complexity.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jul 5, 2019

he
Update rust to version 1.36.0
Pkgsrc changes:
 * NetBSD/sparc64 disabling of "packed" removed ("packed" removed upstream)
 * Adapt src_libstd_build.rs patch, update sed'ing of .cargo-checksum.json

Build verified on NetBSD 8.0/amd64.

Upstream changes:

Version 1.36.0 (2019-07-04)
==========================

Language
--------
- [Non-Lexical Lifetimes are now enabled on the 2015 edition.][59114]
- [The order of traits in trait objects no longer affects the semantics of that
  object.][59445] e.g. `dyn Send + fmt::Debug` is now equivalent to
  `dyn fmt::Debug + Send`, where this was previously not the case.

Libraries
---------
- [`HashMap`'s implementation has been replaced with `hashbrown::HashMap` implem
entation.][58623]
- [`TryFromSliceError` now implements `From<Infallible>`.][60318]
- [`mem::needs_drop` is now available as a const fn.][60364]
- [`alloc::Layout::from_size_align_unchecked` is now available as a const fn.][6
0370]
- [`String` now implements `BorrowMut<str>`.][60404]
- [`io::Cursor` now implements `Default`.][60234]
- [Both `NonNull::{dangling, cast}` are now const fns.][60244]
- [The `alloc` crate is now stable.][59675] `alloc` allows you to use a subset
  of `std` (e.g. `Vec`, `Box`, `Arc`) in `#![no_std]` environments if the
  environment has access to heap memory allocation.
- [`String` now implements `From<&String>`.][59825]
- [You can now pass multiple arguments to the `dbg!` macro.][59826] `dbg!` will
  return a tuple of each argument when there is multiple arguments.
- [`Result::{is_err, is_ok}` are now `#[must_use]` and will produce a warning if
  not used.][59648]

Stabilized APIs
---------------
- [`VecDeque::rotate_left`]
- [`VecDeque::rotate_right`]
- [`Iterator::copied`]
- [`io::IoSlice`]
- [`io::IoSliceMut`]
- [`Read::read_vectored`]
- [`Write::write_vectored`]
- [`str::as_mut_ptr`]
- [`mem::MaybeUninit`]
- [`pointer::align_offset`]
- [`future::Future`]
- [`task::Context`]
- [`task::RawWaker`]
- [`task::RawWakerVTable`]
- [`task::Waker`]
- [`task::Poll`]

Cargo
-----
- [Cargo will now produce an error if you attempt to use the name of a required
dependency as a feature.][cargo/6860]
- [You can now pass the `--offline` flag to run cargo without accessing the netw
ork.][cargo/6934]

You can find further change's in [Cargo's 1.36.0 release notes][cargo-1-36-0].

Clippy
------
There have been numerous additions and fixes to clippy, see [Clippy's 1.36.0 rel
ease notes][clippy-1-36-0] for more details.

Misc
----

Compatibility Notes
-------------------
- [`std::arch::x86::_rdtsc` returns `u64` instead of `i64`][stdsimd/559]
- [`std::arch::x86_64::_mm_shuffle_ps` takes an `i32` instead of `u32` for `mask
`][stdsimd/522]
- With the stabilisation of `mem::MaybeUninit`, `mem::uninitialized` use is no
  longer recommended, and will be deprecated in 1.38.0.

[60318]: rust-lang/rust#60318
[60364]: rust-lang/rust#60364
[60370]: rust-lang/rust#60370
[60404]: rust-lang/rust#60404
[60234]: rust-lang/rust#60234
[60244]: rust-lang/rust#60244
[58623]: rust-lang/rust#58623
[59648]: rust-lang/rust#59648
[59675]: rust-lang/rust#59675
[59825]: rust-lang/rust#59825
[59826]: rust-lang/rust#59826
[59445]: rust-lang/rust#59445
[59114]: rust-lang/rust#59114
[cargo/6860]: rust-lang/cargo#6860
[cargo/6934]: rust-lang/cargo#6934
[`VecDeque::rotate_left`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_left
[`VecDeque::rotate_right`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_right
[`Iterator::copied`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#tymethod.copied
[`io::IoSlice`]: https://doc.rust-lang.org/std/io/struct.IoSlice.html
[`io::IoSliceMut`]: https://doc.rust-lang.org/std/io/struct.IoSliceMut.html
[`Read::read_vectored`]: https://doc.rust-lang.org/std/io/trait.Read.html#method.read_vectored
[`Write::write_vectored`]: https://doc.rust-lang.org/std/io/trait.Write.html#method.write_vectored
[`str::as_mut_ptr`]: https://doc.rust-lang.org/std/primitive.str.html#method.as_mut_ptr
[`mem::MaybeUninit`]: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html
[`pointer::align_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.align_offset
[`future::Future`]: https://doc.rust-lang.org/std/future/trait.Future.html
[`task::Context`]: https://doc.rust-lang.org/beta/std/task/struct.Context.html
[`task::RawWaker`]: https://doc.rust-lang.org/beta/std/task/struct.RawWaker.html
[`task::RawWakerVTable`]: https://doc.rust-lang.org/beta/std/task/struct.RawWakerVTable.html
[`task::Waker`]: https://doc.rust-lang.org/beta/std/task/struct.Waker.html
[`task::Poll`]: https://doc.rust-lang.org/beta/std/task/enum.Poll.html
[clippy-1-36-0]: https://github.com/rust-lang/rust-clippy/blob/master/CHANGELOG.md#rust-136
[cargo-1-36-0]: https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-136-2019-07-04
[stdsimd/522]: rust-lang/stdarch#522
[stdsimd/559]: rust-lang/stdarch#559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.