Skip to content

Change workings of experimental into modifier on parameter types #19673

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

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Feb 12, 2024

into is now a modifier that sits on the precise parameter part where a conversion is
allowed.

Hint for reviewers:

  • Start with the doc page into-modifier.md, to see what changed.
  • Then, take a look at the doc comments of the into and $into annotations. These explain what role these annotations play in type checking.
  • Then the rest should make sense.

@odersky odersky force-pushed the change-into branch 4 times, most recently from 53ac2da to 587f3b4 Compare February 14, 2024 17:46
@odersky odersky requested a review from sjrd February 14, 2024 20:25
@odersky odersky marked this pull request as ready for review February 14, 2024 20:26
Copy link
Member

@sjrd sjrd 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. Looks good overall. I only have minor comments.

@@ -211,6 +211,12 @@ FunArgType ::= Type
FunArgTypes ::= FunArgType { ‘,’ FunArgType }
ParamType ::= [‘=>’] ParamValueType
ParamValueType ::= Type [‘*’] PostfixOp(t, "*")
| IntoType
| ‘(’ IntoType ‘)’ ‘*’ PostfixOp(t, "*")
IntoType ::= [‘into’] IntoTargetType Into(t)
Copy link
Member

Choose a reason for hiding this comment

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

Having into optional here leads to an ambiguity. If I want to parse a ParamValue and I have Int in front of me, I can choose the Type ['*'] alternative, or the IntoType alternative with this alternative inside.

It seems weird that it is optional here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but the recognized language the same either way. It's just the AST construction which is ambiguous. But the grammar does not guarantee that anyway.

The into needs to be optional since IntoType is also used after => in IntoTargetTyope.

@@ -58,24 +58,63 @@ concatAll(List('a'), "bc", Array('d', 'e'))
```
would apply two _different_ implicit conversions: the conversion from `String` to `Iterable[Char]` gets applied to the second argument and the conversion from `Array[Char]` to `Iterable[Char]` gets applied to the third argument.

Note that a vararg parameter type with into modifiers needs to be put in parentheses, as is shown in the example above. This is to make the precedence clear: each element of the argument sequence is converted by itself.

## Retrofitting Scala 2 libraries
Copy link
Member

Choose a reason for hiding this comment

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

If and when the scala2-library-tasty becomes stable, hopefully we won't need any of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also the case where a library wants to cross-build between Scala 2 and 3 and wants to avoid feature warnings for conversions in Scala 3 clients. That library could use @into in its API.

Copy link
Member

Choose a reason for hiding this comment

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

Such a library wouldn't have Conversions either, though. It would have implicit defs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except if the library also wants to accommodate conversions defined in client code.

@sjrd sjrd assigned odersky and unassigned sjrd Feb 27, 2024
@odersky odersky removed their assignment Feb 28, 2024
@odersky odersky merged commit e994cf0 into scala:main Feb 28, 2024
@odersky odersky deleted the change-into branch February 28, 2024 09:35
@Kordyjan Kordyjan added this to the 3.4.2 milestone Mar 28, 2024
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.

3 participants