Skip to content

Properly check the defining scope of existential types - #63093

Merged
bors merged 3 commits into
rust-lang:masterfrom
Aaron1011:fix/existential-closure
Jul 30, 2019
Merged

Properly check the defining scope of existential types#63093
bors merged 3 commits into
rust-lang:masterfrom
Aaron1011:fix/existential-closure

Conversation

@Aaron1011

@Aaron1011Aaron1011 commented Jul 28, 2019

Copy link
Copy Markdown
Contributor

Fixes#52632

Existential types (soon to be 'impl trait' aliases) can either be
delcared at a top-level crate/module scope, or within another item such
as an fn. Previously, we were handling the second case incorrectly when
recursively searching for defining usages - we would check children of
the item, but not the item itself. This lead to us missing closures
that consituted a defining use of the existential type, as their opaque
type instantiations are stored in the TypeckTables of their parent
function.

This commit ensures that we explicitly visit the defining item itself,
not just its children.

@Aaron1011
Properly check the defining scope of existential types
18bf9dd
Fixesrust-lang#52632
Existential types (soon to be 'impl trait' aliases) can either be
delcared at a top-level crate/module scope, or within another item such
as an fn. Previously, we were handling the second case incorrectly when
recursively searching for defining usages - we would check children of
the item, but not the item itself. This lead to us missing closures
that consituted a defining use of the existential type, as their opaque
type instantiations are stored in the TypeckTables of their parent
function.
This commit ensures that we explicitly visit the defining item itself,
not just its children.
@rust-highfive

rust-highfive commented Jul 28, 2019

Copy link
Copy Markdown
Contributor

r? @petrochenkov

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2019
@Aaron1011

Aaron1011 commented Jul 28, 2019

Copy link
Copy Markdown
ContributorAuthor

While I don't make any behavior changes to src/librustc/infer/opaque_types/mod.rs, the modified `trace~ was helpful in tracking down the cause of the ICE.

Centril
Centril reviewed Jul 28, 2019
View reviewed changes
Comment threadsrc/test/ui/existential-type/issue-52843.rs
@@ -0,0 +1,11 @@
#![feature(existential_type)]

@CentrilCentrilJul 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you also add some comments to this file and rename it to issue-52843-$description?

Centril
Centril reviewed Jul 28, 2019
View reviewed changes

@CentrilCentril left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nits

Comment threadsrc/librustc_typeck/collect.rs Outdated
Comment threadsrc/librustc_typeck/collect.rs
Comment threadsrc/librustc_typeck/collect.rs
Comment threadsrc/librustc_typeck/collect.rs Outdated
Comment threadsrc/librustc_typeck/collect.rs Outdated
@Centril

Centril commented Jul 28, 2019

Copy link
Copy Markdown
Contributor

cc @oli-obk@cramertj@varkor

Aaron1011and others added 2 commits July 28, 2019 18:33
@Aaron1011@Centril
Fix formatting
8811b9c
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
@Aaron1011
Rename test and add comment
3e98c3a
@Aaron1011

Aaron1011 commented Jul 28, 2019

Copy link
Copy Markdown
ContributorAuthor

@Centril fixed

@cramertj

cramertj commented Jul 29, 2019

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jul 29, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 3e98c3a has been approved by cramertj

@borsbors 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 29, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 29, 2019
@Centril
Rollup merge ofrust-lang#63093- Aaron1011:fix/existential-closure, …
4551dc2
…r=cramertj
Properly check the defining scope of existential types
Fixesrust-lang#52632
Existential types (soon to be 'impl trait' aliases) can either be
delcared at a top-level crate/module scope, or within another item such
as an fn. Previously, we were handling the second case incorrectly when
recursively searching for defining usages - we would check children of
the item, but not the item itself. This lead to us missing closures
that consituted a defining use of the existential type, as their opaque
type instantiations are stored in the TypeckTables of their parent
function.
This commit ensures that we explicitly visit the defining item itself,
not just its children.
@CentrilCentril mentioned this pull request Jul 29, 2019
Closed
@petrochenkov

petrochenkov commented Jul 29, 2019

Copy link
Copy Markdown
Contributor

