Skip to content

Fix member accesses for nested structs returned from funcs#537

Closed
vezenovm wants to merge 6 commits intomasterfrom
mv/nested-structs-fix
Closed

Fix member accesses for nested structs returned from funcs#537
vezenovm wants to merge 6 commits intomasterfrom
mv/nested-structs-fix

Conversation

@vezenovm
Copy link
Copy Markdown
Contributor

@vezenovm vezenovm commented Nov 29, 2022

Related issue(s)

Resolves #529

Trying to resolve #492, however, there is still an issue.

Description

This PR aims to fix how nested tuples are handled in SSA. When returning a nested struct from a function there is a high chance of a mismatch in the expected field member node actually being accessed and the one that should be accessed. The error is elaborated upon in issue #529.

Summary of changes

I added extra logic to the codegen_identifier method. We determine whether the ident being accessed is a nested struct/tuple that was returned from a function.When creating an SSA function we flatten the return values. This can lead to a mismatch in the NodeId that is fetched. We recreate a struct/tuple with an unflattened structure in order to enable accurate indexing. If the Value::Tuple has a longer list of NodeIds (meaning it has been flattened) we know to use our recreated value which possesses the correct structure.

NOTE I had an issue with stack overflows that I previously mentioned in this PR description, but I just partially fixed it. However, it broke this change I mention in the paragraph below. Reference Additional Context section for more details.

I also added a conditional to into_field_member in code_gen.rs. In #492 it looks like when we had a struct with a singular element, which when returned from a function is also flattened. However, it is flattened into a singular value causing it to hit unreachable code. I simply check whether the field_index being accessed is 0 and then return the value object itself if this is the case.

Dependency additions / changes

N/A

Test additions / changes

I added a test showing the usage of nested structs being returned from functions. I also added a test for the example provided in issue #492. Look to # Additional Context for more information about the comments.

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

THIS ISSUE PARTIALLY FIXED
There is something missing still in SSA when handling the nested struct inside of the main function. In my test I highlight the snippets that can cause this error. Currently, I must specify the return value in the Prover.toml if that return value comes from a nested struct returned by a function in main. If I attempt to have Noir auto-fill I will get a stack overflow when attempting to solve. However, auto-fill still works as expected if I access the nested struct member in a separate function (non-main func). I can then return from main the value returned from this separate function as expected. This is shown in my test in this PR. I have a suspicion this could be related to accessing the correct SSA nodes when I construct a new Value::Tuple.

Fields from a nested struct that have been constrained elsewhere in main can be main return values, however, I am still getting a stack overflow when trying to return nested struct fields that have not been used. It is looking like I may have to add extra constraints for nested structs return by a func that are then accessed inside of main.

STILL BROKEN
This PR is still not feature complete. Fixing the fixed handling of individual nested structs seems to not be working as well for tuples. The issue that was occurs in #492 has been replaced with this bug:

thread 'main' panicked at 'removal index (is 1) should be < len (is 1)', crates/noirc_evaluator/src/ssa/code_gen.rs:92:48

I did not have a chance to investigate this further, but I do not think it should be a hard fix and want to get it into this PR.

Copy link
Copy Markdown
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

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

Considering the comments explaining that this does not solve all the cases, I thought it would be simpler to start from scratch, so I implemented nested structures from functions in PR #542 and I suggest to close this one.

@vezenovm
Copy link
Copy Markdown
Contributor Author

This PR is deprecated by PR #542

@vezenovm vezenovm closed this Nov 30, 2022
@TomAFrench TomAFrench deleted the mv/nested-structs-fix branch January 8, 2026 17:07
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.

Nested structs not handled correctly in codegen Cannot return tuple with struct inside

2 participants