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

Fixes to isFunctionCallExternal() after end-to-end test #215

Merged
merged 6 commits into from
Jul 13, 2023

Conversation

blitz-1306
Copy link
Contributor

@blitz-1306 blitz-1306 commented Jun 16, 2023

Preface

Test of pending release against Scribble uncovered additional edge-cases for external call detection:

  • We sometimes may have ContractType.method() member accesses for public functions, where ContractType is one of the inheritance bases. Some of those calls are invoked internally. Example is here.
  • FunctionLikeSetTypes are now yet handled, where we have various possible functions via overloading. Example is here.

This PR makes attempts to address these issues. Related Scribble tuning PR is here: Consensys/scribble#232

Changes

  • Reintroduce isFunctionCallExternal() as method of InferType.
  • Improved detection of calls in external context.
  • Added relevant cases to a test sample.

Notes

This is a breaking change (previous PR #213 was also breaking due to signature change), so pending release would have to bump major version of the package to avoid breaking dependencies.

Regards.

…detection of calls in external context. Added relevant cases to a test sample.
@blitz-1306 blitz-1306 added bug Something isn't working breaking change Changes that would cause a backward compatibility break labels Jun 16, 2023
@blitz-1306 blitz-1306 requested a review from cd1m0 June 16, 2023 13:31
src/types/infer.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2023

Codecov Report

Merging #215 (4ea38d2) into master (18444a4) will increase coverage by 0.08%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   91.65%   91.73%   +0.08%     
==========================================
  Files         267      267              
  Lines        6527     6549      +22     
  Branches     1307     1320      +13     
==========================================
+ Hits         5982     6008      +26     
+ Misses        289      287       -2     
+ Partials      256      254       -2     
Impacted Files Coverage Δ
src/types/infer.ts 82.59% <92.10%> (+0.59%) ⬆️
src/types/utils.ts 88.10% <100.00%> (+0.10%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@cd1m0 cd1m0 left a comment

Choose a reason for hiding this comment

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

One question, some nits. Otherwise looking decent!

src/types/infer.ts Show resolved Hide resolved
src/types/infer.ts Show resolved Hide resolved
test/integration/types/external_calls.spec.ts Show resolved Hide resolved
@blitz-1306 blitz-1306 changed the title Fixes to isFunctionCallExternal() afted end-to-end test Fixes to isFunctionCallExternal() after end-to-end test Jul 11, 2023
@blitz-1306 blitz-1306 requested a review from cd1m0 July 12, 2023 11:03
@cd1m0 cd1m0 merged commit c8e68fe into master Jul 13, 2023
2 checks passed
@cd1m0 cd1m0 deleted the fix-is-fc-external branch July 13, 2023 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that would cause a backward compatibility break bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants