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

Use native compilers for AST building #143

Merged
merged 13 commits into from
Feb 22, 2022
Merged

Use native compilers for AST building #143

merged 13 commits into from
Feb 22, 2022

Conversation

blitz-1306
Copy link
Contributor

@blitz-1306 blitz-1306 commented Feb 3, 2022

Preface

This PR allows to use native Solidity compilers to build and process AST.

Changes

  • Updated dependencies to latest.
  • Use native compilers instead of WASM for AST building and processing.
  • Introduced compiler kind configuring via CLI option --compiler-kind and environment variable SCRIBBLE_DEFAULT_COMPILER_KIND. Precedence is following:
    • --compiler-kind is primary;
    • SCRIBBLE_DEFAULT_COMPILER_KIND is secondary;
    • If getCompilerPrefixForOs() result is one of linux-amd64, windows-amd64 or macosx-amd64, then CompilerKind.Native. Otherwise the fallback value is CompilerKind.WASM.
  • Tweaked executable and tests to be compatible with async execution.

Notes

  • It seems that solc-typed-ast v8.0.0 adds ./ prefix to all relative input paths, including entry file path. Such behavior affected a lot of artifacts and snapshots, so it probably would also affect dependent components. We better to assess possible implications of this change and consider if it worth fixing in the solc-typed-ast itself.

Regards.

…for AST building and processing. Tweaked executable and tests to be compatible with async execution.
@blitz-1306 blitz-1306 added the enhancement New feature or request label Feb 3, 2022
@blitz-1306 blitz-1306 requested a review from cd1m0 February 3, 2022 07:43
Copy link
Collaborator

@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.

Just 1 question and 2 nits. Will fix then its shortly

src/bin/scribble.ts Outdated Show resolved Hide resolved
src/bin/scribble.ts Outdated Show resolved Hide resolved
test/unit/custom_maps.spec.ts Outdated Show resolved Hide resolved
@cd1m0 cd1m0 self-requested a review February 8, 2022 02:35
cd1m0
cd1m0 previously approved these changes Feb 8, 2022
Copy link
Collaborator

@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.

lgtm

@blitz-1306 blitz-1306 changed the title [WIP] Use native compilers Use native compilers for AST building Feb 14, 2022
@cd1m0 cd1m0 self-requested a review February 15, 2022 07:41
Copy link
Collaborator

@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.

After discussion with @blitz-1306 we decided to make the logic for selecting default compiler kind a little more robust. Specifically the selection logic should be:

  1. If there is --compiler-kind specified use that
  2. Otherwise if there is an env variable SCRIBBLE_DEFAULT_COMPILER_KIND, then use that
  3. Finally if no options are given use CompilerKind.Native IF the return value of getCompilerPrefixForOs() is one of the 3 supported platforms - "linux-amd64", "windows-amd64" and "macosx-amd64". Otherwise we use CompilerKind.WASM.

Copy link
Collaborator

@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.

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2022

Codecov Report

Merging #143 (7229248) into develop (4897e78) will increase coverage by 0.02%.
The diff coverage is 76.30%.

❗ Current head 7229248 differs from pull request most recent head 3c236bb. Consider uploading reports for the commit 3c236bb to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #143      +/-   ##
===========================================
+ Coverage    86.68%   86.70%   +0.02%     
===========================================
  Files           72       72              
  Lines         5203     5190      -13     
  Branches      1192     1191       -1     
===========================================
- Hits          4510     4500      -10     
+ Misses         420      417       -3     
  Partials       273      273              
Impacted Files Coverage Δ
src/bin/scribble.ts 78.11% <75.49%> (+0.22%) ⬆️
src/rewriter/merge.ts 89.58% <100.00%> (-0.08%) ⬇️
src/spec-lang/tc/semcheck.ts 85.58% <100.00%> (+0.06%) ⬆️
src/instrumenter/utils.ts 73.80% <0.00%> (-0.61%) ⬇️
src/instrumenter/state_vars.ts 88.23% <0.00%> (-0.15%) ⬇️
src/instrumenter/instrument.ts 94.55% <0.00%> (-0.11%) ⬇️
src/spec-lang/tc/typecheck.ts 82.67% <0.00%> (-0.10%) ⬇️
src/instrumenter/custom_maps.ts 84.79% <0.00%> (-0.07%) ⬇️
src/instrumenter/annotations.ts 86.25% <0.00%> (-0.07%) ⬇️
... and 7 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 f956f2a...3c236bb. Read the comment docs.

@cd1m0 cd1m0 merged commit 4221622 into develop Feb 22, 2022
@cd1m0 cd1m0 deleted the use-native-compilers branch February 22, 2022 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants