Skip to content

Detect overlapping supertype variants via leaf type IDs#1080

Open
Veykril wants to merge 1 commit intosalsa-rs:masterfrom
Veykril:push-wmkonznwmxrt
Open

Detect overlapping supertype variants via leaf type IDs#1080
Veykril wants to merge 1 commit intosalsa-rs:masterfrom
Veykril:push-wmkonznwmxrt

Conversation

@Veykril
Copy link
Copy Markdown
Member

@Veykril Veykril commented Mar 30, 2026

This can cause hard to debug issues and is never useful so we should recognize this. Ideally at compile time but comparing type ids at compile time is not yet possible

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 30, 2026

👷 Deploy Preview for salsa-rs processing.

Name Link
🔨 Latest commit d1c1ee1
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/69ca68f04786c800089c56cb

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 30, 2026

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit d1c1ee1
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/69ca68f04786c800089c56cb

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 30, 2026

Merging this PR will not alter performance

✅ 13 untouched benchmarks


Comparing Veykril:push-wmkonznwmxrt (d1c1ee1) with master (2f687a1)

Open in CodSpeed

@MichaReiser
Copy link
Copy Markdown
Contributor

Do you know what the reason is for the perf regression?

@Veykril
Copy link
Copy Markdown
Member Author

Veykril commented Mar 31, 2026

did you rerun it just now? EIther way no clue what was up with codespeed there

@MichaReiser
Copy link
Copy Markdown
Contributor

Yeah, I did re-run it.

I do plan to look at this (unless @ibraheemdev or someone of the r-a team beats me to it). I'm just a bit swamped with code reviews across projects, so it may take me until next week to get to it.

@Veykril
Copy link
Copy Markdown
Member Author

Veykril commented Apr 1, 2026

Yea no rush, this isn't a feature anyways (and r-a right now can't upgrade since we still need to investigate the fallback immediate cycle handling regression)

@ibraheemdev ibraheemdev self-assigned this Apr 2, 2026
@MichaReiser
Copy link
Copy Markdown
Contributor

Could we use https://docs.rs/static_assertions/latest/static_assertions/macro.assert_type_ne_all.html or does it perform structural equality?

@Veykril
Copy link
Copy Markdown
Member Author

Veykril commented Apr 2, 2026

We can't do that unfortuantely, we need to transitively check all contained variants which we can't do at macro expansion time. That is

#[derive(Supertype)]
enum E {
    A(A),
    B(B),
}

#[derive(Supertype)]
enum B {
    A(A)
}

would be needed to be detected here. Supertype effectively flattens this variant tree. Thats what we want to guard against here, as that means that salsa will turn a E(B::A(A)) into an E::A(A), so variants lose their meaning if they overlap

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