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 #10087 : Change syntax of given instances in patterns #10091

Merged
merged 5 commits into from
Oct 30, 2020

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 26, 2020

See the reference doc pages in this PR for details

@odersky odersky changed the title Fix #10087 Fix #10087 : Change syntax of given instances in patterns Oct 27, 2020
for given Context <- applicationContexts do

pair match
case (ctx as given Context, y) => ...
Copy link
Contributor

Choose a reason for hiding this comment

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

How would we re-write the following pattern to make x, y and z givens?

case x @ (y: Y, z @ Z()) =>

Copy link
Contributor

Choose a reason for hiding this comment

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

We should have tests with nested given patterns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can't make x a given here. for x and y it would simply be:

case (x as given Y, y as given Z)

We already have a test like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a core limitation of the syntax.

The given statement syntax is

given X
given x as X

What if we used the same in patterns?

case given X =>
case given x as X =>

Then we could extend it to

case given x @ X(y, z) =>

@@ -4,11 +4,11 @@ class Test {
import scala.collection.immutable.{TreeSet, HashSet}

def f2[T](x: Ordering[T]) = {
val (given y: Ordering[T]) = x
val (given Ordering[T]) = x
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. This allows for a given to be bound to a val which we could not do with given y as Ordering[T] = x

val (y as given Ordering[T]) = x

Copy link
Member

Choose a reason for hiding this comment

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

If I understand this correctly, the y is not implicit here (hence the error on next line), and the given Ordering is not visible outside of the pattern its part of, so while this syntax is permitted because any pattern can be used in a val, it's useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarter: correct. implicits in patterns behave the same way.

new TreeSet[T] // error: no implicit ordering defined for T
}
def f3[T](x: Ordering[T]) = {
val given y: Ordering[T] = x // error: pattern expected
val given Ordering[T] = x
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this one work as a val definition in class?

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, it should work

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we should have some tests

@odersky
Copy link
Contributor Author

odersky commented Oct 29, 2020

Since it is a syntax change it would be good to merge this before M1. I don't think we need more tests for this at this point.

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

Implementation-wise it LGTM. Though I have some concerns about it being the right syntax due to limitations in expressivity and it not being aligned with statement given syntax.

@nicolasstucki
Copy link
Contributor

This syntax improvement is clearly the right one

inline def setFor[T]: Set[T] = summonFrom {
-  case given _: Ordering[T] => new TreeSet[T]
+  case given Ordering[T] => new TreeSet[T]
  case _                 => new HashSet[T]
}

But this other change is not so clear

inline def setFor[T]: Set[T] = summonFrom {
-  case given t: Ordering[T] => new TreeSet[T]
+  case t as given Ordering[T] => new TreeSet[T]
  case _                 => new HashSet[T]
}

The following would be clearer if we compare with given statements

inline def setFor[T]: Set[T] = summonFrom {
-  case given t: Ordering[T] => new TreeSet[T]
+  case given t as Ordering[T] => new TreeSet[T]
  case _                 => new HashSet[T]
}

@smarter smarter added this to the 3.0.0-M1 milestone Oct 30, 2020
@smarter
Copy link
Member

smarter commented Oct 30, 2020

case t as given Ordering[T] => new TreeSet[T]

This syntax always has to be supported (assuming #9837 stays in)

case given t as Ordering[T] => new TreeSet[T]

Because the previous syntax already exists, this one doesn't add anything, so it's not clear if it should be supported.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

I think we should get this in M1 since it's a net improvement and we can discuss if we want more changes later.

@bishabosha bishabosha merged commit cf8fbd8 into scala:master Oct 30, 2020
@bishabosha bishabosha deleted the fix-#10087 branch October 30, 2020 12:05
@Kordyjan Kordyjan modified the milestones: 3.0.0-M1, 3.0.0 Aug 2, 2023
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.

5 participants