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 individual erased parameters #16507

Merged

Conversation

natsukagami
Copy link
Contributor

@natsukagami natsukagami commented Dec 12, 2022

Description

Syntax

Erased parameters in a method / lambda comes with an erased modifier before its name:

def erasedSecondParam(x: Int, erased y: Int): Int = x
type EraseSecondParam[T, U] = (T, erased U) => T

val esp: EraseSecondParam[Int, Int] = (x, erased y) => erasedSecondParam(x, y)

This is a breaking change, as previously erased methods / functions with multiple parameters now only have its first parameter erased.

Semantics

[Impure][Contextual]ErasedFunctionN traits are no longer available. Instead, erased function values are denoted by refining the scala.runtime.ErasedFunction trait:

type Int_EInt = (Int, erased Int) => Int
// is equivalent to
type Int_EInt2 = scala.runtime.ErasedFunction {
  def apply(x$0: Int, erased x$1: Int): Int
}

They are subsequently compiled (during Erasure) into [Contextual]FunctionM where M is the number of non-erased parameters.

Erased Classes

Any parameter that is an instance of an erased class is automatically erased. This is different from before, where the parameters are erased only if all parameters are instances of erased classes.

@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch 2 times, most recently from fb6d70f to d200a4b Compare December 12, 2022 18:17
@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch 3 times, most recently from 5e50053 to 7fe8b14 Compare December 20, 2022 13:21
@natsukagami natsukagami marked this pull request as ready for review December 21, 2022 17:36
library/src/scala/ErasedFunction.scala Outdated Show resolved Hide resolved
library/src/scala/ErasedFunction.scala Outdated Show resolved Hide resolved
library/src/scala/ErasedFunction.scala Outdated Show resolved Hide resolved
library/src/scala/ErasedFunction.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Types.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Definitions.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Types.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala Outdated Show resolved Hide resolved
@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch 2 times, most recently from ac13659 to 0cd5c46 Compare January 11, 2023 07:54
@natsukagami
Copy link
Contributor Author

Thanks for taking a look at my PR! Re: interaction with poly functions, afaik poly functions don’t handle erased parameters at all right now. I think having poly functions being a refined PolyFunction & ErasedFunction should work, but I agree that at this point we might just want to have a refined Function trait as you mentioned. I want to discuss this a bit further in person.

@natsukagami
Copy link
Contributor Author

natsukagami commented Jan 12, 2023

If we pickle this information after the flags

This seems promising, but since we are pickling the mods just as the tail of methodic trees, we can't easily do both.

Perhaps if we just attach the erased parameters right next to the ERASED flag? I think it will work (perhaps with another flag, since ERASED was also used for erased vals) but it seems to be kinda messy.

I will think a bit more about this.

@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch from ce8a90b to 2ca5b78 Compare January 13, 2023 10:01
@natsukagami
Copy link
Contributor Author

I implemented the above approach, still with the ERASED flag, but I think the implementation is clean enough. I also did bit-packing, though I didn't find the logic doing the same thing.

@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch 2 times, most recently from 2f3159a to 176b23c Compare January 17, 2023 10:51
compiler/src/dotty/tools/dotc/core/TypeComparer.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/transform/Erasure.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/parsing/Parsers.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/parsing/Parsers.scala Outdated Show resolved Hide resolved
@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch from 1bfd88e to 60c157d Compare January 23, 2023 16:14
@smarter
Copy link
Member

smarter commented Jan 23, 2023

  • It's a detail, but I think scala.runtime is a bit misleading for something that only exists at compile-time. PolyFunction is in scala because the [X] => ... syntax is just syntactic sugar, just like A => B is syntactic sugar for scala.Function1[A, B].
  • In the past i explored a bit having a generic Function trait to handle all function types and while it seems possible it would take quite a bit of work, and we'd have to carefully check the impact on compile-time performance.
  • We realized recently that some things like eta-expansion would be much simpler if [X] => A => B was encoded as PolyFunction { def apply[X]: A => B }. Unfortunately it's too late to change this now since it would break binary compatibility, but it might make sense to encode a polymorphic function with erased parameters as PolyFunction { def apply[X]: ErasedFunction { ... } }, not sure.

@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch from 60c157d to b810b00 Compare January 24, 2023 09:44
@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch from 2093b87 to 3e172e0 Compare January 30, 2023 11:42
@nicolasstucki nicolasstucki added needs-minor-release This PR cannot be merged until the next minor release and removed needs-minor-release This PR cannot be merged until the next minor release labels Feb 7, 2023
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.

