Better handling around #if conditionals by JonatanWaern · Pull Request #227 · intel/dml-language-server

JonatanWaern · 2026-06-09T07:08:39Z

Refactor how we store object conds for objectdecls
Add logic to select between hashif branches

Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

This PR improves handling of #if/#else conditionals during analysis by introducing expression evaluation support and using existence conditions to avoid reporting conflicts across mutually exclusive branches.

Changes:

Added an evaluation module to evaluate a small subset of expressions for conditional existence checks.
Refactored ExistCondition storage to use Arc and added helpers (exists, guaranteed_exists, guaranteed_excluded_from) to reason about conditional branches.
Updated object/spec symbol collection and conflict detection to consider evaluated #if conditions and avoid conflicts between #if and corresponding #else.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
src/analysis/templating/objects.rs	Uses `ExistCondition` evaluation to filter specs/decls and refines conflict reporting to ignore provably exclusive branches
src/analysis/templating/mod.rs	Exposes the new `evaluation` module
src/analysis/templating/evaluation.rs	Introduces expression evaluation utilities used for `#if` condition resolution
src/analysis/structure/toplevel.rs	Stores conditional stacks in `Arc` and adds existence/exclusion helpers on `ExistCondition`
CHANGELOG.md	Documents improved conflict handling across `#if`/`#else` and built-in version-condition behavior

Suppressed comments (1)

src/analysis/structure/toplevel.rs:97

is_same can return true for different-length conditional stacks because zip() truncates to the shorter iterator (e.g., [A] vs [A,B] will return true). Add an explicit length equality check before the loop (or compare the full vectors) so only identical conditional stacks are treated as the same.

            (ExistCondition::Conditional(selfvec),
             ExistCondition::Conditional(othervec)) => {
                // Currently we cannt check if a condition is equivalent with another,
                // so we will only check if they are literally the same condition expression
                for ((_, cond1),
                     (_, cond2)) in selfvec.iter().zip(othervec.iter()) {
                    if cond1 != cond2 {
                        return false;
                    }
                }
                true
            },

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟡 Changes recommended

It contains a correctness issue in ExistCondition::guaranteed_overlaps plus several documentation/formatting problems in CHANGELOG that should be fixed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 5/5 changed files
Comments generated: 4
Review effort level: Lite

Copilot

🟡 Changes recommended

The new expression evaluation contains confirmed logic defects that can cause #if conditions to be treated incorrectly, undermining the PR’s core behavioral goal.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/analysis/structure/toplevel.rs:90

