Add way to differentiate argument locals from other locals in Stable MIR

[klinvill]

This PR resolves rust-lang/project-stable-mir#47 which request a way to differentiate argument locals in a SMIR Body from other locals.

Specifically, this PR exposes the arg_count field from the MIR Body. However, I'm opening this as a draft PR because I think there are a few outstanding questions on how this information should be exposed and described. Namely:

• Is exposing arg_count the best way to surface this information to SMIR users? Would it be better to leave arg_count as a private field and add public methods (e.g. fn arguments(&self) -> Iter<'_, LocalDecls>) that may use the underlying arg_count info from the MIR body, but expose this information to users in a more convenient form? Or is it best to stick close to the current MIR convention?
• If the answer to the above point is to stick with the current MIR convention (arg_count), is it reasonable to also commit to sticking to the current MIR convention that the first local is always the return local, while the next arg_count locals are always the (in-order) argument locals?
• Should Body in SMIR only represent function bodies (as implied by the comment I added)? That seems to be the current case in MIR, but should this restriction always be the case for SMIR?

r? @celinval
r? @oli-obk

[rustbot]

Failed to set assignee to celinval: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @celinval (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[celinval]

I definitely see a lot of benefit to add more user friendly APIs, such as get the ret place and a list of arguments. Unfortunately, we either have to make the fields all public or add a constructor to Body if we want to keep the details private

[klinvill]

Is there a reason to avoid adding a constructor? I'd imagine a constructor and helper methods could be as simple as shown below (and then rustc_smir could just call the constructor instead of setting the fields directly):

impl Body {
    /// Constructs a `Body`.
    ///
    /// Required to build a `Body` from outside the crate because the `arg_count` field is private.
    pub fn new(blocks: Vec<BasicBlock>, locals: LocalDecls, arg_count: usize) -> Self {
        Self { blocks, locals, arg_count }
    }

    /// Gets the function's return local.
    pub fn ret_local(&self) -> &LocalDecl {
        &self.locals[0]
    }

    /// Gets the locals in `self` that correspond to the function's arguments.
    pub fn arg_locals(&self) -> &[LocalDecl] {
        &self.locals[1 .. self.arg_count + 1]
    }
}

[celinval]

No... I actually like the encapsulation in cases where the type safety invariant depends on the relationship of its variables. I would suggest that you add a sanity check in the constructor: locals.len() > arg_count.

[celinval]

Looks good to me. Do you mark it as ready for review?

[klinvill]

Looks good to me. Do you mark it as ready for review?

I'll go ahead and mark it as ready. I was trying to figure out how to add a unit test to make sure I didn't have any off-by-one errors, but it seems like constructing a LocalDecl requires a TyCtxt (as mentioned here) which seems like much more complexity than such a simple test should have. Should tests just be added to the project-stable-mir repo instead? (edit: or I suppose I could just add it to the ui tests that are currently failing)

[rustbot]

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

[celinval]

Yeah, unit tests can be quite challenging. For StableMIR, you can see this description on how we test our code: https://rust-lang.github.io/project-stable-mir/dev/index.html#test-suites

[klinvill]

I ended up just adding an extra test within the ui-fulldeps tests. I think that should be the right place for it.

[oli-obk]

@bors r+ rollup

[bors]

📌 Commit bac7d5b has been approved by oli-obk

It is now in the queue for this repository.

[oli-obk]

@bors r+ rollup

[bors]

📌 Commit d55487d has been approved by oli-obk

It is now in the queue for this repository.