r? @cramertj

Centril added a commit to Centril/rust that referenced this pull request Jul 29, 2019
@Centril
Rollup merge ofrust-lang#63093- Aaron1011:fix/existential-closure, …
e93f9cf
…r=cramertj
Properly check the defining scope of existential types
Fixesrust-lang#52632
Existential types (soon to be 'impl trait' aliases) can either be
delcared at a top-level crate/module scope, or within another item such
as an fn. Previously, we were handling the second case incorrectly when
recursively searching for defining usages - we would check children of
the item, but not the item itself. This lead to us missing closures
that consituted a defining use of the existential type, as their opaque
type instantiations are stored in the TypeckTables of their parent
function.
This commit ensures that we explicitly visit the defining item itself,
not just its children.
@CentrilCentril mentioned this pull request Jul 29, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jul 30, 2019
@Centril
Rollup merge ofrust-lang#63093- Aaron1011:fix/existential-closure, …
e464c9d
…r=cramertj
Properly check the defining scope of existential types
Fixesrust-lang#52632
Existential types (soon to be 'impl trait' aliases) can either be
delcared at a top-level crate/module scope, or within another item such
as an fn. Previously, we were handling the second case incorrectly when
recursively searching for defining usages - we would check children of
the item, but not the item itself. This lead to us missing closures
that consituted a defining use of the existential type, as their opaque
type instantiations are stored in the TypeckTables of their parent
function.
This commit ensures that we explicitly visit the defining item itself,
not just its children.
@CentrilCentril mentioned this pull request Jul 30, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jul 30, 2019
@Centril
Rollup merge ofrust-lang#63093- Aaron1011:fix/existential-closure, …
f3750e3
…r=cramertj
Properly check the defining scope of existential types
Fixesrust-lang#52632
Existential types (soon to be 'impl trait' aliases) can either be
delcared at a top-level crate/module scope, or within another item such
as an fn. Previously, we were handling the second case incorrectly when
recursively searching for defining usages - we would check children of
the item, but not the item itself. This lead to us missing closures
that consituted a defining use of the existential type, as their opaque
type instantiations are stored in the TypeckTables of their parent
function.
This commit ensures that we explicitly visit the defining item itself,
not just its children.
@CentrilCentril mentioned this pull request Jul 30, 2019
Merged
bors added a commit that referenced this pull request Jul 30, 2019
@bors
Auto merge of#63124- Centril:rollup-onohtqt, r=Centril
4eeaaa7
Rollup of 12 pull requests
Successful merges:
- #61965 (Remove mentions of removed `offset_to` method from `align_offset` docs)
- #62928 (Syntax: Recover on `for ( $pat in $expr ) $block`)
- #63000 (Impl Debug for Chars)
- #63083 (Make generic parameters always use modern hygiene)
- #63087 (Add very simple edition check to tidy.)
- #63093 (Properly check the defining scope of existential types)
- #63096 (Add tests for some `existential_type` ICEs)
- #63099 (vxworks: Remove Linux-specific comments.)
- #63106 (ci: Skip installing SWIG/xz on OSX )
- #63108 (Add links to None in Option doc)
- #63109 (std: Fix a failing `fs` test on Windows)
- #63111 (Add syntactic and semantic tests for rest patterns, i.e. `..`)
Failed merges:
r? @ghost
oli-obk
oli-obk reviewed Jul 30, 2019
View reviewed changes
Comment threadsrc/test/ui/existential-type/issue-52843-closure-constrain.rs
fn main() {
existential type Existential: Debug;
fn _unused() -> Existential { String::new() }
//~^ ERROR: concrete type differs from previous defining existential type use

@oli-obkoli-obkJul 30, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

moderately confusing to have the first item (textually) talk about "previous". Maybe we should reword "previous" to "other"

@bors
bors merged commit 3e98c3a into rust-lang:masterJul 30, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obkoli-obkoli-obk left review comments
+1 more reviewer
@CentrilCentrilCentril left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@cramertjcramertj

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Existential type can have inconsistent concrete type

7 participants

@Aaron1011@rust-highfive@Centril@cramertj@bors@petrochenkov@oli-obk