Fix #481: subtype check for refinement type members uses semantic equality#594
Open
MilosM348 wants to merge 2 commits into
Open
Fix #481: subtype check for refinement type members uses semantic equality#594MilosM348 wants to merge 2 commits into
MilosM348 wants to merge 2 commits into
Conversation
…equality Reproduction (zio#481): trait A { type T } trait AInt extends A { override type T = Int } Tag[AInt].tag <:< Tag[A { type T = Int }].tag // returned `false`, should be `true` The bug lives in `LightTypeTagInheritance.compareDecl` for the case where both sides have a `RefinementDecl.TypeMember` and the right-hand-side member is *concrete* (e.g. `type T = Int`), not abstract (e.g. `type T <: AnyVal`). Type-member assignments are invariant, so the historical implementation used structural AST equality: case (RefinementDecl.TypeMember(ln, lref), RefinementDecl.TypeMember(rn, rref)) => ln == rn && lref == rref Two `LightTypeTagRef` AST values can denote the *same* type while differing in incidental shape — presence/absence of an empty `Boundaries.Empty`, distinct prefix chains for the same symbol, refinement form vs. plain `NameReference` for a fixed type member, etc. When that happens the `==` short-circuits to `false` and a perfectly valid subtype relation is lost. The principled invariant-equality check is mutual subtyping: two refs denote the same type iff each is a subtype of the other. We keep the `==` fast-path so the common case stays cheap and we avoid a recursive `isChild` call when not needed. case (RefinementDecl.TypeMember(ln, lref), RefinementDecl.TypeMember(rn, rref)) => ln == rn && (lref == rref || (ctx.isChild(lref, rref) && ctx.isChild(rref, lref))) Tests ----- * Added `Issue481A` / `Issue481AInt` / `Issue481AStr` fixtures to `TestModel`. * Added a regression block in `SharedLightTypeTagTest` (so it runs across all Scala versions) that covers: - the direct repro from the issue body (`AInt <:< A { type T = Int }`), - the negative direction (`AInt` is not a subtype of `A { type T = String }`), - already-passing adjacent cases with upper-bounded abstract type members, to guard against regressions in surrounding paths. Refs: zio#481 Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
The previous run on 552e2f2 failed at "Prepare all required actions" with ##[error]The action carabiner-dev/actions@360ffa1eb909b... is not allowed in zio/izumi-reflect because all actions must be from a repository owned by zio, ... This is a GitHub Actions allowlist check that fires on transitive `uses:` resolution. The same workflow file is shared with other open PRs (zio#586, zio#592) that passed CI on April 8 / May 1 -- nothing in this PR's diff touches `.github/workflows` or any action reference. The chain that resolves to `carabiner-dev/actions/ampel/verify` appears to be transitively pulled in from one of the pinned actions and the allowlist check seems to be intermittent. Empty commit to bump the head SHA so the workflow re-resolves all `uses:` references with a fresh action cache. If it fails again with the same allowlist error, the fix has to come from a maintainer who can either pin the offending transitive action to a specific SHA or add `carabiner-dev/*` to the repository's allowed-actions list. No code changes. Signed-off-by: Milos M <MilosM348@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Author
|
Heads up to maintainers (cc @neko-kai @pshirshov): the failing CI on this PR is not caused by the diff. Every
Where
|
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
2 participants
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.
/claim #481
What
Fixes the long-standing #481 regression: a trait that fixes an inherited abstract
type member should be a subtype of the structural refinement form fixing the
same member, but
Tag[AInt].tag <:< Tag[A { type T = Int }].tagwas returningfalse.Why
LightTypeTagInheritance.compareDeclcompared twoRefinementDecl.TypeMemberdeclarations with
lref == rref— strict structural AST equality. Two refs candenote the same type while differing in incidental shape (presence/absence of an
empty
Boundaries.Empty, distinct prefix chains for the same symbol, refinementform vs. plain
NameReferencefor a fixed type member, etc.), so the==short-circuited to
falseand a perfectly valid subtype relation was lost.How
Mutual subtyping is the principled invariant-equality check: two refs denote the
same type iff each is a subtype of the other. Keep the
==fast-path so thecommon case stays cheap.