[hermes] Discover mod foo; declarations
#3008
Conversation
Summary of ChangesHello @joshlf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the parsing logic to discover mod foo; declarations, in addition to items with lean doc-blocks. The changes are well-structured, renaming several functions to better reflect their new purpose of scanning a full compilation unit. My review includes a critical fix for error handling where the program could panic instead of returning an error, a suggestion to improve code readability by flattening nested conditionals, and a note on an unused variable.
| where | ||
| F: FnMut(&str, Result<ParsedLeanItem, HermesError>), | ||
| { | ||
| let source = fs::read_to_string(path).expect("Failed to read file"); |
There was a problem hiding this comment.
The function is declared to return a Result<..., io::Error>, but .expect() will cause a panic on file read failure instead of propagating the io::Error. Using the ? operator will correctly propagate the error, aligning with the function's signature and preventing a potential crash.
| let source = fs::read_to_string(path).expect("Failed to read file"); | |
| let source = fs::read_to_string(path)?; |
| }); | ||
|
|
||
| let source = res.unwrap_or_else(|e| { | ||
| let (source, unloaded_modules) = res.unwrap_or_else(|e| { |
There was a problem hiding this comment.
The unloaded_modules variable is not used after being assigned. This will cause a compiler warning. If it's intended for use in a future change, consider prefixing it with an underscore to suppress the warning.
| let (source, unloaded_modules) = res.unwrap_or_else(|e| { | |
| let (source, _unloaded_modules) = res.unwrap_or_else(|e| { |
| for attr in attrs { | ||
| if attr.path().is_ident("path") { | ||
| if let Meta::NameValue(nv) = &attr.meta { | ||
| if let Expr::Lit(expr_lit) = &nv.value { | ||
| if let Lit::Str(lit_str) = &expr_lit.lit { | ||
| return Some(lit_str.value()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The nested if let statements make the code harder to read. This can be refactored using let-else statements to flatten the structure and improve readability.
for attr in attrs {
if !attr.path().is_ident("path") {
continue;
}
let Meta::NameValue(nv) = &attr.meta else { continue; };
let Expr::Lit(expr_lit) = &nv.value else { continue; };
let Lit::Str(lit_str) = &expr_lit.lit else { continue; };
return Some(lit_str.value());
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a #3008 +/- ##
==========================================================================
Coverage 91.87% 91.87%
==========================================================================
Files 20 20
Lines 6057 6057
==========================================================================
Hits 5565 5565
Misses 492 492 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the capability to discover mod foo; declarations within Rust source files. This is a key step towards understanding the full module structure of a crate. The implementation involves renaming several parsing functions for better clarity, enhancing the HermesVisitor to identify module declarations without a body, and introducing a new UnloadedModule struct. The changes are logical and well-executed. I have a couple of minor suggestions to handle unused variables, which will prevent compiler warnings.
| }); | ||
|
|
||
| let source = res.unwrap_or_else(|e| { | ||
| let (source, unloaded_modules) = res.unwrap_or_else(|e| { |
There was a problem hiding this comment.
The unloaded_modules variable is captured but not used within this function, which will trigger a compiler warning. Since this is likely intended for use in a future change in this PR stack, you can prefix the variable with an underscore to indicate it's intentionally unused for now.
| let (source, unloaded_modules) = res.unwrap_or_else(|e| { | |
| let (source, _unloaded_modules) = res.unwrap_or_else(|e| { |
joshlf
force-pushed
the
G93b59d9ea956b7e2e06b2e91c45dea803a391801
branch
from
ab89fe4 to
176be07
Compare
joshlf
force-pushed
the
G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a
branch
from
7ee6fde to
1f72412
Compare
joshlf
force-pushed
the
G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a
branch
from
1f72412 to
4d110ea
Compare
joshlf
force-pushed
the
G93b59d9ea956b7e2e06b2e91c45dea803a391801
branch
from
176be07 to
030bb4e
Compare
gherrit-pr-id: G93b59d9ea956b7e2e06b2e91c45dea803a391801
joshlf
force-pushed
the
G93b59d9ea956b7e2e06b2e91c45dea803a391801
branch
from
030bb4e to
b5601de
Compare
joshlf
force-pushed
the
G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a
branch
from
4d110ea to
755b70a
Compare
2 participants
mod foo;declarations #3008Latest Update: v33 — Compare vs v32
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/G93b59d9ea956b7e2e06b2e91c45dea803a391801 && git checkout -b pr-G93b59d9ea956b7e2e06b2e91c45dea803a391801 FETCH_HEADCheckout
git fetch origin refs/heads/G93b59d9ea956b7e2e06b2e91c45dea803a391801 && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/G93b59d9ea956b7e2e06b2e91c45dea803a391801 && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.