Skip to content

fix[ux]: improve error message on failed imports#4409

Merged
charles-cooper merged 8 commits intovyperlang:masterfrom
sandbubbles:fix/add-node-info-to-module-not-found
Jan 3, 2025
Merged

fix[ux]: improve error message on failed imports#4409
charles-cooper merged 8 commits intovyperlang:masterfrom
sandbubbles:fix/add-node-info-to-module-not-found

Conversation

@sandbubbles
Copy link
Copy Markdown
Contributor

@sandbubbles sandbubbles commented Dec 18, 2024 •
edited by charles-cooper
Loading

What I did

Report file and line of an incorrect import as requested by #4408.

How I did it

Simply added the node as argument to ModuleNotFound.

How to verify it

Test the error message contains the correct file.

Commit message

Previously, when an import failed, the error message only displayed
the paths that were attempted, but did not point to the specific import
statement which caused the exception.

To address this, we wrap the main node-handling loop in
`ImportAnalyzer` with `tag_exception`, which propagates information
with the specific line and file to the error message.

Description for the changelog

Cute Animal Picture

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

@sandbubbles
Copy link
Copy Markdown
Contributor Author

sandbubbles commented Dec 18, 2024

should we add the node argument to _load_builtin_import too?

@charles-cooper
Copy link
Copy Markdown
Member

charles-cooper commented Dec 18, 2024

should we add the node argument to _load_builtin_import too?

this means if they try to import something like from ethereum.ercs import invalid_module we report the line correctly right? if so, then yes. although honestly we should maybe have a catchall exception handler which adds node information if missing, as is done here:

vyper/vyper/codegen/expr.py

Line 87 in 296a071

with tag_exceptions(node, fallback_exception_type=CodegenPanic, note=fn_name):

@sandbubbles
Copy link
Copy Markdown
Contributor Author

sandbubbles commented Dec 18, 2024

should we add the node argument to _load_builtin_import too?

this means if they try to import something like from ethereum.ercs import invalid_module we report the line correctly right? if so, then yes. although honestly we should maybe have a catchall exception handler which adds node information if missing, as is done here:

vyper/vyper/codegen/expr.py

Line 87 in 296a071

with tag_exceptions(node, fallback_exception_type=CodegenPanic, note=fn_name):

yes, right now _load_builtin_import does not contain node but when added it as its parameter to use in the ModuleNotFound call nothing seems to break, all tests are fine. But the tag_exceptions would be probably nicer as we would not have to change the signature of the _load_builtin_import

@cyberthirst cyberthirst added the release - must release blocker label Dec 27, 2024
charles-cooper
charles-cooper reviewed Dec 30, 2024
View reviewed changes
vyper/semantics/analysis/imports.py Outdated
node._metadata["import_info"] = ImportInfo(
alias, qualified_module_name, compiler_input, ast
)
with tag_exceptions(node):
Copy link
Copy Markdown
Member

@charles-cooper charles-cooper Dec 30, 2024

Choose a reason for hiding this comment

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

let's move this to the highest scope where we have access to node -- i think inside the loop body on line 108

charles-cooper
charles-cooper approved these changes Jan 2, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

lgtm!

@charles-cooper charles-cooper enabled auto-merge (squash) January 3, 2025 15:32
@charles-cooper charles-cooper merged commit 4507d2a into vyperlang:master Jan 3, 2025
@pcaversaccio pcaversaccio mentioned this pull request Jan 3, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charles-cooper charles-cooper charles-cooper approved these changes

@cyberthirst cyberthirst cyberthirst approved these changes

Assignees

No one assigned

Labels

release - must release blocker

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sandbubbles @charles-cooper @cyberthirst