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

[Fix] External struct equality #27902

Merged
merged 13 commits into from
Apr 18, 2024
Merged

[Fix] External struct equality #27902

merged 13 commits into from
Apr 18, 2024

Conversation

evan-schott
Copy link
Collaborator

Resolves #27900

@evan-schott evan-schott requested a review from d0cd April 16, 2024 18:58
@evan-schott
Copy link
Collaborator Author

The parsing of composite types will always be ambiguous in the sense that there is no way to know whether it is a struct or a record. This PR makes it so that all composites are parsed as having no program of origin. The ambiguity is resolved because after ST creator pass no two composites can share the same name, even if one is a record and the other is a struct. This means that the ST lookup_struct function can search again with program name set to the local program if it fails to find a record because the type indicated that its program of origin is None. @d0cd lmk if you think there is a better way to do this.

Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

Fairly large PR so reviewing in stages for manageability.

My main questions are:

  • do we need all of the additional clones?
  • why is does the program name need to be removed?
  • is see a number of files where the generated bytecode has changed. Is this expected?

One immediate TODO is add back in the removed errors and regen expectations. This will reduce a lot of the files changed.

compiler/ast/src/functions/finalize.rs Show resolved Hide resolved
compiler/ast/src/functions/mod.rs Show resolved Hide resolved
compiler/ast/src/stub/finalize_stub.rs Show resolved Hide resolved
compiler/ast/src/types/type_.rs Outdated Show resolved Hide resolved
compiler/parser/src/parser/type_.rs Outdated Show resolved Hide resolved
tests/expectations/compiler/records/gates_is_allowed.out Outdated Show resolved Hide resolved
compiler/passes/src/common/graph/mod.rs Outdated Show resolved Hide resolved
compiler/passes/src/type_checking/mod.rs Show resolved Hide resolved
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 86.91099% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 77.06%. Comparing base (8056874) to head (88b57be).
Report is 2 commits behind head on testnet3.

Files Patch % Lines
compiler/passes/src/flattening/flatten_program.rs 64.28% 5 Missing ⚠️
compiler/ast/src/stub/function_stub.rs 73.33% 4 Missing ⚠️
compiler/passes/src/type_checking/checker.rs 89.47% 4 Missing ⚠️
compiler/ast/src/mapping/mod.rs 0.00% 3 Missing ⚠️
compiler/ast/src/stub/finalize_stub.rs 25.00% 3 Missing ⚠️
compiler/passes/src/common/graph/mod.rs 50.00% 3 Missing ⚠️
...mpiler/passes/src/code_generation/visit_program.rs 83.33% 2 Missing ⚠️
compiler/ast/src/types/type_.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           testnet3   #27902      +/-   ##
============================================
+ Coverage     76.95%   77.06%   +0.10%     
============================================
  Files           205      204       -1     
  Lines          7200     7158      -42     
  Branches       7200     7158      -42     
============================================
- Hits           5541     5516      -25     
+ Misses         1659     1642      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@evan-schott
Copy link
Collaborator Author

evan-schott commented Apr 18, 2024

@d0cd

do we need all of the additional clones?

get_output_type() and Output::type_() cloned them in previous version, so these are not additional clones.

why is does the program name need to be removed?

I initially made this choice to allow the ST to compare struct equality using eq_flat. Looking back, since I had to define eq_flat_relax_struct in type_.rs anyways, it makes more sense to have all composites be labeled with the local program, even though through the lens of snarkVM structs are technically not attached to a program.

is see a number of files where the generated bytecode has changed. Is this expected?

All of these changes are because the records and structs defined are unused. I am going to change the tests so that the composites are used, since the point of those tests is to test behavior of composites. Another possibility is to only remove unused structs when DCE is on. Or to default remove redefinitions of unused external structs, and adhere to the DCE flag otherwise.

@d0cd d0cd self-requested a review April 18, 2024 23:07
Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work!

@d0cd d0cd merged commit 95ee4a4 into testnet3 Apr 18, 2024
15 checks passed
@d0cd d0cd deleted the fix/external-struct branch April 18, 2024 23:08
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.

[Bug] Refactor TYC equality checking
2 participants