Skip to content

Rework Diagnostics Folding#140

Merged
josephbirkner merged 5 commits intonoserdefrom
rework-diagnostics-folding
Mar 16, 2026
Merged

Rework Diagnostics Folding#140
josephbirkner merged 5 commits intonoserdefrom
rework-diagnostics-folding

Conversation

@johannes-wolf
Copy link
Collaborator

@johannes-wolf johannes-wolf commented Mar 11, 2026

Changes

  • Removed clone() from Expr.
  • Made eval and ieval const again (prior diagnostics state).
  • Diagnostics data is now stored in the diagnostics object instead of the AST nodes. We do not need to copy the whole AST prior evaluation anymore. Instead, AST nodes can query ctx.diag for a matching diagnostics data object.
  • Expr now contains a stable and unique id()
  • Moved ExprVisitor to its own files.

Because of the simplification of the diagnostics message generation, some corner-cases such as
or short-circuiting may produce no diagnostics for skipped branches and any could produce diagnostics even though another branch matched.


Note

Medium Risk
Touches core expression evaluation/parsing APIs (Expr becomes const-eval with stable ids, Context gains Diagnostics*) and rewires diagnostics collection/serialization, which could affect query evaluation results and diagnostic accuracy across complex expressions.

Overview
Reworks diagnostics collection to be external to the AST: expressions now have a stable ExprId, and runtime stats (field hits/evaluations and comparison operand types/results) are recorded via Context::diag into Diagnostics storage rather than mutating/cloning AST nodes.

Introduces a standalone recursive ExprVisitor (include/simfil/expression-visitor.h + src/expression-visitor.cpp) and updates all expressions, completion nodes, and the parser to be const-evaluated (eval/ieval) with accept(...) const. eval() now builds per-run diagnostics (prepareIndices + local merge) and diagnostics serialization format is updated accordingly; tests are adjusted to reflect the new message behavior and aggregation.

Written by Cursor Bugbot for commit e83f0d2. This will update automatically on new commits. Configure here.

@johannes-wolf johannes-wolf force-pushed the rework-diagnostics-folding branch from b852a96 to b1c70f7 Compare March 11, 2026 00:07
@johannes-wolf johannes-wolf changed the title Rework diagnostics folding Rework Diagnostics Folding Mar 11, 2026
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Free Tier Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Always-true condition bypasses type intersection check
    • Replaced the always-true guard with an intersection.none() check so diagnostics are emitted only for non-overlapping operand types.
  • ✅ Fixed: Non-portable unistd.h include in public header
    • Removed the unused POSIX-only <unistd.h> include from the public diagnostics header to restore portability.

Create PR

Or push these changes by commenting:

@cursor push 74200e836f
Preview (74200e836f)
diff --git a/include/simfil/diagnostics.h b/include/simfil/diagnostics.h
--- a/include/simfil/diagnostics.h
+++ b/include/simfil/diagnostics.h
@@ -9,7 +9,6 @@
 
 #include <tl/expected.hpp>
 #include <optional>
-#include <unistd.h>
 #include <vector>
 #include <string>
 #include <memory>

diff --git a/src/diagnostics.cpp b/src/diagnostics.cpp
--- a/src/diagnostics.cpp
+++ b/src/diagnostics.cpp
@@ -165,7 +165,7 @@
         }
 
         const auto intersection = leftTypes.flags & rightTypes.flags;
-        if (true || intersection.none()) {
+        if (intersection.none()) {
             const auto allTrue = data.trueResults > 0 && data.falseResults == 0;
             const auto allFalse = data.falseResults > 0 && data.trueResults == 0;
             const auto prefix =

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

@github-actions
Copy link

github-actions bot commented Mar 11, 2026

Test Results

 1 files  ±0   1 suites  ±0   6m 53s ⏱️ +2s
90 tests  - 1  90 ✅  - 1  0 💤 ±0  0 ❌ ±0 
95 runs   - 1  95 ✅  - 1  0 💤 ±0  0 ❌ ±0 

Results for commit 33c42ac. ± Comparison against base commit 9a3911b.

♻️ This comment has been updated with latest results.

@johannes-wolf johannes-wolf force-pushed the rework-diagnostics-folding branch from b1c70f7 to 7660bb4 Compare March 11, 2026 00:32
Also remove the unused clone() function.
@johannes-wolf johannes-wolf force-pushed the rework-diagnostics-folding branch from 7660bb4 to a52ceee Compare March 11, 2026 00:37
@johannes-wolf johannes-wolf force-pushed the rework-diagnostics-folding branch 2 times, most recently from 005f9ce to 33f0def Compare March 11, 2026 14:25
@johannes-wolf johannes-wolf force-pushed the rework-diagnostics-folding branch from 33f0def to 8b9395b Compare March 11, 2026 15:50
@johannes-wolf johannes-wolf force-pushed the rework-diagnostics-folding branch from 8b9395b to e83f0d2 Compare March 11, 2026 16:00
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@johannes-wolf johannes-wolf force-pushed the rework-diagnostics-folding branch from e83f0d2 to e305bb9 Compare March 11, 2026 16:52
@cursor
Copy link

cursor bot commented Mar 11, 2026

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 22.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

@johannes-wolf johannes-wolf force-pushed the rework-diagnostics-folding branch from e305bb9 to c6e04e3 Compare March 11, 2026 16:53
@cursor
Copy link

cursor bot commented Mar 11, 2026

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 22.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

@johannes-wolf johannes-wolf force-pushed the rework-diagnostics-folding branch from 14eec62 to 33c42ac Compare March 13, 2026 15:20
@sonarqubecloud
Copy link

@github-actions
Copy link

Package Line Rate Branch Rate Health
include.simfil 24% 10%
include.simfil.model 75% 46%
src 80% 47%
src.model 82% 46%
Summary 46% (7782 / 16915) 28% (4776 / 17361)

Base automatically changed from feature/split-field-storage to noserde March 16, 2026 10:17
Copy link
Collaborator

@josephbirkner josephbirkner left a comment

Choose a reason for hiding this comment

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

Well done 👍

@josephbirkner josephbirkner merged commit 1223db0 into noserde Mar 16, 2026
6 checks passed
@josephbirkner josephbirkner deleted the rework-diagnostics-folding branch March 16, 2026 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants