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

Implement name resolution for the IfLet expression. #1241

Merged
merged 1 commit into from
May 23, 2022

Conversation

antego
Copy link
Contributor

@antego antego commented May 10, 2022

Addresses #1177.

Guidance from the ticket #1177:

You should be able to copy how we do name resolution for the if blocks from that file. Though if let statements add a new binding and scope level so you need to declare the pattern so you should be able to see how that is done from rust-ast-resolve-stmt.

I don't understand how to modify the block expression resolution so that it can handle the IfLet expression. For now, I copied the code from the MatchExpr resolution. Not sure how to test it either and what is the desired expected result of the name resolution so I just hope that reviewers will spot the errors.

I created this PR in order to get some guidance about how to proceed with it. Thanks!

@antego antego marked this pull request as ready for review May 10, 2022 12:08
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

That looks good to me, but waiting for @philberty's expert opinion. One way to test this would be to add a test case using an if let construct with a pattern which depends on name resolution, something like:

mod a {
     enum Foo {
         Bar,
         Baz(i32),
      }
}

fn main() -> i32 {
    let value = a::Foo::Baz(15);
    if let a::Foo::Baz(value) = a {
        return 0;
    }
    
    1
}

Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

I think this looks good. What sort of test-case are you using at the moment?

@antego
Copy link
Contributor Author

antego commented May 12, 2022

What sort of test-case are you using at the moment?

@philberty still trying to compile the code from #1177

fn main() -> i32 {
    let mut res = 0;

    enum E {
        X(u8),
    }
    let v = E::X(4);
    if let E::X(n) = v {
        res = n;
    }

    0
}

It fails with

rust1: internal compiler error: in append_reference_for_def, at rust/resolve/rust-name-resolver.cc:200

I don't know if this error should be fixed by this PR or not.

@CohenArthur
Copy link
Member

@antego I think this might be because you're not inserting a new definition? I'm not really sure as I've never worked on pattern resolution before. But the assertion is being triggered because the defId was not declared in this scope or an outer one. If we look at the ResolvePattern or PatternDeclaration class, they often include calls to resolver->insert_new_definition which might be what you need?

Sorry if this doesn't help :DD Trying to figure out something so you can progress!

@antego
Copy link
Contributor Author

antego commented May 18, 2022

@CohenArthur appreciate the attempt but I still can't understand why MatchExpr works without calling the insert_new_definition.

While trying out different things I noticed that the part of the test code which causes the error is actually the line

    let v = E::X(4);

The minimal test case that fails with the error above is:

fn main() -> i32 {
    enum E {
        X(u8),
    }
    let v = E::X(4);

    0
}

looking into this

@antego antego closed this May 18, 2022
@antego antego reopened this May 18, 2022
@antego
Copy link
Contributor Author

antego commented May 18, 2022

This test case

enum E {
    X(u8),
}

fn main() -> i32 {
    let mut res = 0;
    let v = E::X(4);
    if let E::X(n) = v {
        res = n;
    }

    0
}

fails with the error

error: failed to type resolve expression
    8 |     if let E::X(n) = v {
      |     ^

Looks like the name resolution part is figured out and the type resolution is next?

@antego
Copy link
Contributor Author

antego commented May 18, 2022

I guess the failing name resolution for enums should be addressed in another issue?

@antego
Copy link
Contributor Author

antego commented May 23, 2022

@CohenArthur @philberty are you happy to merge this PR since the name resolution doesn't throw the error anymore?

@philberty
Copy link
Member

bors try

bors bot added a commit that referenced this pull request May 23, 2022
@philberty
Copy link
Member

@antego sorry this is my fault let me double-check the merge is ok first :)

@bors
Copy link
Contributor

bors bot commented May 23, 2022

try

Build succeeded:

@philberty
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented May 23, 2022

Build succeeded:

@bors bors bot merged commit 63762cc into Rust-GCC:master May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants