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

feat[tool]: separate import resolution pass #4229

Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Sep 8, 2024

What I did

separate import resolution into its own pass

How I did it

How to verify it

Commit message

this commit separates import resolution into its own pass. there are
two reasons for this:

a) improved performance on certain operations, specifically,
computation of the integrity hash. this is a hotspot for tooling, which
needs to compute the integrity hash of a contract quickly to check if
it needs to be recompiled or not

b) logical separation. after this commit, all the I/O and filepath
computation of the module analysis is moved into its own pass, making
the module analyzer "pure" - it no longer needs to deal about input
bundles or filepaths or anything like that.

misc/refactor: this commit moves integrity hash computation into
a compiler pass. besides that, some some minor code touchups were
performed, but because of moving code into a new file, an effort
was made to keep the semantics of the code largely the same, to ease
reasoning about the diff.

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

hint = "try renaming `vyper.interfaces` to `ethereum.ercs`"

# copy search_paths, makes debugging a bit easier
search_paths = self.input_bundle.search_paths.copy() # noqa: F841

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable search_paths is not used.
vyper/semantics/analysis/module.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Sep 8, 2024

Codecov Report

Attention: Patch coverage is 39.49580% with 144 lines in your changes missing coverage. Please review.

Project coverage is 48.73%. Comparing base (9a208a6) to head (0716f0a).

Files with missing lines Patch % Lines
vyper/semantics/analysis/imports.py 35.48% 114 Missing and 6 partials ⚠️
vyper/semantics/analysis/module.py 27.58% 21 Missing ⚠️
vyper/semantics/analysis/base.py 75.00% 2 Missing ⚠️
vyper/compiler/output_bundle.py 50.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (9a208a6) and HEAD (0716f0a). Click for more details.

HEAD has 127 uploads less than BASE
Flag BASE (9a208a6) HEAD (0716f0a)
140 13
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4229       +/-   ##
===========================================
- Coverage   91.35%   48.73%   -42.62%     
===========================================
  Files         109      109               
  Lines       15637    15692       +55     
  Branches     3443     3452        +9     
===========================================
- Hits        14285     7648     -6637     
- Misses        920     7438     +6518     
- Partials      432      606      +174     

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

@vyperlang vyperlang deleted a comment from dimmuborgir666 Oct 5, 2024
@charles-cooper charles-cooper marked this pull request as ready for review October 5, 2024 18:46

# load an InterfaceT or ModuleInfo from an import.
# raises FileNotFoundError
def _load_import(self, node: vy_ast.VyperNode, level: int, module_str: str, alias: str) -> Any:
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we name module_str as qualified_name (or qualified_module_name), which better conveys the semantics and is consistent with the previous naming?

@cyberthirst
Copy link
Collaborator

contains bugs as per #4268. we probably should merge and fix within the mentioned PR

@cyberthirst
Copy link
Collaborator

no new tests - i see that we mainly shuffled code around, but we should carefully analyze coverage before merging (we should fix the rate limits for codecov while we're at it)

@cyberthirst
Copy link
Collaborator

cyberthirst commented Oct 11, 2024

since codecov isn't working properly, I went over the coverage locally:

edit: fixed link

@charles-cooper charles-cooper enabled auto-merge (squash) October 14, 2024 13:53
@charles-cooper charles-cooper merged commit 6843e79 into vyperlang:master Oct 14, 2024
156 checks passed
@charles-cooper charles-cooper deleted the feat/import-resolution-pass branch October 14, 2024 14:02
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.

2 participants