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

Rework ElimByName #14295

Merged
merged 10 commits into from
Jan 26, 2022
Merged

Rework ElimByName #14295

merged 10 commits into from
Jan 26, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jan 19, 2022

Rework ElimByName, so that all transformations are now done in one phase, which
implements all of the following transformations:

  1. For types of method and class parameters:

    => T becomes () ?=> T

  2. For references to cbn-parameters:

    x becomes x.apply()

  3. For arguments to cbn parameters

    e becomes () ?=> e

An optimization is applied: If the argument e to a cbn parameter is already
of type () ?=> T and is a pure expression, we avoid (2) and (3), i.e. we
pass e directly instead of () ?=> e.apply().

Note that () ?=> T cannot be written in source since user-defined context functions
must have at least one parameter. We use the type here as a convenient marker
of something that will erase to Function0, and where we know that it came from
a by-name parameter.

Note also that the transformation applies only to types of parameters, not to other
occurrences of ExprTypes. In particular, embedded occurrences in function types
such as (=> T) => U are left as-is here (they are eliminated in erasure).
Trying to convert these as well would mean traversing all the types, and that
leads to cyclic reference errors in many cases. This can cause problems in that
we might have sometimes a () ?=> T where a => T is expected. To compensate,
there is a new clause in TypeComparer#subArg that declares () ?=> T to be a
subtype of => T for arguments of type applications at any point after this phase
and up to erasure.

@odersky
Copy link
Contributor Author

odersky commented Jan 19, 2022

Can someone help figure out why the analyzer plugin test fails? @liufengyun do you have an idea?

@olhotak
Copy link
Contributor

olhotak commented Jan 19, 2022

In ./sbt-test/sbt-dotty/analyzer-plugin/plugin/Analyzer.scala line 34:
override val runsBefore = Set(ElimRepeated.name)
Probably should be updated to ProtectedAccessors.

@odersky
Copy link
Contributor Author

odersky commented Jan 19, 2022

@olhotak That's probably it. Thanks for figuring it out!

@@ -31,7 +31,7 @@ class InitChecker extends PluginPhase {
val phaseName = "symbolTreeChecker"

override val runsAfter = Set(SetDefTree.name)
override val runsBefore = Set(ElimRepeated.name)
override val runsBefore = Set(ProtectedAccessors.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

ProtectedAccessors needs to be imported at line 17.

@odersky odersky changed the title Move elim repeated earlier Rework ElimByName Jan 20, 2022
@odersky odersky marked this pull request as ready for review January 20, 2022 13:41
I tried to extend specialization to all context functions, not just ones of 0 arity.
But that runs into problems for dependent context functions, since the necessary casts
get complicated. Since context functions over primitive types are an anti-pattern anyway
I don't think we need to optimize this case, after all.
 - Split out forward reference checks and cross version (i.e. experimental/deprecated) checks
   into their own miniphases. These have nothing to do with the core tasks of RefChecks.
 - Move RefChecks earlier in the pipeline, so that it is not affected by ElimByName's questionable
   type manipulations.
@odersky
Copy link
Contributor Author

odersky commented Jan 20, 2022

This PR now also contains a rework of RefChecks:

  • Split out forward reference checks and cross version (i.e. experimental/deprecated/since) checks
    into their own miniphases. These have nothing to do with the core tasks of RefChecks.
  • Move RefChecks earlier in the pipeline, so that it is not affected by ElimByName's questionable
    type manipulations.

RefChecks is one of the last things that we ported from Scala 2 and left largely unchanged. Despite being formally a miniphase, it was really the typical Scala 2 megaphase that mixed many different things. We could also go further and break out other things. There are still some checks that have nothing to do with overriding or overloading: checkImplicitNotFoundAnnotation and checkUnaryMethods, for instance.

Perform the overriding checks after elimByName. I observed some problem with
catsEffect2, where a super accessor method with a `() ?=> T` parameter was compared with a
corresponding super accessor method with a `=> T` parameter. One of these methods was generated
before elimByName, the other after. So comparing them at phase elimRepeated + 1 gave two different
types. The problem is fixed by comparing after elimByName, which means that the type of the second
method is converted to match the first.
@odersky odersky merged commit 5543ea8 into scala:master Jan 26, 2022
@odersky odersky deleted the test-elim-repeated branch January 26, 2022 16:57
griggt added a commit to dotty-staging/cats-effect that referenced this pull request Feb 2, 2022
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.

4 participants