Implement support for generators

[fzaiser]

Description of changes:

This implements the generator feature for Rust. We translate a generator type similarly to an enum with a variant for each suspend point.

Consider the following generator:

|| {
    let a = true;
    let b = &a;
    yield;
    assert_eq!(b as *const _, &a as *const _);
    yield;
};

Rustc compiles this to something similar to the following enum (but there are differences, see below!):

enum GeneratorEnum {
    Unresumed,                        // initial state of the generator
    Returned,                         // generator has returned
    Panicked,                         // generator has panicked
    Suspend0 { b: &bool, a: bool },   // state after suspending (`yield`ing) for the first time
    Suspend1,                         // state after suspending (`yield`ing) for the second time
}

However, its layout may differ from normal Rust enums in the following ways:

• Contrary to enums, the discriminant may not be at offset 0.
• Contrary to enums, there may be other fields than the discriminant "at the top level" (outside the variants).

This means that we CANNOT use the enum translation, which would be roughly as follows:

struct GeneratorEnum {
    int case; // discriminant
    union GeneratorEnum-union cases; // variant
}

union GeneratorEnum-union {
    struct Unresumed variant0;
    struct Returned variant1;
    // ...
}

Instead, we use the following translation:

union GeneratorEnum {
    struct DirectFields direct_fields;
    struct Unresumed generator_variant_Unresumed;
    struct Returned generator_variant_Returned;
    // ...
}

struct DirectFields {
    // padding (for bool *b in Suspend0 below)
    char case;
    // padding (for bool a in Suspend0 below)
}

struct Unresumed {
    // padding (this variant has no fields)
}

// ...

struct Suspend0 {
    bool *generator_field_0; // variable b in the generator code above
    // padding (for char case in DirectFields)
    bool generator_field_1; // variable a in the generator code above
}

Of course, if the generator has any other top-level/direct fields, they'd be included in the DirectFields struct as well.

Resolved issues:

Resolves #416

Call-outs:

• The function ignore_var_ty became redundant, so I removed it.
• There is some code duplication with codegen_enum. I'm not sure how to remove it because codegen_enum relies on an AdtDef, which is not available for generators (because the relevant enum is generated in MIR).
• I added a couple of assertions in codegen_enum that document assumptions that we currently make about enum layouts. (I don't know if they're guaranteed for enums, but they don't hold for generators, so I thought it would be good to be explicit about it.)
• One of the new tests (discriminant.rs, taken from rustc) takes a bit over a minute on my laptop. Is that too much?
  • Discussed with @danielsn, decision: leave it in, but move slow tests to a separate test suite in a follow-up PR

Testing:

• How is this change tested? 3 existing regression tests (that now work) + 16 test cases from rustc (src/test/ui/generator/)

• Is this a refactor change? No

Checklist

• Each commit message has a non-empty body, explaining why the change was made
• Methods or procedures are documented
• Regression or unit tests are included, or existing tests cover the modified code
• My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

[celinval]

Can you please add comments to your code and maybe a high level explanation of how you modeled generators in the PR body?

[fzaiser]

@celinval I added both a comment in the code and the PR.

[fzaiser]

I believe this comment was outdated because the if condition only checks for ZST.

I'll try to take a look at this tomorrow. Remove my change request since you have added a great description to this issue. Thanks!

[celinval]

This is my first pass. I'll keep taking a look at it tomorrow. Thanks!

[celinval]

The same here. Can you please make this test more meaningful.

[fzaiser]

I don't think this test is really necessary anymore, but I'm hesitant to remove tests. It used to test that a file containing generators worked as long as generators are not actually used in the execution of the program. Should I remove it?

[celinval]

Either remove it or add a check at the end

[fzaiser]

I've changed the test completely. It now checks whether generators work with resume types that are not (). I don't think the original test makes sense anymore, now that generators are properly implemented.

[celinval]

Can you please document what you are doing here?

[fzaiser]

To be honest, I don't really understand what's happening there (or in closure_sig, which also has no explanatory comment). Both are copied from compiler/rustc_middle/src/ty/layout.rs. That's why I added the tracking issue: #1365

[danielsn]

⛏️ error message could be more informative

[fzaiser]

Improved

[danielsn]

Might be clearer to lift these to a precondition

if t.is_generator {
    assert!(matches!(layout.variants, Variants::Single { .. });
    etc ...
}

[fzaiser]

Created a function check_generator_layout_assumptions because this code was repeated in a few places.

[danielsn]

What happens in a Niche encoding

[fzaiser]

It seems that they have a tag_field anyway: https://doc.rust-lang.org/stable/nightly-rustc/rustc_target/abi/enum.Variants.html#variant.Multiple.field.tag_field. So this assertion should not be a problem (and the regression tests pass).

[fzaiser]

Regarding the assertions checking that the layout of a generator is multiple variants, with direct encoding: I created a function check_generator_layout_assumptions because there were many places where you suggested we check these assumptions. But thinking about it more, I think it's enough to check it in once, when the generator is created, i.e. in codegen_ty_generator. The later checks seem redundant. Do you agree and can I remove them?

[celinval]

Regarding the issue #1395, can you please inject a unimplemented failure if the user tried to get the size of a generator? Silently failing might result in an unsound analysis.

[celinval]

That's a much nicer error message. :)

[fzaiser]

Regarding the issue #1395, can you please inject a unimplemented failure if the user tried to get the size of a generator? Silently failing might result in an unsound analysis.

@celinval I first thought this was a good idea and I just tried it, but it turns out that 5 of the other tests rely on size_of, so we'd have to deactivate those as well, which I think does more harm than good. The WASM backend has similarly decided that it's fine to disable that one test where the size differs. (Why LLVM computes a different size is still a mystery to me. All the code I can find in rustc's codegen uses layout.size.bytes(), which is exactly what we're doing.)

[fzaiser]

Regarding the assertions checking that the layout of a generator is multiple variants, with direct encoding: I created a function check_generator_layout_assumptions because there were many places where you suggested we check these assumptions. But thinking about it more, I think it's enough to check it in once, when the generator is created, i.e. in codegen_ty_generator. The later checks seem redundant. Do you agree and can I remove them?

I have decided to remove the additional checks and only check the assumption once, when we create the generator. This is what we do for other assumptions as well: check them on creation, not every time the datatype is used.

[celinval]

Thanks for doing this!

[celinval]

Can you please create an issue to take a look at the performance of this test?

[fzaiser]

Opened #1406