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

librustc: Remove for desugaring #15809

Merged
merged 1 commit into from
Jul 25, 2014
Merged

Conversation

pcwalton
Copy link
Contributor

librustc: Stop desugaring for expressions and translate them directly.

This makes edge cases in which the Iterator trait was not in scope
and/or Option or its variants were not in scope work properly.

This breaks code that looks like:

struct MyStruct { ... }

impl MyStruct {
    fn next(&mut self) -> Option<int> { ... }
}

for x in MyStruct { ... } { ... }

Change ad-hoc next methods like the above to implementations of the
Iterator trait. For example:

impl Iterator<int> for MyStruct {
    fn next(&mut self) -> Option<int> { ... }
}

Closes #15392.

[breaking-change]

@ghost
Copy link

ghost commented Jul 19, 2014

check_match.rs will also have to be extended to look at ExprForLoop.

@huonw
Copy link
Member

huonw commented Jul 19, 2014

If anyone else is confused why this diff-stat so large: the 500+ lines of the Iterator trait and its documentation have been duplicated purely for staging.

@alexcrichton
Copy link
Member

Could you also add some tests that do things like implement next but not Iterator or shadow Some and None?

@pcwalton
Copy link
Contributor Author

@alexcrichton Yup, I was planning to do that before submitting the final.

@lilyball
Copy link
Contributor

If anyone else is confused why this diff-stat so large: the 500+ lines of the Iterator trait and its documentation have been duplicated purely for staging.

Why is the duplication necessary? The stage0 compiler should still be able to desugar the for loops from the current implementation.

@huonw
Copy link
Member

huonw commented Jul 21, 2014

It now needs a lang item; and one would expect that unrecognised lang items are an error, meaning the stage0 compiler couldn't handle it. However, this doesn't seem to be the case (unrecognised lang items are silently ignored: http://is.gd/wfG8JS), so it does appear that the duplication is unnecessary.

@lilyball
Copy link
Contributor

@huonw Hrm, I would have expected an unrecognized lang item to emit a warning (instead of an error).

@pcwalton
Copy link
Contributor Author

This is ready for initial review; there's still one borrow check test failing but it should be fairly easy to fix. r? @pnkfelix

@pcwalton pcwalton changed the title librustc: WIP patch for removing for desugaring librustc: Remove for desugaring Jul 22, 2014
@pcwalton
Copy link
Contributor Author

This is passing tests now. There are two bugs which I would like to fix in follow ups, as they are backwards compatible modulo gated features and don't impact much code (less than 10 occurrences in the whole codebase):

  1. for &x in foo.iter() { ... x ... } doesn't pass the borrow check anymore. Instead use for x in foo.iter() { ... *x ... }. This is probably not too hard of a fix, but I wanted to get this in for now as this code is really borrow check finicky.
  2. The for pattern is no longer macro-hygienic. This is because macro hygiene is applied to each pattern production separately, and this introduces a new pattern production.

@pcwalton
Copy link
Contributor Author

r? @pnkfelix

@pnkfelix
Copy link
Member

@pcwalton make sure to update the description in this ticket, so that bors commit message doesn't say "WIP" and "do not merge yet".

(or i am happy to update the description myself, if you would prefer to hand that off to me).

@@ -1701,6 +1702,77 @@ fn try_overloaded_index(fcx: &FnCtxt,
}
}

/// Given the head of a `for` expression, looks up the `next` method in the
/// `Iterator` trait. Fails if the expression does not implement `next`.
Copy link
Member

Choose a reason for hiding this comment

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

On this comment, add a note to the effect that "the return type represents the concrete element type A of in the Iterator<A> that is producing the for loop's elements"

@pnkfelix
Copy link
Member

@pcwalton okay, LGTM. You can mark with r=me after you add the comment I asked for in src/librustc/middle/typeck/check/mod.rs

This makes edge cases in which the `Iterator` trait was not in scope
and/or `Option` or its variants were not in scope work properly.

This breaks code that looks like:

    struct MyStruct { ... }

    impl MyStruct {
        fn next(&mut self) -> Option<int> { ... }
    }

    for x in MyStruct { ... } { ... }

Change ad-hoc `next` methods like the above to implementations of the
`Iterator` trait. For example:

    impl Iterator<int> for MyStruct {
        fn next(&mut self) -> Option<int> { ... }
    }

Closes rust-lang#15392.

[breaking-change]
bors added a commit that referenced this pull request Jul 25, 2014
librustc: Stop desugaring `for` expressions and translate them directly.

This makes edge cases in which the `Iterator` trait was not in scope
and/or `Option` or its variants were not in scope work properly.

This breaks code that looks like:

    struct MyStruct { ... }

    impl MyStruct {
        fn next(&mut self) -> Option<int> { ... }
    }

    for x in MyStruct { ... } { ... }

Change ad-hoc `next` methods like the above to implementations of the
`Iterator` trait. For example:

    impl Iterator<int> for MyStruct {
        fn next(&mut self) -> Option<int> { ... }
    }

Closes #15392.

[breaking-change]
@bors bors closed this Jul 25, 2014
@bors bors merged commit caa564b into rust-lang:master Jul 25, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 13, 2023
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.

For loops should not be desugared
6 participants