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

Dynamic array index assignment #1007

Merged
merged 6 commits into from
Jun 4, 2021
Merged

Dynamic array index assignment #1007

merged 6 commits into from
Jun 4, 2021

Conversation

Protryon
Copy link
Contributor

@Protryon Protryon commented Jun 4, 2021

This PR adds input-indexed array assignment. It covers all arrays and indirect usage of arrays (i.e. myArray[i].1 = X). It does NOT cover input-indexed slice assignment (should be done in another PR).

@Protryon Protryon requested review from damirka, collinc97 and acoglio and removed request for damirka June 4, 2021 12:56
@Protryon Protryon force-pushed the dyn-array-index-assignment branch from bb2ac10 to dc91b07 Compare June 4, 2021 13:16
@codecov
Copy link

codecov bot commented Jun 4, 2021

Codecov Report

Merging #1007 (5cd05d7) into master (c67ef6a) will decrease coverage by 0.00%.
The diff coverage is 58.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1007      +/-   ##
==========================================
- Coverage   75.35%   75.35%   -0.01%     
==========================================
  Files         435      438       +3     
  Lines       17567    17586      +19     
==========================================
+ Hits        13238    13252      +14     
- Misses       4329     4334       +5     
Impacted Files Coverage Δ
compiler/src/expression/array/access.rs 22.31% <0.00%> (-0.77%) ⬇️
...piler/src/statement/assign/assignee/array_index.rs 29.54% <29.54%> (ø)
compiler/src/statement/assign/assignee/member.rs 60.00% <60.00%> (ø)
...src/statement/assign/assignee/array_range_index.rs 60.37% <60.37%> (ø)
compiler/src/statement/assign/assignee/tuple.rs 61.53% <61.53%> (ø)
compiler/src/function/mut_target.rs 57.14% <62.50%> (+24.88%) ⬆️
compiler/src/statement/assign/assignee/mod.rs 89.36% <89.36%> (ø)
asg/src/statement/assign.rs 72.78% <100.00%> (ø)
ast/src/reducer/canonicalization.rs 72.87% <100.00%> (ø)
ast/src/reducer/reconstructing_reducer.rs 85.93% <100.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c67ef6a...5cd05d7. Read the comment docs.

@damirka
Copy link
Contributor

damirka commented Jun 4, 2021

@Protryon I have run this branch over code sample from #935. It runs build and setup successfully, but proof is invalid.

 Verifying Starting...
 Verifying Proof is invalid
      Done Finished in 71 milliseconds 

Copy link
Contributor

@damirka damirka left a comment

Choose a reason for hiding this comment

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

Looks good. Solves the issue.

Copy link
Collaborator

@acoglio acoglio left a comment

Choose a reason for hiding this comment

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

Looks good. Is the proof invalid issue brought up by @damirka solved by the later 'bound check' commit?

@Protryon
Copy link
Contributor Author

Protryon commented Jun 4, 2021

Looks good. Is the proof invalid issue brought up by @damirka solved by the later 'bound check' commit?

No, we confirmed that the case of indexing directly on input is fine, but the ternary causes proof failure and I haven't figured out why.

Copy link
Collaborator

@collinc97 collinc97 left a comment

Choose a reason for hiding this comment

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

Thank you for implementing @Protryon

The new assignee resolver looks great and is leaps and bounds better than the previous approach.

Codecov is detecting a lot of uncovered lines in array_index.rs and array_range_index.rs does this PR have tests to address all cases in those methods?

@acoglio acoglio merged commit e1b59a2 into master Jun 4, 2021
@acoglio acoglio deleted the dyn-array-index-assignment branch June 4, 2021 19:10
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.

4 participants