Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix getOrElse type inference issue in Scala 3, closes #2761 #2803

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

OlegYch
Copy link
Contributor

@OlegYch OlegYch commented Aug 21, 2023

Note: I have no idea what Unconst is doing but this fixes the issue

@OlegYch OlegYch mentioned this pull request Aug 21, 2023
@nafg
Copy link
Member

nafg commented Aug 21, 2023

I'm confused, I thought it's fixed in M4?

@nafg
Copy link
Member

nafg commented Aug 21, 2023

I don't either know offhand exactly what Unconst is for, but with this change its name is no longer accurate.

@github-actions
Copy link
Contributor

Incompatible changes

slick

3 changes since 3.5.0-pre.56.04ce9b09

Code changes

Incompatibility Symbol Problem
Backward slick.compat.collection.package MissingClassProblem

class slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$ MissingClassProblem

object slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$JavaConverters$ MissingClassProblem

object slick.compat.collection.package#JavaConverters does not have a correspondent in current version

@OlegYch
Copy link
Contributor Author

OlegYch commented Aug 22, 2023

@nafg #2761 is actually a problem caused by fix to #2674

the change actually aligns scala3 implementation with scala2

so i think the name is still accurate

perhaps @szeiger can shed some light on it

@OlegYch
Copy link
Contributor Author

OlegYch commented Aug 22, 2023

oh, i get it, Unconst means that it removed ConstColumn ...

@github-actions
Copy link
Contributor

Incompatible changes

slick

3 changes since 3.5.0-pre.62.ed0ba56a

Code changes

Incompatibility Symbol Problem
Backward slick.compat.collection.package MissingClassProblem

class slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$ MissingClassProblem

object slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$JavaConverters$ MissingClassProblem

object slick.compat.collection.package#JavaConverters does not have a correspondent in current version

@github-actions
Copy link
Contributor

Incompatible changes

slick

3 changes since 3.5.0-pre.66.a3e77c60

Code changes

Incompatibility Symbol Problem
Backward slick.compat.collection.package MissingClassProblem

class slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$ MissingClassProblem

object slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$JavaConverters$ MissingClassProblem

object slick.compat.collection.package#JavaConverters does not have a correspondent in current version

@nafg
Copy link
Member

nafg commented Aug 30, 2023

With the new definition of Unconst it seems pointless. Is it really needed?

@nafg
Copy link
Member

nafg commented Aug 30, 2023

or maybe scala 2 should keep type Unconst[P, P2] = P and scala 3 essentially gets type Unconst[P, P2] = P2 ?

@nafg
Copy link
Member

nafg commented Aug 30, 2023

Unconst is only used in getOrElse, where it says

 def getOrElse[M, P2 <: P](default: M)(implicit shape: Shape[FlatShapeLevel, M, ?, P2], ol: OptionLift[P2, O]): ShapedValue.Unconst[P, P2] = {
    // P2 != P can only happen if M contains plain values, which pack to ConstColumn instead of Rep.
    // Both have the same packedShape (RepShape), so we can safely cast here:
    val r = fold[P, P](shape.pack(default): P)(identity)(shape.packedShape.asInstanceOf[Shape[FlatShapeLevel, P, ?, P]])
    r.asInstanceOf[ShapedValue.Unconst[P, P2]]
  }

I don't understand the comment.

Also since that's the only place it's used I don't know why it's in ShapedValue.

@nafg
Copy link
Member

nafg commented Aug 30, 2023

I mean if you always return P then get rid of Unconst and just do r.asInstanceOf[P] over there. In fact it shouldn't even need a cast since r is what's returned from fold[P, P] which is def fold[B, BP]/* ... */: BP

@nafg
Copy link
Member

nafg commented Aug 30, 2023

You could go for the full implication of this change. But have you tried other ways, like adding other match cases or something?

Also what happens if you keep your change but don't change the P2s to P?

Also what happens if scala 2 would use P2?

@OlegYch
Copy link
Contributor Author

OlegYch commented Aug 30, 2023

type Unconst[P, P2] = P2 on scala2 breaks #2674
type Unconst[P, P2] = P match { case Rep[t] => Rep[t] case _ => P } and type Unconst[P, P2] = P2 match { case Rep[t] => Rep[t] case _ => P2 } behave the same on scala3

@OlegYch
Copy link
Contributor Author

OlegYch commented Aug 30, 2023

in fact removing P2 entirely seems to be harmless

@github-actions
Copy link
Contributor

Incompatible changes

slick

3 changes since 3.5.0-pre.68.610c46f8

Code changes

Incompatibility Symbol Problem
Backward slick.compat.collection.package MissingClassProblem

class slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$ MissingClassProblem

object slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$JavaConverters$ MissingClassProblem

object slick.compat.collection.package#JavaConverters does not have a correspondent in current version

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

Incompatible changes

slick

3 changes since 3.5.0-pre.76.da631720

Code changes

Incompatibility Symbol Problem
Backward slick.compat.collection.package MissingClassProblem

class slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$ MissingClassProblem

object slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$JavaConverters$ MissingClassProblem

object slick.compat.collection.package#JavaConverters does not have a correspondent in current version

@OlegYch
Copy link
Contributor Author

OlegYch commented Sep 6, 2023

@nafg i did some cleanup, and think it is ready to be merged

@github-actions
Copy link
Contributor

Incompatible changes

slick

3 changes since 3.5.0-pre.110.88b2ffb1

Code changes

Incompatibility Symbol Problem
Backward slick.compat.collection.package MissingClassProblem

class slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$ MissingClassProblem

object slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$JavaConverters$ MissingClassProblem

object slick.compat.collection.package#JavaConverters does not have a correspondent in current version

@nafg nafg linked an issue Nov 20, 2023 that may be closed by this pull request
Copy link
Member

@nafg nafg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I probably had feedback that I don't recall but I want to do another milestone release so let's just get it in

Copy link
Contributor

Incompatible changes

slick

3 changes since 3.5.0-pre.118.137bf069

Code changes

Incompatibility Symbol Problem
Backward slick.compat.collection.package MissingClassProblem

class slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$ MissingClassProblem

object slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$JavaConverters$ MissingClassProblem

object slick.compat.collection.package#JavaConverters does not have a correspondent in current version

Copy link
Contributor

Incompatible changes

slick

3 changes since 3.5.0-pre.122.6896ef03

Code changes

Incompatibility Symbol Problem
Backward slick.compat.collection.package MissingClassProblem

class slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$ MissingClassProblem

object slick.compat.collection.package does not have a correspondent in current version

Backward slick.compat.collection.package$JavaConverters$ MissingClassProblem

object slick.compat.collection.package#JavaConverters does not have a correspondent in current version

@mergify mergify bot merged commit d56bca8 into slick:main Nov 20, 2023
14 checks passed
@nafg nafg added this to the 3.5.0 milestone Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getOrElse type inference issue in Scala 3
2 participants