[WIP] #3049
[WIP] #3049
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 refactors the internal representation of parsed Rust items within 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
Activity
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
The pull request introduces a refactoring of the parsing logic to support Hermes-specific attributes. However, the current state of the code contains several critical issues that prevent it from compiling and adhering to the project's guidelines. Specifically, there are mismatches between function signatures and their call sites, logic errors where closure results are discarded, and type mismatches in enum variant construction. Furthermore, the use of todo! macros and TODO comments violates the repository's coding guidelines (as specified in AGENTS.md) and will cause CI failures.
| fn process_item< | ||
| T, | ||
| F: FnOnce(T, Option<attr::HermesAttr>) -> Result<ParsedItem, HermesError>, | ||
| >( | ||
| &mut self, | ||
| item: T, | ||
| attrs: &[Attribute], | ||
| span: Span, | ||
| f: F, | ||
| ) { |
There was a problem hiding this comment.
The signature of process_item has been updated to include an attrs parameter, but all call sites (e.g., lines 377, 385, 393, etc.) are still passing only three arguments. This will result in a compilation error. Additionally, the generic parameter T lacks a trait bound that provides the attrs() method used on line 307.
| let attr = attr::parse_hermes_attr(attrs).unwrap(); | ||
| f(item, attr); |
There was a problem hiding this comment.
There are multiple issues here:
attr::parse_hermes_attris not defined in theattrmodule.- The use of
unwrap()will cause the tool to panic on malformed attributes; this should be handled gracefully. - The result of the closure
f(item, attr)is discarded, but it is needed to produce theParsedItemused later in the function.
| trace!("Visiting Fn {}", node.sig.ident); | ||
| self.process_item(ParsedItem::Fn(node.clone()), node.span()); | ||
| self.process_item(node.clone(), node.span(), |item, attr| { | ||
| Ok(ParsedItem::Function { item: FunctionItem::Free(item), attribute: attr }) |
There was a problem hiding this comment.
The construction of ParsedItem::Function is incorrect:
- The field name is
hermes, notattribute. - The type of
attr(likelyOption<HermesAttr>) does not match the expectedHermesBlock<HermesAttr>type defined for this variant.
| f: F, | ||
| ) -> Result<ParsedItem, HermesError> { | ||
| if attr.is_some() { | ||
| return Err(todo!("unsupported attribute on item")); |
There was a problem hiding this comment.
The use of the todo! macro will cause a runtime panic and violates the project's coding guidelines. According to AGENTS.md, TODO markers (including the todo! macro) should only be used if you explicitly intend to block the PR, as CI is configured to fail on them. Consider using a proper error variant or a FIXME if this is intended to be non-blocking.
References
- Guidelines in AGENTS.md prohibit the use of TODO comments/macros unless intended to block the PR, as they trigger CI failures. (link)
| UnsafeAxiom, | ||
| } | ||
|
|
||
| // TODO: We need to "unfold" this type – we unconditionally need `content`, |
There was a problem hiding this comment.
This TODO comment will cause CI to fail. Per the guidelines in AGENTS.md, please use FIXME for non-blocking issues.
| // TODO: We need to "unfold" this type – we unconditionally need `content`, | |
| // FIXME: We need to "unfold" this type – we unconditionally need `content`, |
References
- AGENTS.md specifies that TODO comments should be avoided as they block CI; FIXME should be used for non-blocking issues. (link)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## G4uato76xa2ezceilkolqtzoxhlxmxbg2 #3049 +/- ##
====================================================================
Coverage ? 91.87%
====================================================================
Files ? 20
Lines ? 6057
Branches ? 0
====================================================================
Hits ? 5565
Misses ? 492
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Squashed into #3051 |
2 participants
This PR is on branch hermes.
#[cfg_attr(..., path = "...")]#3045unsaferedaction #3034mod foo;declarations #3008Latest Update: v3 — Compare vs v2
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/Gr5hnlfzgkxvo5cdwir7s5hxfhlji6xjd && git checkout -b pr-Gr5hnlfzgkxvo5cdwir7s5hxfhlji6xjd FETCH_HEADCheckout
git fetch origin refs/heads/Gr5hnlfzgkxvo5cdwir7s5hxfhlji6xjd && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/Gr5hnlfzgkxvo5cdwir7s5hxfhlji6xjd && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.