The only missing change is to update Quotes.scala to align to this new scheme.

docs/_docs/internals/syntax.md Show resolved Hide resolved
compiler/src/dotty/tools/dotc/cc/Setup.scala Outdated Show resolved Hide resolved
@@ -2140,7 +2140,7 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler

given MethodTypeMethods: MethodTypeMethods with
extension (self: MethodType)
def isErased: Boolean = self.isErasedMethod
def isErased: Boolean = self.hasErasedParams
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is not right, it should probably return false as in the new scheme method types are not erased themself. isErased should be deprecated in Quotes.scala.

We have some changes in semantics of:

  • MethodTypeMethods.isErased: Should be deprecated and replaced with erasedParams and maybe hasErasedParams
  • TermParamClauseMethods.isErased: Should be deprecated and replaced with erasedArgs and maybe hasErasedArgs
  • TypeReprMethods.isErasedFunctionType: we could make it work by detecting the refined function type. Though it might be cleaner to deprecate and add a more consistent API.
  • defn.FunctionClass: needs to be deprecated. Should not have had default parameters in the first palce.

We can only deprecate these methods in a minor release.

What we should include in this PR are the new replacement as @experimental methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

I created #16847 to track this

Copy link
Contributor

Choose a reason for hiding this comment

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

And started the work on defn.FunctionClass in #16849

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
def isErased: Boolean = self.hasErasedParams
def isErased: Boolean = false

same for TermParamClauseMethods.isErased.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could change the documentation of TypeReprMethods.isErasedFunctionType in Quotes.scala to be

        /** Is this type a function type with erased parameters?
        *
        *  @see `isFunctionType`
        */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • MethodTypeMethods.isErased: Should be deprecated and replaced with erasedParams and maybe hasErasedParams

  • TermParamClauseMethods.isErased: Should be deprecated and replaced with erasedArgs and maybe hasErasedArgs

  • TypeReprMethods.isErasedFunctionType: we could make it work by detecting the refined function type.

This is what I did, alongside with crashing FunctionClass(isErased = true) and adding ErasedFunctionClass (that just returns the scala.compiletime.ErasedFunction symbol).

@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch 3 times, most recently from 02435f1 to 9b13653 Compare February 8, 2023 00:49
* }
*
* ErasedFunctionN and ErasedContextFunctionN erase to Function0.
*
* ImpureXYZFunctionN follow this template:
*
* type ImpureXYZFunctionN[-T0,...,-T{N-1}, +R] = {*} XYZFunctionN[T0,...,T{N-1}, R]
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 work with erased parameters?

