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

Don't discard marker trait impls when inference variables are present #68057

Merged
merged 1 commit into from Jan 20, 2020

Conversation

@Aaron1011
Copy link
Contributor

Aaron1011 commented Jan 9, 2020

Fixes #61651

Previously, we would unconditionally discard impl candidates for marker
traits during trait selection. However, if the predicate had inference
variables, this could have the effect of constrainting inference
variables (due to a successful trait selection) when we would have
otherwise failed due to mutliple applicable impls,

This commit prevents marker trait impls from being discarded while the
obligation predicate has any inference variables, ensuring that
discarding impls will never cause us to incorrectly constraint inference
variables.

Fixes #61651

Previously, we would unconditionally discard impl candidates for marker
traits during trait selection. However, if the predicate had inference
variables, this could have the effect of constrainting inference
variables (due to a successful trait selection) when we would have
otherwise failed due to mutliple applicable impls,

This commit prevents marker trait impls from being discarded while the
obligation predicate has any inference variables, ensuring that
discarding impls will never cause us to incorrectly constraint inference
variables.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 9, 2020

r? @matthewjasper

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

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Jan 19, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 19, 2020

📌 Commit 4840cd8 has been approved by matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 20, 2020

⌛️ Testing commit 4840cd8 with merge 29b854f...

bors added a commit that referenced this pull request Jan 20, 2020
…wjasper

Don't discard marker trait impls when inference variables are present

Fixes #61651

Previously, we would unconditionally discard impl candidates for marker
traits during trait selection. However, if the predicate had inference
variables, this could have the effect of constrainting inference
variables (due to a successful trait selection) when we would have
otherwise failed due to mutliple applicable impls,

This commit prevents marker trait impls from being discarded while the
obligation predicate has any inference variables, ensuring that
discarding impls will never cause us to incorrectly constraint inference
variables.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 20, 2020

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 29b854f to master...

@bors bors added the merged-by-bors label Jan 20, 2020
@bors bors merged commit 4840cd8 into rust-lang:master Jan 20, 2020
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20200109.36 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@Aaron1011 Aaron1011 deleted the Aaron1011:fix/marker-trait-selection branch Jan 25, 2020
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Jan 25, 2020
See rust-lang#29864

This has been replaced by `#[feature(marker_trait_attr)]`

A few notes:

* Due to PR rust-lang#68057 not yet being in the bootstrap compiler, it's
  necessary to continue using `#![feature(overlapping_marker_traits)]`
  under `#[cfg(bootstrap)]` to work around type inference issues.
* I've updated tests that used `overlapping_marker_traits` to now use
  `marker_trait_attr` where applicable

The test `src/test/ui/overlap-marker-trait.rs` doesn't make any sense
now that `overlapping_marker_traits`, so I removed it.

The test `src/test/ui/traits/overlap-permitted-for-marker-traits-neg.rs`
now fails, since it's no longer possible to have multiple overlapping
negative impls of `Send`. I believe that this is the behavior we want
(assuming that `Send` is not going to become a `#[marker]` trait, so I
renamed the test to `overlap-permitted-for-marker-traits-neg`
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Jan 28, 2020
See rust-lang#29864

This has been replaced by `#[feature(marker_trait_attr)]`

A few notes:

* Due to PR rust-lang#68057 not yet being in the bootstrap compiler, it's
  necessary to continue using `#![feature(overlapping_marker_traits)]`
  under `#[cfg(bootstrap)]` to work around type inference issues.
* I've updated tests that used `overlapping_marker_traits` to now use
  `marker_trait_attr` where applicable

The test `src/test/ui/overlap-marker-trait.rs` doesn't make any sense
now that `overlapping_marker_traits`, so I removed it.

The test `src/test/ui/traits/overlap-permitted-for-marker-traits-neg.rs`
now fails, since it's no longer possible to have multiple overlapping
negative impls of `Send`. I believe that this is the behavior we want
(assuming that `Send` is not going to become a `#[marker]` trait, so I
renamed the test to `overlap-permitted-for-marker-traits-neg`
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Jan 28, 2020
See rust-lang#29864

This has been replaced by `#[feature(marker_trait_attr)]`

A few notes:

* Due to PR rust-lang#68057 not yet being in the bootstrap compiler, it's
  necessary to continue using `#![feature(overlapping_marker_traits)]`
  under `#[cfg(bootstrap)]` to work around type inference issues.
* I've updated tests that used `overlapping_marker_traits` to now use
  `marker_trait_attr` where applicable

The test `src/test/ui/overlap-marker-trait.rs` doesn't make any sense
now that `overlapping_marker_traits`, so I removed it.

The test `src/test/ui/traits/overlap-permitted-for-marker-traits-neg.rs`
now fails, since it's no longer possible to have multiple overlapping
negative impls of `Send`. I believe that this is the behavior we want
(assuming that `Send` is not going to become a `#[marker]` trait, so I
renamed the test to `overlap-permitted-for-marker-traits-neg`
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Feb 4, 2020
See rust-lang#29864

This has been replaced by `#[feature(marker_trait_attr)]`

A few notes:

* Due to PR rust-lang#68057 not yet being in the bootstrap compiler, it's
  necessary to continue using `#![feature(overlapping_marker_traits)]`
  under `#[cfg(bootstrap)]` to work around type inference issues.
* I've updated tests that used `overlapping_marker_traits` to now use
  `marker_trait_attr` where applicable

The test `src/test/ui/overlap-marker-trait.rs` doesn't make any sense
now that `overlapping_marker_traits`, so I removed it.

The test `src/test/ui/traits/overlap-permitted-for-marker-traits-neg.rs`
now fails, since it's no longer possible to have multiple overlapping
negative impls of `Send`. I believe that this is the behavior we want
(assuming that `Send` is not going to become a `#[marker]` trait, so I
renamed the test to `overlap-permitted-for-marker-traits-neg`
bors added a commit that referenced this pull request Feb 4, 2020
Remove the `overlapping_marker_traits` feature

See #29864

This has been replaced by `#[feature(marker_trait_attr)]`

A few notes:

* Due to PR #68057 not yet being in the bootstrap compiler, it's
  necessary to continue using `#![feature(overlapping_marker_traits)]`
  under `#[cfg(bootstrap)]` to work around type inference issues.
* I've updated tests that used `overlapping_marker_traits` to now use
  `marker_trait_attr` where applicable

The test `src/test/ui/overlap-marker-trait.rs` doesn't make any sense
now that `overlapping_marker_traits`, so I removed it.

The test `src/test/ui/traits/overlap-permitted-for-marker-traits-neg.rs`
now fails, since it's no longer possible to have multiple overlapping
negative impls of `Send`. I believe that this is the behavior we want
(assuming that `Send` is not going to become a `#[marker]` trait, so I
renamed the test to `overlap-permitted-for-marker-traits-neg`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.