[hermes] Add source code transformation #3004
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
joshlf
force-pushed
the
G1bd8ca80c7b97b4c799cec1504d281ae79f329b1
branch
from
cd76106 to
cdbf265
Compare
joshlf
force-pushed
the
G9611b9a3ca0254d8ba488d591e6a21980cca3d29
branch
from
3c5c8f4 to
d92de78
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## G1bd8ca80c7b97b4c799cec1504d281ae79f329b1 #3004 +/- ##
==========================================================================
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 |
![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 introduces source code transformation capabilities to the hermes tool. The main changes involve adding a transform module to modify the source code of unsafe functions, and refactoring the parsing logic to accommodate this. The transformed code is not yet utilized, which seems to be planned for future work. My review focuses on improving error handling and fixing a bug in the UI test shim's error reporting.
| F: FnMut(&str, Result<ParsedLeanItem, HermesError>), | ||
| { | ||
| visit_hermes_items_internal(source, Some(path.to_path_buf()), f) | ||
| let source = fs::read_to_string(path).expect("Failed to read file"); |
There was a problem hiding this comment.
Using expect here will cause a panic if the file cannot be read. The function signature -> Result<String, io::Error> suggests that the error should be propagated. Using the ? operator would be more appropriate and align with the function's contract.
| let source = fs::read_to_string(path).expect("Failed to read file"); | |
| let source = fs::read_to_string(path)?; |
| parse::read_file_and_visit_hermes_items(&file_path, |source, res| { | ||
| if let Err(e) = res { | ||
| has_errors = true; | ||
| emit_rustc_json(&e, &source, file_path.to_str().unwrap()); |
There was a problem hiding this comment.
The source variable from the closure is a slice of the source code for the specific item, not the entire file. emit_rustc_json needs the full file source to calculate correct line/column numbers. This will lead to incorrect error locations in UI tests. You should pass the full file source to emit_rustc_json. One way to achieve this is to collect errors in the closure and process them later when you have access to the full source string returned by read_file_and_visit_hermes_items.
| let mut source = source.into_bytes(); | ||
| transform::apply_edits(&mut source, &edits); |
There was a problem hiding this comment.
The transformed source buffer is created but not used. I assume the intention is to either print it to stdout or write it to a file, which will be implemented in a subsequent change. Could you confirm if this is the case?
| } | ||
| }); | ||
| }) | ||
| .unwrap(); |
There was a problem hiding this comment.
Using unwrap() here will cause a panic if read_file_and_visit_hermes_items returns an io::Error (e.g., if the file doesn't exist). The previous implementation was more robust with unwrap_or_default(). Please handle the Err case gracefully, for example by printing an error and exiting with a non-zero status code.
joshlf
force-pushed
the
G1bd8ca80c7b97b4c799cec1504d281ae79f329b1
branch
from
cdbf265 to
a113bca
Compare
joshlf
force-pushed
the
G9611b9a3ca0254d8ba488d591e6a21980cca3d29
branch
from
d92de78 to
777a0e4
Compare
gherrit-pr-id: G9611b9a3ca0254d8ba488d591e6a21980cca3d29
joshlf
force-pushed
the
G9611b9a3ca0254d8ba488d591e6a21980cca3d29
branch
from
777a0e4 to
3150404
Compare
2 participants
mod foo;declarations #3008Latest Update: v35 — Compare vs v34
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/G9611b9a3ca0254d8ba488d591e6a21980cca3d29 && git checkout -b pr-G9611b9a3ca0254d8ba488d591e6a21980cca3d29 FETCH_HEADCheckout
git fetch origin refs/heads/G9611b9a3ca0254d8ba488d591e6a21980cca3d29 && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/G9611b9a3ca0254d8ba488d591e6a21980cca3d29 && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.