The old encoding seemed to support ImpureErasedFunctionN/ImpureErasedContextFunctionN. With the new encoding we do not have an equivalent XYZFunctionN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I don't think so. I've been trying to see how poly functions deal with it, but it seems like poly functions just make scalac crash with CC on (#16871)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the crash fixed I think it doesn't work with poly functions as well. I wonder if we can change the parser to recognize fat arrows and add a capturing wrapper for both poly functions and erased functions?

@@ -2140,7 +2140,7 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler

given MethodTypeMethods: MethodTypeMethods with
extension (self: MethodType)
def isErased: Boolean = self.isErasedMethod
def isErased: Boolean = self.hasErasedParams
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
def isErased: Boolean = self.hasErasedParams
def isErased: Boolean = false

same for TermParamClauseMethods.isErased.

@@ -2140,7 +2140,7 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler

given MethodTypeMethods: MethodTypeMethods with
extension (self: MethodType)
def isErased: Boolean = self.isErasedMethod
def isErased: Boolean = self.hasErasedParams
Copy link
Contributor

Choose a reason for hiding this comment

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

We could change the documentation of TypeReprMethods.isErasedFunctionType in Quotes.scala to be

        /** Is this type a function type with erased parameters?
        *
        *  @see `isFunctionType`
        */

@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch 3 times, most recently from faea51f to 6d639ec Compare February 16, 2023 10:42
@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch 2 times, most recently from e1cc96c to 41db4f5 Compare February 23, 2023 14:09
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.

Otherwise LGTM

library/src/scala/compiletime/ErasedFunction.scala Outdated Show resolved Hide resolved
library/src/scala/quoted/Quotes.scala Outdated Show resolved Hide resolved
compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala Outdated Show resolved Hide resolved
@nicolasstucki
Copy link
Contributor

@natsukagami It seems good. Could you squash the commits?

Breaking change for erasedDefinitions: this effectively makes the
current `erased` marker in parameter list apply to only the first
parameter.

    def f(erased a: int, b: int)

should now be written as

    def f(erased a: int, erased b: int)

    type Function1 = (x: Int, erased y: Int) => Int
    type Function2 = (Int, erased Int) => Int

Use refined traits for erased functions

- Function types with erased parameters are now always `ErasedFunction` refined with the correct `apply` definition,
  for example:

    scala.runtime.ErasedFunction {
        def apply(x1: Int, erased x2: Int): Int
    }
  where ErasedFunction is an @experimental empty trait.
- Polymorphic functions cannot take erased parameters.
- By-name parameters cannot be erased.
- Internally, use the @ErasedParam annotation as a marker for an erased parameter.
- Parameters that are erased classes are now marked `erased` at Typer phase (with an annotation),
  and in later phases, they are not taken into account when considering erasedness.
- Erased parameters/functions quotes API are changed:
    - `isErased` => `erasedArgs`/`erasedParams` and `hasErasedArgs`/`hasErasedParams`
    - `FunctionClass` now fails when `isErased = true`. Add `ErasedFunctionClass`.
- Added tests and test-fixes for `erasedDefinitions` feature
- Updated specs and internal syntax
- Aside, reject normal tuples with ValDefs in them. This comes up when trying to parse parameters out of tuples.
  In practice they don't show up.

Co-authored-by: Nicolas Stucki <nicolas.stucki@gmail.com>
@natsukagami natsukagami force-pushed the individual-erased-params-take-2 branch from ba78c70 to 0f7c3ab Compare March 1, 2023 09:42
@natsukagami natsukagami merged commit e422066 into scala:main Mar 3, 2023
@bishabosha
Copy link
Member

bishabosha commented Apr 18, 2023

This seems to change the erasure for context functions with erased parameter:

@Linyxus found this example

object TestyFactorial:
  import scala.annotation.tailrec
  import language.experimental.erasedDefinitions

  erased class Foo1
  class Foo2

  @tailrec
  final def test1(n: Int, acc: Int): Foo1 ?=> Int =
    if n <= 0 then acc
    else test1(n - 1, acc * n)

  @tailrec
  final def test2(n: Int, acc: Int): Foo2 ?=> Int =
    if n <= 0 then acc
    else test2(n - 1, acc * n)

before this change the erasure of test1 is @ContextResultCount(1) @tailrec def test1(n: Int, acc: Int): Int, but now it is @tailrec final def test1(n: Int, acc: Int): Function0

odersky added a commit that referenced this pull request Feb 12, 2024
…19654)

Fixes #19641 

How we got here:

Originally, overloading resolution for types that were not applied was
handled like this:
```scala
      case defn.FunctionOf(args, resultType, _) =>
        narrowByTypes(alts, args, resultType)

      case pt =>
        val compat = alts.filterConserve(normalizedCompatible(_, pt, keepConstraint = false))
        if (compat.isEmpty)
          /*
           * the case should not be moved to the enclosing match
           * since SAM type must be considered only if there are no candidates
           * For example, the second f should be chosen for the following code:
           *   def f(x: String): Unit = ???
           *   def f: java.io.OutputStream = ???
           *   new java.io.ObjectOutputStream(f)
           */
          pt match {
            case SAMType(mtp, _) =>
              narrowByTypes(alts, mtp.paramInfos, mtp.resultType)
            case _ =>
              // pick any alternatives that are not methods since these might be convertible
              // to the expected type, or be used as extension method arguments.
              val convertible = alts.filterNot(alt =>
                  normalize(alt, IgnoredProto(pt)).widenSingleton.isInstanceOf[MethodType])
              if convertible.length == 1 then convertible else compat
          }
        else compat
```
Note the warning comment that the case for SAM types should not be moved
out, yet we do exactly the same thing for plain function types. I
believe this was simply wrong, but it was not discovered in a test.

Then in #16507 we changed the `defn.FunctionOf` extractor so that
aliases of function types were matched by it. This triggered test
failures since we now hit the wrong case with aliases of function types.

In #18286, we moved the extractor test around, but that was not enough,
as #19641 shows. Instead the test for `FunctionOf` should be aligned
with the test for SAM case. But it turns out that's not even necessary
since the
preceding `val compat = ...` handles function prototypes correctly by
simulating an eta expansion. So in the end
we could simply delete the problematic case.
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