Skip to content

fix: NodeMeta and PortMeta should be generic#283

Open
aborgna-q wants to merge 2 commits intomainfrom
ab/node-meta-generic
Open

fix: NodeMeta and PortMeta should be generic#283
aborgna-q wants to merge 2 commits intomainfrom
ab/node-meta-generic

Conversation

@aborgna-q
Copy link
Collaborator

Fixes #282.

We partially implemented generic node/port/portOffset parameters for some portgraph containers, but NodeMeta and PortMeta still were stuck hardcoding the types.

This change ensures everything is generic.

  • I had to add a Unsigned::NonZero associated type so we can have niche optimization with arbitrary indices.
  • The main Portgraph impl is still only done for <N=u32, P=u32>. I'll check if we can actually make the blanket impl next.

This may be a breaking change due to the new Unsigned::NonZero. I would hope users are not implementing that trait, but we forgot to seal it so we may need to mark this as breaking :/

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 18, 2026

Merging this PR will degrade performance by 15.17%

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 49 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
clone_line_graph[10000] 654.2 µs 511 µs +28.02%
clone_line_graph[1000000] 43 ms 50.7 ms -15.17%

Comparing ab/node-meta-generic (f320844) with main (b1c2ab5)

Open in CodSpeed

@aborgna-q aborgna-q force-pushed the ab/node-meta-generic branch from defe585 to c1b7268 Compare March 18, 2026 17:32
@hugrbot
Copy link
Collaborator

hugrbot commented Mar 18, 2026

This PR contains breaking changes to the public Rust API.
Please deprecate the old API instead (if possible), or mark the PR with a ! to indicate a breaking change.

cargo-semver-checks summary
    Building portgraph v0.15.3 (current)
     Built [  10.357s] (current)
   Parsing portgraph v0.15.3 (current)
    Parsed [   0.046s] (current)
  Building portgraph v0.15.3 (baseline)
     Built [   6.647s] (baseline)
   Parsing portgraph v0.15.3 (baseline)
    Parsed [   0.037s] (baseline)
  Checking portgraph v0.15.3 -> v0.15.3 (assume minor change)
   Checked [   0.064s] 196 checks: 194 pass, 2 fail, 0 warn, 56 skip

--- failure trait_associated_type_added: non-sealed public trait added associated type without default value ---

Description:
A non-sealed trait has gained an associated type without a default value, which breaks downstream implementations of the trait
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/trait_associated_type_added.ron

Failed in:
trait associated type portgraph::index::Unsigned::NonZero in file /home/runner/work/portgraph/portgraph/PR_BRANCH/src/index.rs:457

--- failure trait_method_added: pub trait method added ---

Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/trait_method_added.ron

Failed in:
trait method portgraph::index::Unsigned::to_nonzero in file /home/runner/work/portgraph/portgraph/PR_BRANCH/src/index.rs:470
trait method portgraph::index::Unsigned::to_nonzero_unchecked in file /home/runner/work/portgraph/portgraph/PR_BRANCH/src/index.rs:477
trait method portgraph::index::Unsigned::from_nonzero in file /home/runner/work/portgraph/portgraph/PR_BRANCH/src/index.rs:480

   Summary semver requires new major version: 2 major and 0 minor checks failed
  Finished [  18.628s] portgraph

@aborgna-q aborgna-q force-pushed the ab/node-meta-generic branch from 6bce5f3 to f320844 Compare March 18, 2026 17:46
@aborgna-q
Copy link
Collaborator Author

The changes to PortEntry broke serialization. I'll update the impl there.

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.

Assert panics on allocating NodeMeta with more ports than capacity

2 participants