Skip to content

Add position-independent diagnostic syntax to syntax tests#11188

Closed
tilladam wants to merge 2 commits intoslint-ui:masterfrom
tilladam:syntax-test-position-independent-warnings
Closed

Add position-independent diagnostic syntax to syntax tests#11188
tilladam wants to merge 2 commits intoslint-ui:masterfrom
tilladam:syntax-test-position-independent-warnings

Conversation

@tilladam
Copy link
Copy Markdown
Contributor

Summary

  • Add //-error{message} and //-warning{message} syntax for matching diagnostics by message and level only, ignoring source position
  • Useful for layout-related binding loop warnings where the diagnostic position varies across platforms due to analysis traversal order differences
  • Includes self-tests verifying the new syntax works (positive match, wrong message, wrong level)

This is needed by #10552 (binding loop detection for callbacks), where several syntax tests fail on CI because layout binding loop warnings are emitted at platform-dependent source locations.

Test plan

  • self_test passes — 3 new assertions (positive, wrong message, wrong level)
  • syntax_tests passes — no regressions in existing tests
  • CI passes

tilladam and others added 2 commits March 30, 2026 14:02
Add `//-error{message}` and `//-warning{message}` syntax for matching
diagnostics by message and level only, ignoring source position. This
is useful for binding loop warnings on layout properties where the
diagnostic position varies across platforms due to analysis traversal
order differences.
@ogoffart
Copy link
Copy Markdown
Member

I'm curious what makes the position change across platform.
Is there something not deterministic in the diagnostics?
(The #10552 PR fails currently because some loop message have different order.)

What i have noticed is that sometimes diagnostics are not attached to a location and a file and this feature could help, although i don't know if it would work as is because there is also not a file.

@tilladam
Copy link
Copy Markdown
Contributor Author

The non-determinism comes from visited: HashSet<PropertyPath> in AnalysisContext (binding_analysis.rs:161). It uses Rust's default RandomState hasher, so the hash seed differs between runs/platforms.

While visited is never iterated, it affects control flow at line 366: if !context.visited.insert(current.clone()) { return }. This early-return determines which traversal path through the binding graph discovers the cycle first. For layout-related cycles (which involve implicit properties like layoutinfo-h, width, layout-cache), the cycle can be entered from multiple directions. Different hash seeds → different traversal order → cycle detected from a different entry point → the currently_analyzing stack has the same properties but in a different order → diagnostics are emitted at different source positions.

Additionally, at line 346, is_in_binding_loop.replace(true) causes the diagnostic emission loop to break early if a property was already marked from a previous traversal. Different traversal orders can cause different properties to be pre-marked, which changes how many diagnostics are emitted and where.

For the case of diagnostics without a file — the current implementation requires matching against a specific file's source, so position-independent matching within a file wouldn't help with that case. That would need a separate mechanism.

@LeonMatthes
Copy link
Copy Markdown
Member

@tilladam Then IMO the better solution is to not use HashSet and use a BTreeSet instead, which is deterministic.
A non-deterministic compiler is a bad idea anyhow, and is causing us issues, like unnecessarily re-writing files that haven't really changed.

@tilladam
Copy link
Copy Markdown
Contributor Author

tilladam commented Apr 1, 2026

You are right, droppign this one in favor of #11219

@tilladam tilladam closed this Apr 1, 2026
@tilladam
Copy link
Copy Markdown
Contributor Author

tilladam commented Apr 1, 2026

After investigating @ogoffart's point further, I agree that the visited HashSet explanation doesn't hold up — since visited is only used for membership checks (insert returns a bool), its internal hash ordering doesn't affect control flow.

I did find a real source of non-determinism: Element::property_analysis is a HashMap that gets iterated in several passes (inlining, move_declarations, etc.) before binding analysis runs. #11219 fixes this by switching it to BTreeMap.

However, I can't confirm locally that this is the specific cause of the cross-platform diagnostic position differences in #10552 — that would need CI validation. So the position-independent diagnostic syntax proposed here may still be needed if other sources of non-determinism remain.

@tilladam tilladam deleted the syntax-test-position-independent-warnings branch April 10, 2026 08:43
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.

3 participants