Skip to content

Conversation

@olhotak
Copy link
Contributor

@olhotak olhotak commented Feb 4, 2015

Depends on #339 and #345.

Changes phases up to Erasure to handle AnnotatedTypes transparently like their underlying type.

In Erasure, all type annotations are removed (AnnotatedTypes are replaced with their underlying). Without this, the backend blows up in various ways when it encounters an AnnotatedType.

These changes are necessary and almost sufficient to make the current test suite pass if Typer.adapt is modified to attach an annotation to the type of every tree whose type isValueType.

The remaining tests that fail when all such types are annotated are:

  • dotc_core due to dotc_core test fails when Config.cacheImplicitScopes == false #347
  • lambdalift in pos_all due to a symbol with invalid period: a denot for module C in method foo2 has a period of SyntheticMethods, but denot is called on it from a context in FirstTransform.
  • neg_i0091_infpaths loops due to an unboundedly growing TermRef (whose prefix is a shorter version of itself) that seems to only grow when it is wrapped in an AnnotatedType.

odersky and others added 9 commits January 27, 2015 12:46
Overriding pairs needs to match ExprTypes with field types. Closes scala#329.
Reformulated matchign spec and implemented accordingly. Previous
fix for scala#329 would have missed third new error case in over.scala.
1) Drop redundant signature comparison in overriding pairs
2) Abstract from repeated parameters when calculating matches
As noticed by @retronym, Any and Object are not identified when matching Scala and Java methods. I believe this is because the Java method does not have the Java flag set. @olhotak can you take a look?
This was a left-over from a failed attempt to have OverridingPiars work exclusively by comparing signatures.
If we do that then repeated and underlying do have the same signature and therefore are supposed to match.
But as @retronym notes, this leads to problems. In any case, we no longer try to make overriding pairs
work that way, because it fails for other reasons as well.
… Any

Fixes two bugs needed for java-override test:

Namer was creating a MethodType instead of a JavaMethodType even though
the JavaDefined flag was set on the DefDef.

Following Scalac, Namer needs to convert Java method parameters
of type j.l.Object to s.Any.
@olhotak
Copy link
Contributor Author

olhotak commented Feb 5, 2015

I fixed a few more cases where AnnotatedTypes were not handled transparently. This fixes the neg_i0091_infpaths test.

@samuelgruetter
Copy link
Contributor

This fixes the neg_i0091_infpaths test.

but that's by accident, I guess, because it changes the completion order. So it's not clear if the problem is really fixed, or just "not exposed any more by neg/i0091_infpaths".

@olhotak
Copy link
Contributor Author

olhotak commented Feb 5, 2015

No, the neg_i0091_infpaths test was looping due to the missing stripAnnots in isInteresting. As a result, checkNonCyclic failed to detect a cycle and turn it into an error.

@samuelgruetter
Copy link
Contributor

On what branch/commit was this the case? I've just tested tests/neg/i0091-infpaths.scala on master (1ab02ce) and on this PR (211fe56), and the cycle is detected in both.
Looks like this was already fixed in #279.

@olhotak
Copy link
Contributor Author

olhotak commented Feb 5, 2015

i0091-infpaths is not a problem normally (it passes).

It was only a problem when types have annotations, because the AnnotatedTypes were not being handled transparently in many places (including isInteresting). This PR fixes most of the places where AnnotatedTypes were not being handled transparently, so that all of the test suite (except lambdalift in pos_all, currently) now passes even if you annotate the type of every tree whose type is a value type. (Annotating non-value types still breaks many things, but non-value types are less likely to be annotated.)

@samuelgruetter
Copy link
Contributor

Ok, got it, so that commit message has to be understood as "neg_i0091_infpaths now also passes if Typer.adapt is modified to attach an annotation to the type of every tree whose type is a value type".

@olhotak
Copy link
Contributor Author

olhotak commented Feb 5, 2015

Oh, right, I see why it is confusing.

I should probably squash the two commits into one, with only the commit message of the first one, with no mention of the specific test (since before the first commit, almost all of the tests fail if types have annotations). Would that make sense?

@samuelgruetter
Copy link
Contributor

I think so :)

In Erasure, all type annotations are dropped (AnnotatedTypes are
replaced with their underlying).
@olhotak olhotak force-pushed the pr-transparentannotatedtypes branch from 211fe56 to 20673f4 Compare February 6, 2015 07:37
@olhotak
Copy link
Contributor Author

olhotak commented Feb 6, 2015

I have squashed the two commits. Sorry for the confusion.

@olhotak
Copy link
Contributor Author

olhotak commented Feb 6, 2015

I investigated the failure in lambdalift when types are annotated.

transformStats in FirstTransform calls reorderAndComplete with ctx.withPhase(thisTransformer.next)). I don't understand why the phase is changed.

reorderAndComplete creates the symbol for the module class and gives it its denotation, with a period in the next phase, SyntheticMethods.

