Skip to content

Add support for accessing single-element aggregates in #traverseProjection - #657

Merged
automergerpr-permission-manager[bot] merged 3 commits into
masterfrom
jh/deref-aggregate
Aug 25, 2025
Merged

automergerpr-permission-manager[bot] merged 3 commits into
masterfrom
jh/deref-aggregate

Conversation

@Stevengre

@Stevengre Stevengre commented Aug 21, 2025

Copy link
Copy Markdown
Contributor
  • Updated documentation to explain how to directly access elements of an Aggregate when #traverseProjection becomes stuck.
  • Introduced a new rule for handling single-element aggregates in the semantics.
  • Added a new test case for closure access to a struct, ensuring correct behavior.
  • Updated integration tests to include the new failure case for closure access to structs.

…ction

- Updated documentation to explain how to directly access elements of an Aggregate when #traverseProjection becomes stuck.
- Introduced a new rule for handling single-element aggregates in the semantics.
- Added a new test case for closure access to a struct, ensuring correct behavior.
- Updated integration tests to include the new failure case for closure access to structs.
@Stevengre Stevengre changed the title Jh/deref aggregate Add support for accessing single-element aggregates in #traverseProjection Aug 21, 2025
@Stevengre Stevengre self-assigned this Aug 21, 2025
@Stevengre
Stevengre marked this pull request as ready for review August 21, 2025 05:54
Comment on lines +15 to +18
// Closure function that takes &MyStruct reference
let get_value = |struct_ref: &MyStruct| {
struct_ref.data
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(probably off-topic)
While reading this code, I was thinking, what if we were using the struct_list in scope (capturing the variable in the closure), like this:

let get_value = | i: usize | { struct_list[i].data }

We should investigate closures a bit more (but unrelated to p-token)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Provided an issue here: #660

</k>
```

If an Aggregate contains only one element and #traverseProjection becomes stuck, you can directly access this element. For more details, you may remove this rule and run `tests/integration/data/prove-rs/closure_access_struct.rs`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe add text describing what we would see in that test when removing the rule.
Anyone who reads it will understand better without having to repeat your experiments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the description!

│ (277 steps)
└─ 3 (stuck, leaf)
#traverseProjection ( toLocal ( 19 ) , thunk ( #decodeConstant ( constantKindAll

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we write the test program in a way that avoids this constant allocation?
Maybe using a literal array of i32 and producing the struct_list via into_iter, map, and collect?
Would be nicer to have a test that terminates with the fix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is that something like:

struct MyStruct {
    data: i32
}

fn main() {
    // Create data dynamically using iterator pattern to avoid constant allocation
    let data = vec![10, 20, 30, 40, 50];
    let struct_list: Vec<MyStruct> = data.into_iter()
        .map(|d| MyStruct { data: d })
        .collect();
    
    // Closure function that takes &MyStruct reference
    let get_value = |struct_ref: &MyStruct| {
        struct_ref.data
    };
    
    // Use closure to access struct field
    let result = get_value(&struct_list[2]);
    
    // Verify result
    assert!(result == 30);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Initial Attempt: The new test avoided constant allocation but failed with TypeError: 'NoneType' object is not subscriptable in KMIR's smir.py:183.
  2. First Bug Fix: Found that some functions have body: None (specifically alloc::alloc::__rust_no_alloc_shim_is_unstable). Fixed by adding null check in call_edges property.
  3. Second Bug Fix: Encountered KeyError: 131 in reduce_to method. Fixed by filtering out non-existent types from the reachable set.
  4. Root Cause Discovery: Investigated what "type 131" actually is. Found that 131 is not a function type but a span ID (source location identifier) that appears in a StorageLive statement:
    {'kind': {'StorageLive': 15}, 'span': 131}.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It encounters a more weird issue than before.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK nevermind, then we keep this test for now .
The type 131 still exists, IDs of types and spans are disjoint so a span 131 does not mean there is no type 131. (BTW type and function IDs aren't disjoint).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I'm still learning. So it is a bug in smir-json?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not a bug, just that we don't extract all types. (take a look at the TyVisitor https://github.com/runtimeverification/stable-mir-json/blob/master/src/printer.rs#L520)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Let me learn about this.

…rseProjection

- Expanded the explanation of accessing single-element aggregates when #traverseProjection becomes stuck.
- Clarified the behavior of the new rule and its impact on execution steps.
- Emphasized the importance of the rule for closures accessing struct fields.
│ (277 steps)
└─ 3 (stuck, leaf)
#traverseProjection ( toLocal ( 19 ) , thunk ( #decodeConstant ( constantKindAll

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK nevermind, then we keep this test for now .
The type 131 still exists, IDs of types and spans are disjoint so a span 131 does not mean there is no type 131. (BTW type and function IDs aren't disjoint).

@automergerpr-permission-manager
automergerpr-permission-manager Bot merged commit 42e1bb8 into master Aug 25, 2025
5 checks passed
@automergerpr-permission-manager
automergerpr-permission-manager Bot deleted the jh/deref-aggregate branch August 25, 2025 06:48
Stevengre added a commit that referenced this pull request Aug 25, 2025
…ction (#657)

- Updated documentation to explain how to directly access elements of an
Aggregate when #traverseProjection becomes stuck.
- Introduced a new rule for handling single-element aggregates in the
semantics.
- Added a new test case for closure access to a struct, ensuring correct
behavior.
- Updated integration tests to include the new failure case for closure
access to structs.
jberthold pushed a commit that referenced this pull request Sep 5, 2025
…ction (#657)

- Updated documentation to explain how to directly access elements of an
Aggregate when #traverseProjection becomes stuck.
- Introduced a new rule for handling single-element aggregates in the
semantics.
- Added a new test case for closure access to a struct, ensuring correct
behavior.
- Updated integration tests to include the new failure case for closure
access to structs.
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.

2 participants