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 hir ProjectionKind more precise #20

Closed
wants to merge 1 commit into from

Conversation

arora-aman
Copy link
Member

@arora-aman arora-aman commented Jul 7, 2020

This commit also categorizing access as Field, Index, or Subslice.

Ideas are taken from mir::ProjectionElem.

Proposed changes: https://github.com/rust-lang/project-rfc-2229/blob/master/hir-place-target.md

Closes: rust-lang/project-rfc-2229#1,
Closes: rust-lang/project-rfc-2229#2


This change is Reviewable

Copy link
Member Author

@arora-aman arora-aman left a comment

Choose a reason for hiding this comment

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

Leaving some thoughts here so that I don't forget

src/librustc_typeck/mem_categorization.rs Outdated Show resolved Hide resolved
@arora-aman arora-aman requested a review from a team July 7, 2020 06:03
@arora-aman
Copy link
Member Author

arora-aman commented Jul 7, 2020

@arora-aman
Copy link
Member Author

arora-aman commented Jul 7, 2020

@arora-aman arora-aman marked this pull request as ready for review July 7, 2020 08:26
@arora-aman
Copy link
Member Author

arora-aman commented Jul 7, 2020

src/librustc_typeck/mem_categorization.rs Outdated Show resolved Hide resolved
src/librustc_typeck/mem_categorization.rs Outdated Show resolved Hide resolved
Copy link

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

In rust-lang/project-rfc-2229#1 it mentioned Downcast projection kind, which is not present in this PR. Should this PR mark that issue as closed ?

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @arora-aman, @Azhng, @ChrisPardy, @jenniferwills, @logmosier, and @null-sleep)


src/librustc_typeck/mem_categorization.rs, line 822 at r1 (raw file):

                // S { f1: p1, ..., fN: pN }

                let variant_index = if let Some(variant_index) =

Shouldn't this be 0 since there's is only one variant for a struct ?

@arora-aman
Copy link
Member Author

arora-aman commented Jul 7, 2020

@Azhng

Enums can be defined like structures too: https://doc.rust-lang.org/book/ch18-03-pattern-syntax.html#destructuring-enums

Downcast in MIR is to represent using an enum variant

Copy link

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

Also use mc::Result for error handling so it's consistent with rest of the code

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @arora-aman, @ChrisPardy, @jenniferwills, @logmosier, and @null-sleep)

Copy link
Member Author

@arora-aman arora-aman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Azhng, @ChrisPardy, @jenniferwills, @logmosier, and @null-sleep)


src/librustc_typeck/mem_categorization.rs, line 822 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Shouldn't this be 0 since there's is only one variant for a struct ?

Enums can be defined like structures too: https://doc.rust-lang.org/book/ch18-03-pattern-syntax.html#destructuring-enums

@arora-aman
Copy link
Member Author

arora-aman commented Jul 7, 2020

@arora-aman
Copy link
Member Author

@Azhng fixed

Copy link

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @arora-aman, @ChrisPardy, @jenniferwills, @logmosier, and @null-sleep)


src/librustc_typeck/mem_categorization.rs, line 786 at r2 (raw file):

                // Any ADT related errors would've been catched when reading the `variant_index`
                let total_fields =
                    self.total_fields_in_adt_variant(pat.hir_id, variant_index, pat.span).unwrap();

I think we can use the ? syntax here instead of unwrapping


src/librustc_typeck/mem_categorization.rs, line 816 at r2 (raw file):

                for fp in field_pats {
                    let field_ty = self.pat_ty_adjusted(&fp.pat)?;
                    let field_index = self

The code here that retrieve field_index is almost identical to the code in cat_expr_unadjusted. Maybe refactor it to a helper func ?

Copy link

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @arora-aman, @ChrisPardy, @jenniferwills, @logmosier, and @null-sleep)

Copy link
Member

@roxelo roxelo left a comment

Choose a reason for hiding this comment

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

LGTM too

@arora-aman
Copy link
Member Author

Rust PR: rust-lang#74140

This commit also categorizing access as Field, Index, or Subslice.

Ideas are taken from `mir::ProjectionElem`.

Proposed changes: https://github.com/rust-lang/project-rfc-2229/blob/master/hir-place-target.md

Closes: rust-lang/project-rfc-2229#1,
rust-lang/project-rfc-2229#2

Co-authored-by: Aman Arora <me@aman-arora.com>
Co-authored-by: Chris Pardy <chrispardy36@gmail.com>
Co-authored-by: Dhruv Jauhar <dhruvjhr@gmail.com>
Copy link

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @arora-aman, @ChrisPardy, @jenniferwills, @logmosier, and @null-sleep)


src/librustc_typeck/mem_categorization.rs, line 799 at r3 (raw file):

                    self.total_fields_in_adt_variant(pat.hir_id, variant_index, pat.span)?;

                for (i, subpat) in subpats.iter().enumerate_and_adjust(total_fields, dots_pos) {

TIL

@arora-aman
Copy link
Member Author

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @arora-aman, @ChrisPardy, @jenniferwills, @logmosier, and @null-sleep)


src/librustc_typeck/mem_categorization.rs, line 799 at r3 (raw file):

                    self.total_fields_in_adt_variant(pat.hir_id, variant_index, pat.span)?;

                for (i, subpat) in subpats.iter().enumerate_and_adjust(total_fields, dots_pos) {

TIL

This is only for the very direct specific type slide of HirPat borrow

@arora-aman arora-aman closed this Jul 16, 2020
@arora-aman arora-aman deleted the precise_hir_projections branch July 17, 2020 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring HIR Place: Fields Make hir::Place projections more precise
3 participants