ExistCondition::guaranteed_overlaps uses zip() without checking that selfvec and othervec have the same length. That can return true when one condition list is a strict prefix of the other (e.g., nested #ifs), incorrectly treating conditions as guaranteed-overlapping and leading to missed/incorrect conflict filtering.

            (ExistCondition::Conditional(selfvec),
             ExistCondition::Conditional(othervec)) => {
                // TODO/NOTE: Currently we cannt check if a condition is equivalent with another,
                // so we will only check if they are literally the same condition expression
                for ((invert1, cond1),

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Lite

Copilot

🟡 Changes recommended

It introduces a correctness issue in ExistCondition::guaranteed_overlaps (prefix-length conditions can be misclassified as guaranteed overlapping), which should be fixed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

CHANGELOG.md:12

These changelog entries have awkward grammar ("properly not report", "branches on conditions"); tightening the wording will make the release notes clearer.

- The DLS will now properly not report conflicts between statements in a `#if` and its corresponding `#else` branch
- The DLS will now consider all `#if` branches on conditions directly based on `dml_1_2` and `dml_1_4` dead or alive appropriately

Files reviewed: 5/5 changed files
Comments generated: 4
Review effort level: Lite

Copilot

🟡 Changes recommended

Indeterminate expressions can select incorrect branches, and some mutually exclusive declaration sets still produce missed or false conflicts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

src/analysis/templating/evaluation.rs:223

false || unknown currently evaluates to false (and the symmetric case does too), so callers incorrectly discard the true #if branch. An unknown operand is only irrelevant when the other operand is known true; all other partially-known || cases must remain unknown.

                (Some(false), Some(false)) => (combine_constants(&[left.constant, right.constant]), Some(false)),
                (b, None) => (left.constant, b),
                (Some(true), _) => (left.constant, Some(true)),
                (_, b) => (right.constant, b),

src/analysis/templating/evaluation.rs:247

An unevaluable ternary condition falls into the else arm here, turning unknown ? true : false into a definite false. If such an expression guards a #if, this incorrectly removes the true branch; preserve an unknown result when the condition is unknown.

            crate::analysis::structure::expressions::TertiaryOp::Cond => 
                if left_evaluated.as_bool() == Some(true) {
                    middle_evaluated
                } else {
                    right_evaluated

src/analysis/templating/evaluation.rs:262

This also selects the right operand when the hash-conditional condition is unknown. That produces a definite branch result from an indeterminate condition and can incorrectly prune declarations guarded by an outer #if; return an empty result for None.

                if left_evaluated.as_bool() == Some(true) {
                    middle_evaluated
                } else {
                    right_evaluated
                }

src/analysis/templating/objects.rs:1639

Each symbol retains only the first declaration's condition. With one declaration in a true branch followed by two same-name declarations in its else branch, both later declarations compare only against the true-branch condition and are treated as excluded, so the real collision between the two else declarations is missed. Retain all prior (condition, span) entries and compare new declarations pairwise.

    let mut symbols: HashMap<String, (ExistCondition, ZeroSpan, Vec<ZeroSpan>)>
        = HashMap::new();

src/analysis/structure/toplevel.rs:106

Correct the typo in this explanatory comment.

                // Currently we cannt check if a condition is equivalent with another,

Files reviewed: 5/5 changed files
Comments generated: 3
Review effort level: Balanced

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved conditions can select incorrect branches, and some dead conditional declarations remain active.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 · 4

Open (6)

Preserve unknown results for OR operands · New
Honor conditions when recording template instantiations · New
Filter parameters by their own conditions · New
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…

Resolved since last review (8)

When one operand is unknown, true && unknown currently becomes true. ExistCondition::exists…
exists returns true for both sides of an unevaluable #if, so this loop still compares mutually…
Only declarations conflicting with the first source-ordered definition are checked. For `#if A {…
guaranteed_overlaps can incorrectly return true when one condition list is a strict prefix of the…
This comment is an unfinished sentence, which is confusing in the middle of a dense type/setup…
Grammar/wording in this comment is incorrect ("used in inferred", "existconditions"), which makes…
Typo in comment: "cannt" -> "cannot".
Typo in comment: 'cannt' -> 'cannot'.

Copilot

Copilot review overview

🟡 Changes recommended

Logical AND mishandles a decisive false operand, retaining dead conditional branches.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 4

Open (4)

Preserve decisive false in And expressions with unevaluable operands · New
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…

Resolved since last review (3)

Honor conditions when recording template instantiations
Preserve unknown results for OR operands
Filter parameters by their own conditions

Copilot

Copilot review overview

🟡 Changes recommended

Nested object and in each declarations can lose inherited conditions, causing mutually exclusive branches to be analyzed together.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 4 · 1

Open (5)

Preserve inherited guards when flattening in-each declarations · New
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
Add test coverage for evaluator and branch-pruning behavior · New

Resolved since last review (1)

Preserve decisive false in And expressions with unevaluable operands

Copilot

Copilot review overview

🟡 Changes recommended

Conditional imports and hooks can still be discarded, while nested in each declarations can be incorrectly treated as unconditional.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 6

Open (6)

Preserve inherited conditions for nested in-each declarations · New
Preserve imports from top-level conditional branches · New
Propagate hooks collected from conditional branches · New
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…

Resolved since last review (2)

Preserve inherited guards when flattening in-each declarations
Add test coverage for evaluator and branch-pruning behavior

Copilot

Copilot review overview

🟡 Changes recommended

Dead conditional imports and in-each blocks can still affect diagnostics and implementation results.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 5

Open (5)

Exclude proven-dead imports from dependency resolution · New
Ignore proven-dead in-each blocks in goto-implementation results · New
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…

Resolved since last review (3)

Propagate hooks collected from conditional branches
Preserve imports from top-level conditional branches
Preserve inherited conditions for nested in-each declarations

Rather speculative for now, as exact future info required to make this choice isn't well-known Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

I don't wanna go _too_ far back with this, but fixed some obviously-visible errors Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

Copilot

Copilot review overview

🟡 Changes recommended

Conditional symbol variants can be discarded, and inactive imports still affect topology analysis.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 5

Open (5)

Filter inactive imports during topology dependency collection · New
Preserve branch-specific symbol variants when resolving name conflicts · New
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…
This repeatedly constructs a new EvaluationContext and reevaluates conditions in tight loops. If…

Resolved since last review (2)

Ignore proven-dead in-each blocks in goto-implementation results
Exclude proven-dead imports from dependency resolution

+    let mut symbols: HashMap<String, (ExistCondition, ZeroSpan, Vec<ZeroSpan>)>
        = HashMap::new();


JonatanWaern force-pushed the select-hashif-branch branch from 88ae3a0 to 0c8cd42 Compare June 12, 2026 09:22

Refactor how we store object conds for objectdecls

6f749b2

Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

JonatanWaern force-pushed the select-hashif-branch branch from 0c8cd42 to b7a331b Compare September 1, 2026 12:08

JonatanWaern requested review from TSonono and a lite review from Copilot September 1, 2026 12:33

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Copilot started reviewing on behalf of JonatanWaern September 1, 2026 12:58 View session

JonatanWaern force-pushed the select-hashif-branch branch from b7a331b to 21f1f59 Compare September 4, 2026 09:48

JonatanWaern requested a lite review from Copilot September 4, 2026 09:48

Copilot started reviewing on behalf of JonatanWaern September 4, 2026 09:49 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread src/analysis/structure/toplevel.rs

Comment thread CHANGELOG.md

Comment thread CHANGELOG.md Outdated

Comment thread src/analysis/templating/evaluation.rs

JonatanWaern force-pushed the select-hashif-branch branch from 21f1f59 to 877999e Compare September 4, 2026 10:56

JonatanWaern requested a lite review from Copilot September 4, 2026 11:04

Copilot started reviewing on behalf of JonatanWaern September 4, 2026 11:04 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread src/analysis/templating/evaluation.rs Outdated

Comment thread src/analysis/templating/evaluation.rs

JonatanWaern force-pushed the select-hashif-branch branch from 877999e to d275d19 Compare September 4, 2026 11:27

JonatanWaern requested a lite review from Copilot September 4, 2026 11:33

Copilot started reviewing on behalf of JonatanWaern September 4, 2026 11:33 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread src/analysis/structure/toplevel.rs Outdated

Comment thread src/analysis/structure/toplevel.rs

Comment thread src/analysis/templating/objects.rs

Comment thread src/analysis/templating/objects.rs Outdated

JonatanWaern force-pushed the select-hashif-branch branch from d275d19 to 64e2ed8 Compare September 11, 2026 07:27

JonatanWaern requested a balanced review from Copilot September 11, 2026 07:27

Copilot started reviewing on behalf of JonatanWaern September 11, 2026 07:28 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread src/analysis/templating/evaluation.rs

Comment thread src/analysis/templating/objects.rs

Comment thread src/analysis/templating/objects.rs

JonatanWaern force-pushed the select-hashif-branch branch from 64e2ed8 to e4ead15 Compare September 11, 2026 10:21

TSonono approved these changes Sep 21, 2026

View reviewed changes

JonatanWaern requested a balanced review from Copilot September 21, 2026 09:12

Copilot started reviewing on behalf of JonatanWaern September 21, 2026 09:13 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread src/analysis/templating/evaluation.rs

Comment thread src/analysis/templating/objects.rs

Comment thread src/analysis/templating/objects.rs

JonatanWaern force-pushed the select-hashif-branch branch from e4ead15 to 8644c67 Compare September 21, 2026 12:45

JonatanWaern requested a balanced review from Copilot September 21, 2026 12:46

Copilot started reviewing on behalf of JonatanWaern September 21, 2026 12:46 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread src/analysis/templating/evaluation.rs

JonatanWaern force-pushed the select-hashif-branch branch from 8644c67 to bcbb5bd Compare September 21, 2026 12:54

JonatanWaern requested a balanced review from Copilot September 21, 2026 12:56

Copilot started reviewing on behalf of JonatanWaern September 21, 2026 12:57 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread src/analysis/structure/toplevel.rs Outdated

Comment thread src/analysis/templating/evaluation.rs

JonatanWaern force-pushed the select-hashif-branch branch from bcbb5bd to 1a7b95c Compare September 21, 2026 13:10

JonatanWaern requested a balanced review from Copilot September 21, 2026 13:25

Copilot started reviewing on behalf of JonatanWaern September 21, 2026 13:25 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread src/analysis/structure/toplevel.rs Outdated

Comment thread src/analysis/structure/toplevel.rs

Comment thread src/analysis/structure/toplevel.rs

JonatanWaern force-pushed the select-hashif-branch branch from 1a7b95c to 7fe1c86 Compare September 21, 2026 13:48

JonatanWaern requested a balanced review from Copilot September 21, 2026 13:53

Copilot started reviewing on behalf of JonatanWaern September 21, 2026 13:54 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread src/analysis/structure/toplevel.rs

Comment thread src/analysis/templating/objects.rs

JonatanWaern added 2 commits September 21, 2026 16:22

Add logic to select between hashif branches

82fd5cb

Rather speculative for now, as exact future info required to make this choice isn't well-known Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

Fixup markdown mistakes in changelog

79a5cd1

I don't wanna go _too_ far back with this, but fixed some obviously-visible errors Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

JonatanWaern force-pushed the select-hashif-branch branch from 7fe1c86 to 79a5cd1 Compare September 21, 2026 14:22

JonatanWaern requested a balanced review from Copilot September 21, 2026 14:22

Copilot started reviewing on behalf of JonatanWaern September 21, 2026 14:23 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread src/analysis/mod.rs

Comment thread src/analysis/templating/objects.rs

Comment on lines +1643 to 1644

let mut symbols: HashMap<String, (ExistCondition, ZeroSpan, Vec<ZeroSpan>)>

= HashMap::new();

		let mut symbols: HashMap<String, (ExistCondition, ZeroSpan, Vec<ZeroSpan>)>
		= HashMap::new();

Conversation

JonatanWaern commented Jun 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (5)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment