Repository navigation
fix: make array type aliases covariant when used as struct fields - #1628
Open
BenjaminLucier wants to merge 1 commit into
Open
BenjaminLucier wants to merge 1 commit into
BenjaminLucier wants to merge 1 commit into
Conversation
rust-ndarray#1480 made `ArrayBase` covariant by adding a defaulted third parameter `A = <S as RawData>::Elem`, but the public type aliases still used the default. rustc computes a struct's variance from its field types without normalizing projections (rust-lang/rust#115799), so any struct holding an `ArrayView`, `ArrayViewMut`, `CowArray`, `Array`, `ArcArray` or `RawArrayView` was invariant over the view lifetime and the element type. Spell out the element type in the seven base aliases. Because of the bound `S: RawData<Elem = A>`, each alias names the same type as before, so this is not a breaking change. Also document the third parameter on `ArrayBase`, and add struct-wrapped regression tests for each alias to tests/variance.rs. Fixes rust-ndarray#1610 AI assisted
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1610.
Bug
Since #1480, a bare
ArrayVieworCowArrayin a function signature shortens fine, but a struct containing one of the type aliases is still invariant, over both the view lifetime and the element type:This affects
Array,ArcArray,CowArray,ArrayView,ArrayViewMutandRawArrayView, and every dimension alias built on them, on 0.17.0 through master. It is also the original use case of #1332, whose reporter hit the problem with a view "deep inside a data structure".Root cause
#1480 made
ArrayBaseitself covariant by adding a defaulted third parameter,ArrayBase<S, D, A = <S as RawData>::Elem>, but the public aliases kept the two-parameter form, so they still expand to a type containing the projection<S as RawData>::Elem.rustc computes a struct's variance from its field types without normalizing projections, and treats every parameter inside a projection as invariant (rust-lang/rust#115799). In a function signature the alias is normalized before the subtyping check, which is why #1480's test passes while a struct field stays invariant.
Fix
Spell out the element type in the seven base aliases:
The bound
S: RawData<Elem = A>forcesA == S::Elem, so each alias names exactly the same type as before — not a breaking change, suitable for a patch release.A struct field written as
ArrayBase<S, D>directly still gets the default and stays invariant; ndarray can't fix that without removing the default (breaking). TheArrayBasedocs now explain the third parameter, with a doc-tested example of a covariant generic wrapper struct.Tests
tests/variance.rsgains a struct-wrappedshorten_*case per alias, which compiles only if the struct is covariant, plus a runtime test calling them.RawArrayViewMutis omitted because, like*mut T, it is correctly invariant. Without the alias change all seven new cases fail to compile; with it they pass.Checked locally on stable 1.99.1 (aarch64-apple-darwin) with
RUSTFLAGS=-D warnings: clippy (-F approx,serde,rayon), thescripts/all-tests.shsteps except BLAS integration and benches (no features /approx/ all features + quickcheck / release /--features blaslib / blas-mock / examples / doc tests), andRUSTDOCFLAGS=-Dwarnings cargo doc --no-deps --all-features. Not run locally: nightlycargo fmt, MSRV 1.87, miri, nostd, cross, cargo-careful, BLAS integration.AI assisted.