Still in FirstTransform, the type assigner attempts to assign a type to a Block. This calls TypeAssigner.avoid, which eventually asks for the denotation of the symbol. The context is in phase FirstTransform but the symbol already has a later denotation, so this causes a crash.

Why does this not happen when types are not annotated? TypedTreeCopier.Block contains a test (expr.tpe eq tree.expr.tpe). If this is true, the type assigner is not used for that tree. When types are not annotated, this test always succeeds and the type assigner is never called (for this test). When types are annotated, expr.tpe and tree.expr.tpe have the same type but different annotations, so the test fails, the type assigner is called, and crashes.

@DarkDimius
Copy link
Contributor

transformStats in FirstTransform calls reorderAndComplete with ctx.withPhase(thisTransformer.next)). I don't understand why the phase is changed.

Most phases actually transform the tree at ctx.withPhase(thisTransformer.next)), because that's the earliest phase when type(denot) transformations defined by phase have been applied.

Can you please tell me how to reproduce the particular error that you are seeing(and maybe share an error log). LambdaLift is very far away in the pipeline from FirstTransform so such failure is strange.

@olhotak
Copy link
Contributor Author

olhotak commented Feb 6, 2015

The problem has nothing to do with the LambdaLift phase. It just occurs when compiling tests/pos/lambdalift.scala with Dotty, in the FirstTransform phase.

You can get my branch here: olhotak/dotty@b5e05fde5

Here is the output: https://gist.github.com/olhotak/67c4aca12a1b39814643

@olhotak olhotak closed this Feb 12, 2015
tgodzik added a commit to tgodzik/scala3 that referenced this pull request Apr 29, 2025
Backport "Warn if interpolator uses toString" to 3.3 LTS
Gedochao added a commit to Gedochao/dotty that referenced this pull request Sep 29, 2025
Fix val parameter override problem
v0.2.1 -- fix potential IDB API issue
dep: update a whole bunch of things (scala#71)

This pr updates the following:

  - various dependencies to the latest
  - adds in dependabot
  - updates java and checkout actions
  - updates ammonite scripts and uses new ammonite in CI
  - updates mill to the latest 0.10.5
  - bumps scalajs to 1.x
  - corrects repo location in pomSettings
Merge pull request scala#4238 from armanbilge/issue/4229

Build docs on 2.13
Merge pull request scala#350 from typelevel/update/sbt-typelevel-0.4.13

Update sbt-typelevel to 0.4.13
Merge pull request scala#187 from typelevel/update/sbt-typelevel-0.4.13

Update sbt-typelevel to 0.4.13
Merge pull request scala#318 from scala-steward/update/sbt-typelevel-0.4.3

Update sbt-typelevel, sbt-typelevel-site to 0.4.3
fix http4s output encoding to UTF-8 (scala#900)

support scala 3.0.0
Migrate to sbt slash syntax
Merge pull request scala#2951 from pchlupacek/patch-1

Update adopters.md
Adapt to new restrictions in fewerBraces
wip: cleanups
Reduce program size in one of the tests to decrease the chance of stack overflow.
v4.6.0
Update scalafmt-core to 3.4.0
Remove import suggestions from assertions
Clarify the unit of proc().call() 'timeout' parameter (scala#107)

Pull request: com-lihaoyi/os-lib#107
Setting version to 2.4.1-SNAPSHOT
Update auxlib, javalib, junit-plugin, ... to 0.4.3
Update serialization base64 for new lazy vals
Merge pull request scala#616 from SethTisue/enable-scala-3-publishing

Disable ExpectationsSpecs message tests
Fix test for change in union type inference
empty
Merge pull request scala#380 from scala-steward/update/sbt-typelevel-0.4.3

Update sbt-typelevel to 0.4.3
Update scalafmt-core to 3.7.12 (scala#163)

Co-authored-by: typelevel-steward[bot] <106827141+typelevel-steward[bot]@users.noreply.github.com>
test: use the same line numbers in the location spec

regardless of the scala compiler version
Disable buggy test

See scala#16390
New reference compiler is Scala 2.13.12
deps: update a bunch of things (scala#395)

* deps: update a bunch of things

This updates the following:

  - make sure Mill is on the latest 0.10.5
  - drops support for scalajs 0.x
  - bumps to the latest scala patch versions
  - bumps to the latest scalaJS and scala native
  - bumps to 0.8.0 of utest
  - adds in dependabot
  - adds in cross testing jdk for 8 and 17
  - bumps version of checkout and java actions

* refactor: restructure build to actually build

* fix: make mima happy by updating
Merge pull request scala#264 from scala-steward/update/test-interface-0.4.7

Update test-interface to 0.4.7
Merge pull request scala#183 from scala-steward/update/auxlib-0.4.3

Update auxlib, javalib, nativelib, nscplugin, ... to 0.4.3
Revert "Fix tests to work with changes after scala#15569"

This reverts commit 912b4f887912792202aa76e93fd19e63bd62f3bc.
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