Feat/string manipulation - #18
Conversation
Not up to standards ⛔🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 228 |
| Duplication | 264 |
🔴 Coverage 78.26% diff coverage · -1.16% coverage variation
Metric Results Coverage variation ❌ -1.16% coverage variation (-1.00%) Diff coverage ✅ 78.26% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (04c31f7) Report Missing Report Missing Report Missing Head commit (69853ff) 2614 (-463) 2486 (-476) 95.10% (-1.16%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#18) 92 72 78.26% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…nd multi declarations
…d empty analysis)
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR is currently not up to standards due to significant logic flaws in the new 'code_analysis' system and a regression in error handling robustness. While the PR successfully introduces string/character handling at the semantic level, the accompanying refactor of the branch analysis engine contains critical bugs. Specifically, 'if' and 'elif' branches are entirely skipped during reachability and return-path validation if a final 'else' block is missing. This means dead code and missing return statements in these branches will go undetected.
Furthermore, the code quality has been impacted by the replacement of the '?' operator with '.unwrap()' in several fallible semantic inference calls, which will cause the compiler to panic on invalid user code rather than reporting a 'GoldError'. Coverage for the new complex logic in 'src/semantic/branch_analysis.rs' is also insufficient, with a 23% drop in coverage for that file. These issues, combined with excessive CI timeouts, must be addressed before merging.
About this PR
- Systemic regression in error handling: several fallible semantic calls previously using the '?' operator have been converted to '.unwrap()'. This converts catchable semantic errors into unrecoverable compiler panics.
- The PR title focuses on 'string manipulation', but the bulk of the changes constitute a significant refactor of the semantic analysis and branch validation layers. Ensure that the documentation and title reflect the scope of the architectural changes to the branch analysis system.
Test suggestions
- Detect unreachable code following a return statement in a function body
- Ensure 'If-Elif-Else' chains are validated to confirm every path returns in a non-void function
- Detect empty branches in 'Infinite', 'While', 'For', and 'If' blocks
- Prevent a variable from being declared with the same name as an existing function or local variable
- Verify that 'Break' statements inside an infinite loop that serves as a function's terminal path are prohibited
- Automatic length inference for string literals during variable declaration
- Unit tests for complex 'if-elif-else' structures where one 'elif' branch is missing a return to verify aggregation logic
- Recursive test scenarios for functions ending in 'infinite' loops containing terminal returns or breaks
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Ensure 'If-Elif-Else' chains are validated to confirm every path returns in a non-void function
2. Verify that 'Break' statements inside an infinite loop that serves as a function's terminal path are prohibited
3. Unit tests for complex 'if-elif-else' structures where one 'elif' branch is missing a return to verify aggregation logic
4. Recursive test scenarios for functions ending in 'infinite' loops containing terminal returns or breaks
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| fn return_branch_analysis_hazmat_wrapper( | ||
| func: &Function | ||
| ) -> Result<(), GoldError> { | ||
| fn return_branch_analysis_hazmat( |
There was a problem hiding this comment.
🔴 HIGH RISK
The refactored recursive return analysis logic in return_branch_analysis_hazmat is largely uncovered. This includes specific handling for infinite loops, while loops, and for loops at the end of functions. Given that the complexity of this file increased by 11, these uncovered recursive paths represent a significant risk for the stability of the semantic analysis layer.
| // If this is a nested loop, like a while loop inside a `infinite` loop, we let you do | ||
| // that. if in_loop is true, it might not be last statement after all. | ||
| // | ||
| certain_return_detected = if_branch_returns && elif_branches_returns && else_branch_returns; |
There was a problem hiding this comment.
🔴 HIGH RISK
Critical logic for multi-branch return and stop analysis is currently uncovered. The loop traversing elif branches and the final aggregation of certain_return_detected (which determines if a function correctly returns) lacks test coverage. This is high-risk logic where off-by-one or logic errors in boolean aggregation could lead to the compiler incorrectly accepting functions that do not return in all paths.
Try running the following prompt in your IDE agent:
Create unit tests in
src/semantic/branch_analysis/branch_analysis_tests.rsthat exercisecode_analysisusing functions containing complexif-elif-elsestructures. Specifically, test a case where all branches return, and another where oneelifbranch is missing a return, to verify thecertain_return_detectedlogic at line 189.
| Stmt::Return(_) => certain_return_detected = true, | ||
| Stmt::Break(_) | Stmt::Continue(_) => certain_stop_detected = true, | ||
| Stmt::Infinite(inf_stmt) => (certain_return_detected, _) = unreachable_code_branch_analysis_hazmat(&inf_stmt.branch)?, | ||
| Stmt::If(if_stmt) => if let Some(else_branch) = &if_stmt.else_branch { |
There was a problem hiding this comment.
🔴 HIGH RISK
Unreachable code analysis is skipped for if and elif branches when an else branch is missing. This pass should always recurse into all branches regardless of the presence of an else block to ensure internal dead code is caught.
Try running the following prompt in your coding agent:
In
src/semantic/branch_analysis.rs, refactorunreachable_code_branch_analysis_hazmatto recursively analyzeif_stmt.if_branchand each branch inif_stmt.elif_branchesbefore evaluating the presence of anelse_branchfor the purpose of determining block-level reachability.
|
|
||
| for s in &mut if_stmt.elif_branches { | ||
| let elif_expr_ty = infer::infer_expr_type(&mut s.0, locals, fun_sigs, Some(Type::Bool))?; | ||
| let elif_expr_ty = infer::infer_expr_type(&mut s.0, locals, fun_sigs, None).unwrap(); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Propagate the error using '?' instead of calling '.unwrap()' to allow the semantic layer to report invalid expressions in 'elif' conditions.
|
|
||
| Stmt::If(if_stmt) => { | ||
| let main_expr_ty = infer::infer_expr_type(&mut if_stmt.condition, locals, fun_sigs, Some(Type::Bool))?; | ||
| let main_expr_ty = infer::infer_expr_type(&mut if_stmt.condition, locals, fun_sigs, None).unwrap(); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Replace '.unwrap()' with '?' to ensure that type mismatches or reference errors in the 'if' condition are handled as semantic errors rather than compiler panics.
|
|
||
| Stmt::While(while_stmt) => { | ||
| let expr_ty = infer::infer_expr_type(&mut while_stmt.condition, locals, fun_sigs, Some(Type::Bool))?; | ||
| let expr_ty = infer::infer_expr_type(&mut while_stmt.condition, locals, fun_sigs, None).unwrap(); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Usage of '.unwrap()' here will cause the compiler to panic on semantic errors (such as undeclared variables in conditions) instead of returning a GoldError. Use '?' to maintain proper error propagation as seen in the original implementation.
|
|
||
| - name: Run unit tests and generate tarpaulin report | ||
| run: cargo tarpaulin --timeout 30000 --verbose --lib --out Xml --output-dir ./coverage | ||
| run: cargo tarpaulin --timeout 80000 --verbose --lib --out Xml --output-dir ./coverage |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The Tarpaulin timeout value of 80,000 is excessive (22+ hours). Use a more reasonable value like 300 seconds.
| run: cargo tarpaulin --timeout 80000 --verbose --lib --out Xml --output-dir ./coverage | |
| run: cargo tarpaulin --timeout 300 --verbose --lib --out Xml --output-dir ./coverage |
| let src = locals.get_mut(src_name).unwrap_or_else(|| panic!( | ||
| "(Compiler bug) infer_expr_type should've already errored if source variable didnt exist, but it didnt. var: {var:?}" | ||
| )); | ||
| let src = locals.get_mut(src_name).unwrap(); |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Replacing descriptive panic messages with raw .unwrap() makes it significantly harder to diagnose compiler bugs. Even if infer_expr_type is expected to catch these issues, using .expect() with a descriptive message is preferred for internal consistency.
| let src = locals.get_mut(src_name).unwrap(); | |
| let src = locals.get_mut(src_name).expect("Compiler bug: variable should exist in locals after successful type inference"); |
There was a problem hiding this comment.
expect is slow, and i use unwrap in some cases instread of custom compiler error messages because the tradeoff of custom message versus branch / region test coverage metric being off / never fully satisifed because of a impossible function call, is not worth it.
1 participant
This PR adds support for string manipulation.
This doesn't add new AST nodes for string manipulation, I re-use array access and array slicing, because technically speaking, strings are arrays at lowest level (.. and I also couldn't figure a cleaner way to separate array access/slicing with string manipulation, it would've complicated the AST for no real design benefits.).
This PR also rewrites branch analysis, and writes a ton of tests for it. empty branch analysis, unreachable code branch analysis, and return branch analysis
And as usual, the PR requests accompanies new unit-tests for char type, string manipulation, and misc bug fixes in general around parser, semantics, transpiler, etc.