Skip to content

Allow prefix operators on the LHS of assignments #13328

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 4 commits into from
Aug 23, 2021

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 18, 2021

Fixes #13282

As you can see, the hard (but essential!) part is fixing the syntax. The parser fix is very small and looks entirely unrelated. But we can't just fix the parser, that would lead to chaos.

…" warning

Also, use a more precise term in the error message. Unary method generally means
something entirely different!

Generally, was it really worth it to go into so much trouble for a warning
for an edge case? The previous fix overshot, excluding things that
are OK. I still believe it would have been better to do nothing aboit scala#9142.
The time we have already spent on this and the number of code lines we use to
address the issue is in no relation to the benefit of the fix.
@@ -2045,7 +2046,7 @@ object Parsers {
def expr1Rest(t: Tree, location: Location): Tree = in.token match
case EQUALS =>
t match
case Ident(_) | Select(_, _) | Apply(_, _) =>
case Ident(_) | Select(_, _) | Apply(_, _) | PrefixOp(_, _) =>
Copy link
Contributor Author

@odersky odersky Aug 19, 2021

Choose a reason for hiding this comment

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

What must have happened in Scala 2: We had similar logic here, but we did not mark prefix operations specially; so a prefix operation was accepted since it was represented by a Select. Similarly for infix. The following expression is parsed in Scala 2, but will get a type error afterwards:

   1 + 1 = 2

In dotc you get instead a parse error:

-- Error: ../../new/test.scala:4:10 --------------------------------------------
4 |    1 + 1 = 2
  |          ^
  |          end of statement expected but '=' found

That's because we represent infix operations as Infix nodes instead of Apply nodes.

/** PrefixExpr ::= [`-' | `+' | `~' | `!'] SimpleExpr
*/
/** PrefixExpr ::= [PrefixOperator'] SimpleExpr
* PrefixOperator ::= ‘-’ | ‘+’ | ‘~’ | ‘!’
Copy link
Contributor

Choose a reason for hiding this comment

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

+, - and ~ are not working.

class Foo(var value: Int):
  def `unary_+_=`(value: Int): Unit = this.value = +value
  def `unary_-_=`(value: Int): Unit = this.value = -value
  def `unary_~_=`(value: Int): Unit = this.value = ~value
end Foo

def test =
  val x = Foo(9)
  +x = 10 // error: value unary_+ is not a member of Foo - did you mean x.unary_+_=?
  -x = 10 // error: value unary_- is not a member of Foo, but could be made available as an extension method.
  ~x = 10 // error: value unary_~ is not a member of Foo - did you mean x.unary_~_=?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because Foo is missing unary_+ and so on. So that's as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean that we cannot write !x = 3 if we have unary_!_= but not unary_!?

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, exactly

Copy link
Contributor

Choose a reason for hiding this comment

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

Why? Do we typecheck x.unary_! in x.unary_! = 3 before fully desugaring it into x.unarry_!_=(3)?

Copy link
Member

Choose a reason for hiding this comment

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

Something like that, yes. Using a setter foo_= as x.foo = y is only allowed/attempted when x.foo exists itself. This is the same in Scala 2, and I think it's mentioned in the spec somewhere.

@@ -16,6 +16,7 @@ final class Baz private (val x: Int) extends AnyVal {
def unary_- : Baz = ???
def unary_+[T] : Baz = ???
def unary_!() : Baz = ??? // error
def `unary_!_=`() : Baz = ??? // ok
Copy link
Contributor

Choose a reason for hiding this comment

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

This file tests that the definitions of unary operators are well defined. If not an error is emitted.

def `unary_!_=`() is clearly not well defined as it does not receive the value of the RHS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a full version of the equivalent test case for unarry_X_=

Test case

tests/neg-custom-args/fatal-warnings/i9241b.scala

class Foo {
  def `unary_!_=`() : Foo = this // error
  def `unary_+_=`(using Int)(): Foo = this // error
  def `unary_-_=`()(implicit i: Int): Foo = this // error
  def `unary_~_=`[T](): Foo = this // error
}

class Bar {
  def `unary_!_=`(value: Int) : Baz = ???
  def `unary_-_=`[T](value: T)(using T): Baz = ???
  def `unary_~_=`() : Baz = ??? // error
  def `unary_+_=`(value: Int, value2: Int) : Baz = ??? // error
}

final class Baz private (val x: Int) extends AnyVal {
  def `unary_!_=`() : Baz = ??? // error
  def `unary_+_=`(value: Int) : Baz = ???
}

extension (x: Int)
  def `unary_!_=` : Int = ??? // error
  def `unary_+_=`[T] : Int = ??? // error
  def `unary_-_=`() : Int = ??? // error
  def `unary_~_=`(using Int) : Int = ??? // error
end extension

extension (x: Long)
  def `unary_!_=`(value: Int): Int = ???
  def `unary_+_=`[T](value: T) : Int = ???
  def `unary_-_=`(value: Int) : Int = ???
  def `unary_~_=`(value: Int)(using Int) : Int = ???
end extension

extension [T](x: Byte)
  def `unary_!_=` : Int = ??? // error
  def `unary_+_=`[U] : Int = ??? // error
  def `unary_-_=`() : Int = ??? // error
  def `unary_~_=`(using Int) : Int = ??? // error
end extension

extension [T](x: Short)
  def `unary_!_=`(value: Int): Int = ???
  def `unary_+_=`[U](value: T) : Int = ???
  def `unary_-_=`(value: Int) : Int = ???
  def `unary_~_=`(value: Int)(using Int) : Int = ???
end extension

extension (using Int)(x: Float)
  def `unary_!_=` : Int = ??? // error
  def `unary_+_=`[T] : Int = ??? // error
  def `unary_-_=`() : Int = ??? // error
  def `unary_~_=`(using Int) : Int = ??? // error
end extension

extension (using Int)(x: Double)
  def `unary_!_=`(value: Int): Int = ???
  def `unary_+_=`[T](value: T) : Int = ???
  def `unary_-_=`(value: Int) : Int = ???
  def `unary_~_=`(value: Int)(using Int) : Int = ???
end extension

Copy link
Contributor

Choose a reason for hiding this comment

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

See RefChecks.checkUnaryMethods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to make sure and show that we can now parse this. We can break it out in a different test, if you think that's better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then this is not the tests file where this should go. It will be extremely misleading to the next person that looks at the test. I suggest we put its own test file and include a comment that says that we can define it but we will not be able to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can enhance the i9241 checks in a later PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I moved the test and will add the checks in a subsequent PR.

@odersky
Copy link
Contributor Author

odersky commented Aug 20, 2021

@nicolasstucki I have already spent way too much time on this. So can I leave to you to finish?

@odersky odersky assigned nicolasstucki and unassigned odersky Aug 20, 2021
@bishabosha
Copy link
Member

reminder to copy changes from the docs to https://docs.scala-lang.org/scala3/reference/syntax.html

@odersky
Copy link
Contributor Author

odersky commented Aug 23, 2021

reminder to copy changes from the docs to https://docs.scala-lang.org/scala3/reference/syntax.html

I thought that was done automatically by now?

@bishabosha
Copy link
Member

reminder to copy changes from the docs to https://docs.scala-lang.org/scala3/reference/syntax.html

I thought that was done automatically by now?

it seems that depends on this PR: #13350

@bishabosha bishabosha added the new-docs copy changes to the new docs when stabilised label Aug 23, 2021
@odersky odersky merged commit 5eff3d0 into scala:master Aug 23, 2021
@odersky odersky deleted the fix-13282 branch August 23, 2021 14:22
@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-docs copy changes to the new docs when stabilised
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use unary method taking 2 operators
5 participants