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

Implement SIP 64 as non-experimental #21668

Merged
merged 9 commits into from
Oct 4, 2024
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Sep 28, 2024

For the new syntax of givens and context bounds, see the diffs in reference/syntax.md.

Also: Some reshuffling and editing of existing material.
@odersky
Copy link
Contributor Author

odersky commented Sep 30, 2024

Best reviewed commit by commit. Most changes are to docs or tests, there are only a few changes to the code.

I left some tests to use the old syntax, just so that we have early warnings for possible regressions. But most tests are now using the new syntax, so that we best reassurance that corner cases work.
@odersky
Copy link
Contributor Author

odersky commented Oct 1, 2024

Who would like to gve this a review? It's all pretty straightforward. Biggest real changes are in the docs.

@smarter
Copy link
Member

smarter commented Oct 3, 2024

Playing a bit with the implementation, I noticed some details that I believe weren't clear from the spec:

Context bounds for type members

I love this feature but I wonder if it could be used to bring extension methods in scope too:

trait Serializable[T]:
  extension (x: T) def serialize: String

trait Base:
  type Elem: Serializable

def test0(b: Base) =
  summon[Serializable[b.Elem]] // ok
  b.serialize // error

Naming context bounds

I expected the as bounds on classes and traits to create public given members:

case class CC1[X: Serializable as s](x: X):
  type Elem = X

def test1(c: CC1[?]) =
  c.s // error
  summon[Serializable[c.Elem]] // error

Adding a public alias isn't enough to have it summon-able like in the context bounds for type members example above:

case class CC2[X: Serializable as s](x: X):
  type Elem = X
  given s1: Serializable[Elem] = s

def test2(c: CC2[?]) =
  c.s1 // ok
  summon[Serializable[c.Elem]] // error

Even if we directly extend from a trait defining a context bound for a type member, I guess because of dealiasing:

case class CC3[X: Serializable](x: X) extends Base:
  type Elem = X

def test3(c: CC3[?]) =
  summon[Serializable[c.Elem]] // error
  val b: Base = c
  summon[Serializable[b.Elem]] // ok

Ultimately, I wish member selection worked as well for regular type bounds than context bounds:

trait Serialize { def serialize: String }
case class Foo[T <: Serializer](x: T)
Foo(...).x.serialize
// vs
case class Foo[T: Serializable](x: T) // maybe with `as s` so I still have the ability to not retain a reference to the typeclass instance
Foo(...).x.serialize

I don't see a clear path to get there so this is more food for thought than a PR review.

@odersky
Copy link
Contributor Author

odersky commented Oct 3, 2024

@smarter I think these additions would be good to have as feature requests.

@Gedochao Gedochao added the needs-minor-release This PR cannot be merged until the next minor release label Oct 4, 2024
@@ -483,6 +485,10 @@ GivenConditional ::= DefTypeParamClause
| GivenType
GivenType ::= AnnotType1 {id [nl] AnnotType1}

OldGivenDef ::= [OldGivenSig] (AnnotType [‘=’ Expr] | StructuralInstance) -- syntax up to Scala 3.5, to be deprecated in the future
OldGivenSig ::= [id] [DefTypeParamClause] {UsingParamClause} ‘:’ -- one of `id`, `DefTypeParamClause`, `UsingParamClause` must be present
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consistent alignment

Suggested change
OldGivenSig ::= [id] [DefTypeParamClause] {UsingParamClause} ‘:’ -- one of `id`, `DefTypeParamClause`, `UsingParamClause` must be present
OldGivenSig ::= [id] [DefTypeParamClause] {UsingParamClause} ‘:’ -- one of `id`, `DefTypeParamClause`, `UsingParamClause` must be present

Comment on lines 157 to 158
The expansion of the right hand side of `Comparer2` expands the `Cmp[X]` alias
and then inserts the context function at the same place as what's done for `Comparer`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the expansion should be shown.

Copy link
Contributor

@kyouko-taiga kyouko-taiga left a comment

Choose a reason for hiding this comment

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

LGTM overall. I left a few inline comments for things that I think could be improved before it gets merged, but nothing looks like a blocker to me.

Very nice step forward!

docs/_docs/reference/contextual/context-bounds.md Outdated Show resolved Hide resolved
docs/_docs/reference/contextual/deferred-givens.md Outdated Show resolved Hide resolved
docs/_docs/reference/contextual/deferred-givens.md Outdated Show resolved Hide resolved
docs/_docs/reference/contextual/givens.md Show resolved Hide resolved
docs/_docs/reference/contextual/givens.md Outdated Show resolved Hide resolved
docs/_docs/reference/contextual/previous-givens.md Outdated Show resolved Hide resolved
docs/_docs/reference/experimental/typeclasses.md Outdated Show resolved Hide resolved
docs/_docs/reference/syntax.md Outdated Show resolved Hide resolved
docs/_docs/reference/syntax.md Outdated Show resolved Hide resolved
Comment on lines 463 to 464
OldGivenDef ::= [OldGivenSig] (AnnotType [‘=’ Expr] | StructuralInstance) -- syntax up to Scala 3.5, to be deprecated in the future
OldGivenSig ::= [id] [DefTypeParamClause] {UsingParamClause} ‘:’ -- one of `id`, `DefTypeParamClause`, `UsingParamClause` must be present
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OldGivenDef ::= [OldGivenSig] (AnnotType [‘=’ Expr] | StructuralInstance) -- syntax up to Scala 3.5, to be deprecated in the future
OldGivenSig ::= [id] [DefTypeParamClause] {UsingParamClause} ‘:’ -- one of `id`, `DefTypeParamClause`, `UsingParamClause` must be present
OldGivenDef ::= [OldGivenSig] (AnnotType ['=' Expr] | StructuralInstance) -- syntax up to Scala 3.5, to be deprecated in the future
OldGivenSig ::= [id] [DefTypeParamClause] {UsingParamClause} ':' -- one of `id`, `DefTypeParamClause`, `UsingParamClause` must be present

odersky and others added 2 commits October 4, 2024 19:07
Co-authored-by: Dimi Racordon <dimitri.racordon@gmail.com>
@odersky odersky merged commit ebbd685 into scala:main Oct 4, 2024
28 checks passed
@odersky odersky deleted the finalize-sip-64 branch October 4, 2024 20:21
@WojciechMazur WojciechMazur added this to the 3.6.0 milestone Oct 8, 2024
@WojciechMazur WojciechMazur added the release-notes Should be mentioned in the release notes label Oct 8, 2024
Comment on lines +134 to +140
From Scala 3.6 on, context bounds can also be used in polymorphic function types and polymorphic function literals:

```scala
type Comparer = [X: Ord] => (x: X, y: X) => Boolean
val less: Comparer = [X: Ord as ord] => (x: X, y: X) =>
ord.compare(x, y) < 0
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this example is either not correct - it does not compile in neither plain Scala 3.6-nightly or with experimental:modularity.
It fails to parse by the compiler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that one is still in the works: #21643

Comment on lines +169 to +172
```scala
class Collection:
type Element: Ord
```
Copy link
Contributor

Choose a reason for hiding this comment

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

This example might be incorrect aswell. I'm not sure it it's a bug or expected behaviour, but it fails to compile with

[error] No given instance of type Ord[Sorted.this.Element] was found for inferring the implementation of the deferred given instance given_Ord_Element in class Sorted

It does however compile when replacing class Collection with trait Collection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, that's a bug. It should be trait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants