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

Loop over all opaque types instead of looking at just the first one with the same DefId #87107

Merged
merged 3 commits into from
Jul 16, 2021

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jul 13, 2021

This exposed a bug in VecMap and is needed for #86410 anyway

r? @spastorino

cc @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2021
|
LL | fn cool_fn<'a>(arg: &'a str) -> OpaqueOutputImpl<'a> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Copy link
Member

Choose a reason for hiding this comment

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

The PR is great to me, we've discussed this with Oli already. I'm not strong enough to approve in particular this part of the PR. cc @nikomatsakis

Copy link
Member

Choose a reason for hiding this comment

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

The span here is kind of confusing. I suppose we want to be pointing to the binding?

@spastorino
Copy link
Member

Assigning to Niko, based on my comment above.
r? @nikomatsakis

@@ -12,6 +12,7 @@ trait Output<'a> {}
impl<'a> Output<'a> for &'a str {}

fn cool_fn<'a>(arg: &'a str) -> OpaqueOutputImpl<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is kind of weird. Does it pass if the let out: ... is removed?

I expect it to pass, because we stabilized member constraints in #84701, but it seems like the let out: ... is probably caused it not to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is kind of weird. Does it pass if the let out: ... is removed?

yes, it does. Note that this test has impl_trait_in_bindings enabled, so it's not really relevant for min_tait. The problem is that the let binding gives us

expected &'<empty> str, got &'a str

which is probably due to the odd way that impl_trait_in_bindings works and that should hopefully all go away once we do not treat them special anymore and just put them into the inference list just like we do with the return type.

Copy link
Member

Choose a reason for hiding this comment

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

This test is really weird, we were talking about it with @lqd some minutes ago.

Note the following https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=3c4de7d1bfdfee59d8e2f3f5c0d39453

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd merge this as is, in my remove let binding PR this test is going away anyway so ...

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 16, 2021

📌 Commit 587e8fd has been approved by nikomatsakis

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 16, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 16, 2021
Loop over all opaque types instead of looking at just the first one with the same DefId

This exposed a bug in VecMap and is needed for rust-lang#86410 anyway

r? `@spastorino`

cc `@nikomatsakis`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2021
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#87107 (Loop over all opaque types instead of looking at just the first one with the same DefId)
 - rust-lang#87158 (Suggest full enum variant for local modules)
 - rust-lang#87174 (Stabilize `[T; N]::map()`)
 - rust-lang#87179 (Mark `const_trait_impl` as active)
 - rust-lang#87180 (feat(rustdoc): open sidebar menu when links inside it are focused)
 - rust-lang#87188 (Add GUI test for auto-hide-trait-implementations setting)
 - rust-lang#87200 (TAIT: Infer all inference variables in opaque type substitutions via InferCx)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8273567 into rust-lang:master Jul 16, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants