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

Add static type sanity check for imported values #1048

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Jul 5, 2021

Work towards #870

Description

Add a check to ensure the inferred type for arrays/ditionaries is always present and is valid.

Ideally, this check should never fail. But this is more of a second-line of defense, to make sure no value is passed into a program without type info / with incorrect type info.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@SupunS SupunS requested a review from turbolent as a code owner July 5, 2021 08:13
@SupunS SupunS force-pushed the supun/ensure-type branch from 08bf23f to b33473b Compare July 5, 2021 08:15
@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2021

Codecov Report

Merging #1048 (b33473b) into feature/container-static-types (3cfe0f1) will increase coverage by 0.09%.
The diff coverage is 90.47%.

Impacted file tree graph

@@                        Coverage Diff                         @@
##           feature/container-static-types    #1048      +/-   ##
==================================================================
+ Coverage                           73.47%   73.57%   +0.09%     
==================================================================
  Files                                 271      271              
  Lines                               34003    34024      +21     
==================================================================
+ Hits                                24984    25033      +49     
+ Misses                               7882     7853      -29     
- Partials                             1137     1138       +1     
Flag Coverage Δ
unittests 73.57% <90.47%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/runtime.go 86.70% <90.47%> (+0.05%) ⬆️
runtime/interpreter/value.go 78.41% <0.00%> (+0.81%) ⬆️

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 3cfe0f1...b33473b. Read the comment docs.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -715,12 +715,49 @@ func validateArgumentParams(
}
}

// Ensure static type info is available for all values
interpreter.InspectValue(arg, func(value interpreter.Value) bool {
Copy link
Member

Choose a reason for hiding this comment

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

👌

@SupunS SupunS merged commit 9312f2f into feature/container-static-types Jul 5, 2021
@SupunS SupunS deleted the supun/ensure-type branch July 5, 2021 16:55
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.

3 participants