Skip to content

Fix #3246: Refine handling of postfix _ for non-functions #3300

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

Closed
wants to merge 2 commits into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 10, 2017

No description provided.

@odersky
Copy link
Contributor Author

odersky commented Oct 10, 2017

@allanrenucci All patterns you showed in #3246 should now be handled without error under -language:Scala2

@odersky odersky requested a review from allanrenucci October 10, 2017 15:57
@allanrenucci
Copy link
Contributor

I'll try to revert the workaround in scalatest and see if I can compile with this patch

@allanrenucci
Copy link
Contributor

class Test {
  def foo(fun: () => Unit) = ???
  def bar(fun: => Any) = foo(fun _)
}

This test case still does not compile with Dotty

@odersky
Copy link
Contributor Author

odersky commented Oct 10, 2017

Here's the error message I get (btw, for future reference, it would be good to add the actual error message to an issue or comment).

11 |  def bar(fun: => Any) = foo(fun _)
   |                             ^^^
   |                             found:    () => Any
   |                             required: () => Unit

This looks OK to me. Why should it compile? I don't see how fun _ could expand to anything but a () => Any. I think this is a bug in scalac.

@allanrenucci
Copy link
Contributor

I agree it shouldn't compile. But then, should the following code compile?

class Test {
  def foo(fun: () => Unit) = ???
  def bar(fun: => Any) = foo(() => fun)
}

if (pt1.eq(AnyFunctionProto) && !defn.isFunctionClass(res.tpe.classSymbol)) {
ctx.errorOrMigrationWarning(i"not a function: ${res.tpe}; cannot be followed by `_'", tree.pos)
val nestedCtx = ctx.fresh.setNewTyperState()
var res = typed(qual, pt1)(nestedCtx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be a val now

@allanrenucci
Copy link
Contributor

As a side effect of this PR, we now accept this code under -language:Scala2 (scalac rejects it)

class Test {
  def foo(x: Int) = 1
  val bar: () => Int = foo(1) _
}

It is definitely an improvement as we used to emit a type error:

-- [E007] Type Mismatch Error: tests/allan/Test.scala:3:26 ---------------------
3 |  val bar: () => Int = foo(1) _
  |                       ^^^^^^
  |                       found:    Int
  |                       required: () => Int
  |                       

one error found

Now:

-- Migration Warning: tests/allan/Test.scala:3:30 ------------------------------
3 |  val bar: () => Int = foo(1) _
  |                       ^^^^^^^^
  |                       not a function: foo(1); cannot be followed by `_'

@allanrenucci
Copy link
Contributor

class Test {
  def foo = 1
  val bar: () => Int = foo _
}
-- Migration Warning: tests/allan/Test.scala:3:27 ------------------------------
3 |  val bar: () => Int = foo _
  |                       ^^^^^
  |                       not a function: foo; cannot be followed by `_'

I think the warning is misleading in this case. If foo is defined as def foo(), then no warning will be emitted at all, even though the _ is not needed. I think we should depreciate _ in all cases.

@allanrenucci
Copy link
Contributor

class Test {
  def foo(x: Int) = 1
  val bar: () => Int = foo _
}

This now crashes under -language:Scala2

> dotc -d out tests/allan/Test.scala -language:Scala2 -migration
[info] Running dotty.tools.dotc.Main -d out tests/allan/Test.scala -language:Scala2 -migration -classpath /Users/renucci/Documents/Projects/dotty/library/target/scala-2.12/dotty-library_2.12-0.4.0-bin-SNAPSHOT-nonbootstrapped.jar
-- Migration Warning: tests/allan/Test.scala:3:27 ------------------------------
3 |  val bar: () => Int = foo _
  |                       ^^^^^
  |                       not a function: foo; cannot be followed by `_'

exception occurred while typechecking tests/allan/Test.scala

exception occurred while compiling tests/allan/Test.scala
Exception in thread "main" java.lang.AssertionError: assertion failed
	at scala.Predef$.assert(Predef.scala:204)
	at dotty.tools.dotc.ast.Trees$Tree.withType(Trees.scala:121)
	at dotty.tools.dotc.typer.Typer.$anonfun$typedTypeTree$1(Typer.scala:1071)
	at dotty.tools.dotc.util.Stats$.track(Stats.scala:35)
	at dotty.tools.dotc.typer.Typer.typedTypeTree(Typer.scala:1056)
	at dotty.tools.dotc.typer.Typer.typedUnnamed$1(Typer.scala:1653)
	at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:1680)
	at dotty.tools.dotc.typer.Typer.$anonfun$typed$2(Typer.scala:1697)
	at dotty.tools.dotc.reporting.Reporting.traceIndented(Reporter.scala:140)
	at dotty.tools.dotc.reporting.Reporting.traceIndented$(Reporter.scala:139)
	at dotty.tools.dotc.core.Contexts$Context.traceIndented(Contexts.scala:57)
	at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1693)
	at dotty.tools.dotc.typer.Namer.$anonfun$typedAheadType$1(Namer.scala:940)
	at dotty.tools.dotc.typer.Namer.typedAheadImpl(Namer.scala:933)
	at dotty.tools.dotc.typer.Namer.typedAheadType(Namer.scala:940)
	at dotty.tools.dotc.typer.Namer.valOrDefDefSig(Namer.scala:1116)
	at dotty.tools.dotc.typer.Namer$Completer.typeSig(Namer.scala:732)

Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

I can confirm that we can now compile all the postfix _ in scalatest under scala2 mode.

Now this compile under scala2 mode:

class Test {
  def foo(fun: () => Unit) = ???

  def bar(fun: => Int) = foo(fun _) // accepted by scalac
  foo(1 _) // rejected by scalac
}

You said previously that it was a bug in scalac. I guess it is fine to support this cases under scala2 mode (even if it is a bug) if we reject them under "normal" mode

@@ -0,0 +1,4 @@
class Test {
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 we should also compile it under scala2 mode, since it only crashed in this mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Can you make the change and merge the PR when done? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Will do

@allanrenucci
Copy link
Contributor

Superseded by #3402

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.

2 participants