Avoid compiler crash with missing transitive dependencies #5671

retronym · 2017-02-02T20:45:53Z

When we unpickle a reference to a.b.C and we don't have a package symbol for a.b, we synthesize a stub symbol. This code was modifed in b47aaf6 to mutate the flags of a stub symbol rather than trying to create a particular subclass of StubSymbol when we figure out that a stub represents a package. However, this neglected to set the MODULE flag, which can lead to an assertion error later in the compiler.

This PR includes a direct fix for this flag error.

It also includes a few improvements to avoid forcing symbol info transformers in the compiler backend that I identified during the investigation.
I have a few more changes of this nature in another branch that I'll revisit.

retronym · 2017-02-03T00:00:04Z

TODO:

Does this fix SI-10171 ?

retronym · 2017-02-04T19:34:45Z

@lrytz Could you please take a look at these commits to see if they are on the right track?

The first one avoids tripping an assertion with a stub symbol during symbol substitution that was happening in Uncurry's info transformer, which had dealiased a method parameter from Predef.String to java.lang.String, but crashed because the method return type was a stub. Special casing boundSyms avoids the problem there.

The second one independentally fixes the problem by stopping the propagation of info transformers sooner, by disabling the specialization transform when the compiler has reached the backend. I believe this is safe, even for multi-run compilation (in that case we discard all but the first element of the info history on the next run).

The specialization info transform is the worst offender in forcing method infos. We could potentially disable parts of other info transforms under the same condition, e.g. the addition of outer accessor methods in ExplicitOuter.

The final commit attempts to lazify part of the conversion from Symbols to BTypes. I haven't validated whether or how much this actually limits the traversal of symbol infos.

A test project is attached to https://issues.scala-lang.org/browse/SI-10171, I'm going to try to extract a run test from that.

retronym · 2017-02-04T20:42:38Z

@lrytz I've added a test case and another simple fix (give the stub symbol the MODULE flag). However, I think the other commits still have value: avoiding needless work limits our surface area to future bugs and helps performance.

retronym · 2017-02-04T20:44:46Z

/cc @jvican

retronym · 2017-02-05T04:17:33Z

/rebuild

retronym · 2017-02-05T16:34:31Z

/rebuild

retronym · 2017-02-05T19:48:38Z

src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala

   *  If it is a 'no-specialization' run, it is applied only to loaded symbols.
   */
  override def transformInfo(sym: Symbol, tpe: Type): Type = {
-    if (settings.nospecialization && currentRun.compiles(sym)) tpe


The backend needs to specialize TupleN to populate its list of tuple constructors to make tests for box-elimination to work. This seems to be the only case covered by our test suite that requires specialization of a type after the specialization phase itself has run.

lrytz

This looks good! Also the commit introducing lazy completion, though I only took a quick look.

lrytz · 2017-02-06T12:12:45Z

src/reflect/scala/reflect/internal/Types.scala

      else super.safeToString
    override def narrow: Type = this
    override def kind = "ThisType"
+    override def boundSyms = if (sym.isInstanceOf[StubSymbol]) emptySymbolSet else super.boundSyms


Looks OK, but looking around for a while, it seems that allowing to access tpe on stub symbols is a feature. So I prefer the fix in your next commit (MODULE flag for stub package symbols), and we could live without the special-case here.

scala> import scala.reflect.internal.Flags._ scala> val c = definitions.ScalaPackageClass.newStubSymbol(newTypeName("C"), "not here") scala> c.tpe res7: $r.intp.global.Type = C scala> val c = definitions.ScalaPackageClass.newStubSymbol(newTypeName("C"), "not here") scala> c.setFlag(PACKAGE) scala> c.tpe java.lang.IllegalArgumentException: requirement failed: package C at scala.reflect.internal.Types$ModuleTypeRef.<init>(Types.scala:1879) scala> val c = definitions.ScalaPackageClass.newStubSymbol(newTypeName("C"), "not here") scala> c.setFlag(PACKAGE | MODULE) scala> c.tpe res12: $r.intp.global.Type = C.type

While looking at boundSyms, do you know why ExistentialType.boundSyms doesn't ++ underlying.boundSyms?

While looking at boundSyms, do you know why ExistentialType.boundSyms doesn't ++ underlying.boundSyms?

Looks like it is called repeatedly as the substituition TypeMap recurses, so its doing the right thing by being shallow.

Come to think of it, I suspect we could actually just return null (in SimpleTypeProxy.boundSyms) and have the same results as we do today.

Would you like to keep this override, given the other more fundamental fix? I'd remove it, but either way is fine.

just a quick ping here, then we can merge.

Does this mean the review is currently still pending? Lets make that clear with "request changes" review. I was about to merge this PR because the review status was "approved".

ok, yes, i just wanted @retronym to consider this once more.

I've removed the override for now. I think we could actually generalize it to:

class SubType { override def boundSyms = emptySymbolSet }

Along with overrides to pick that:

abstract case class ThisType { override def boundSyms = super[SubType].boundSyms } ...

But let's rely on underlying.boundSyms for now.

lrytz · 2017-02-06T13:02:31Z

src/reflect/scala/reflect/internal/pickling/UnPickler.scala

      def readThisType(): Type = {
        val sym = readSymbolRef() match {
-          case stub: StubSymbol => stub.setFlag(PACKAGE)
+          case stub: StubSymbol => stub.setFlag(PACKAGE | MODULE)


This looks good.

lrytz · 2017-02-08T09:21:14Z

src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala

+          assert(!currentRun.compiles(sym), sym)
+          // Skip specialization info transform for third party classes that aren't referenced directly
+          // from the tree or by the specialization info transform itself that are run up to the end of
+          // the specialization phase.


this comment is a bit hard to read

lrytz · 2017-02-08T09:21:39Z

src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala

+    override def apply(unit: CompilationUnit): Unit = {
+      super.apply(unit)
+      FunctionClass.seq.map(_.info)
+      TupleClass.seq.map(_.info)


there should be a comment here

retronym · 2017-02-17T06:39:40Z

@lrytz I've trimmed this down to the essential commit and some low-risk passengers.

In this test case, the backend forces the specialization info transform of `Sub` during computation of its inner class metadata. This in turn runs the info transforms of the `Base`. This leads to the uncurry info tranform transforming a signature that has a type alias as a method parameter type. Subsequent substution of the new method symbol into the result type, which includes a stub symbol for an absent class, tripped an assertion: ``` requirement failed: package b java.lang.IllegalArgumentException: requirement failed: package b at scala.Predef$.require(Predef.scala:277) at scala.reflect.internal.Types$ModuleTypeRef.<init>(Types.scala:1879) at scala.reflect.internal.Types$PackageTypeRef.<init>(Types.scala:1897) at scala.reflect.internal.Types$TypeRef$.apply(Types.scala:2401) at scala.reflect.internal.Types.typeRef(Types.scala:3553) at scala.reflect.internal.Types.typeRef$(Types.scala:3536) at scala.reflect.internal.SymbolTable.typeRef(SymbolTable.scala:16) at scala.reflect.internal.Symbols$TypeSymbol.newTypeRef(Symbols.scala:3026) at scala.reflect.internal.Symbols$TypeSymbol.updateTypeCache(Symbols.scala:3079) at scala.reflect.internal.Symbols$TypeSymbol.maybeUpdateTypeCache(Symbols.scala:3065) at scala.reflect.internal.Symbols$TypeSymbol.tpe_$times(Symbols.scala:3043) at scala.reflect.internal.Symbols$Symbol.typeOfThis(Symbols.scala:2020) at scala.reflect.internal.Types$ThisType.underlying(Types.scala:1184) at scala.reflect.internal.Types$SimpleTypeProxy.boundSyms(Types.scala:150) at scala.reflect.internal.Types$SimpleTypeProxy.boundSyms$(Types.scala:150) at scala.reflect.internal.Types$SingletonType.boundSyms(Types.scala:1088) at scala.reflect.internal.tpe.TypeMaps$SubstMap.apply(TypeMaps.scala:726) at scala.reflect.internal.tpe.TypeMaps$SubstSymMap.apply(TypeMaps.scala:789) at scala.reflect.internal.tpe.TypeMaps$TypeMap.mapOver(TypeMaps.scala:102) at scala.reflect.internal.tpe.TypeMaps$SubstSymMap.apply(TypeMaps.scala:783) at scala.reflect.internal.tpe.TypeMaps$TypeMap.mapOver(TypeMaps.scala:102) at scala.reflect.internal.tpe.TypeMaps$SubstSymMap.apply(TypeMaps.scala:783) at scala.reflect.internal.Types$Type.substSym(Types.scala:727) at scala.reflect.internal.tpe.TypeMaps$TypeMap.mapOver(TypeMaps.scala:123) at scala.reflect.internal.transform.UnCurry$$anon$1.apply(UnCurry.scala:53) at scala.reflect.internal.transform.UnCurry.transformInfo(UnCurry.scala:154) ``` This commit address the direct failure above by setting coherent flags on the stub package class symbol (it also needs the MODULE flag).

Although this is cheap, when debugging log output of info transformer activity this was a major source of noise. This commit avoids the info lookup for methods other than `+`, and then for `+` uses the typer phase info to distinguish concatentation from addition.

dwijnand · 2017-02-22T23:53:54Z

super green

lrytz

()

Given that we correctly setup the flags on the stub symbol, we no longer trip an assertion in ModuleTypeRef's constructor.

only trivial merge conflict involving a method renaming * d1d700e - (origin/HEAD, origin/2.12.x) Merge pull request scala#5754 from Philippus/issue/html-tag-in-hover (2 days ago) <Lukas Rytz> |\ | * cf64718 - pattern for entitylink was too narrow, cleaned up the tests (3 days ago) <Philippus Baalman> * | f771395 - Merge pull request scala#5671 from retronym/topic/stubby-2 (3 days ago) <Lukas Rytz> |\ \ | * | ad13063 - Remove non-essential fix for stub symbol failure (4 days ago) <Jason Zaugg> | * | 7f2d1fa - Avoid forcing info transforms of primitive methods (2 weeks ago) <Jason Zaugg> | * | 37a0eb7 - Avoid stub symbol related crash in backend (2 weeks ago) <Jason Zaugg> * | | 96a7617 - Merge pull request scala#5622 from edmundnoble/extra-errs (4 days ago) <Adriaan Moors> |\ \ \ | * | | 466e52b - Match error lengths (4 weeks ago) <Edmund Noble> | * | | d1fc983 - Improved error messages for identically named, differently prefixed types (9 weeks ago) <Edmund Noble> | / / * | | f2e05c2 - Merge pull request scala#5728 from Philippus/issue/html-tag-in-hover (4 days ago) <Lukas Rytz> |\ \ \ | | |/ | |/| | * | e3c5df8 - added missing Inline matches to inlineToStr so it is now exhaustive scala.xml.XML.loadString(tag).text will remove all html tags inside the HtmlTag (9 days ago) <Philippus Baalman> * | | 920bc4e - Merge pull request scala#5743 from som-snytt/issue/10207-bad-update (7 days ago) <Lukas Rytz> |\ \ \ | * | | 094f7f9 - SI-10207 Error before update conversion (8 days ago) <Som Snytt> * | | | 1b4d36f - Merge pull request scala#5746 from paulp/pr/partest (7 days ago) <Lukas Rytz> |\ \ \ \ | |/ / / |/| | | | * | | eac00e1 - Add partest paths to the list of watched sources. (8 days ago) <Paul Phillips> |/ / / * | | 5f1a638 - Merge pull request scala#5732 from retronym/topic/build-info-malarkey (10 days ago) <Adriaan Moors> |\ \ \ | * | | 5e9acac - More predictable performance of SBT build startup, reload (11 days ago) <Jason Zaugg> | / / * | | 759a7b7 - Merge pull request scala#5735 from SethTisue/sd-313 (10 days ago) <Adriaan Moors> |\ \ \ | * | | ed4ddf5 - increase timeouts on some sys.process tests (11 days ago) <Seth Tisue> * | | | e2aaf2c - Merge pull request scala#5723 from dragos/issue/regression-assert-ide (11 days ago) <Lukas Rytz> |\ \ \ \ | |/ / / |/| | | | * | | e5c957e - Fix regression in 5751763 (12 days ago) <Iulian Dragos> * | | | f174bfb - Merge pull request scala#5731 from janekdb/issue/scalaGH-644/fix-spec-latex-rendering (11 days ago) <Seth Tisue> |\ \ \ \ | * | | | ba4c6d4 - scalaGH-644: Remove static html styling of spec code blocks (11 days ago) <Janek Bogucki> |/ / / / * | | | 8e40bef - Merge pull request scala#5729 from scala/revert-5658-topic/hashhash (12 days ago) <Adriaan Moors> |\ \ \ \ | |_|/ / |/| | | | * | | 86cd70f - (origin/revert-5658-topic/hashhash) Revert "Fix erasure of the qualifier of ##" (12 days ago) <Adriaan Moors> |/ / / * | | cbf7daa - Merge pull request scala#5681 from Philippus/issue/9704 (13 days ago) <Lukas Rytz> |\ \ \ | * | | b8a8ac1 - moved Pattern and TagsNotToClose to a HtmlTag companion object (13 days ago) <Philippus Baalman> | * | | a019082 - SI-9704 don't add a closed HtmlTag if it is already closed (4 weeks ago) <Philippus Baalman> | / / * | | effde0c - Merge pull request scala#5726 from scala/revert-5629-issue/10120-quote-err (13 days ago) <Adriaan Moors> |\ \ \ | * | | d60f6e3 - (origin/revert-5629-issue/10120-quote-err) Revert "SI-10133 Require escaped single quote char lit" (13 days ago) <Adriaan Moors> |/ / / * | | a8c4a54 - Merge pull request scala#5663 from gourlaysama/ticket/sd-256-enable-repl-colors-unix-2 (13 days ago) <Adriaan Moors> |\ \ \ | * | | 6411170 - SD-256 enable colored output by default on unix (13 days ago) <Antoine Gourlay> * | | | c96a977 - Merge pull request scala#5658 from retronym/topic/hashhash (13 days ago) <Lukas Rytz> |\ \ \ \ | |/ / / |/| | | | * | | f85c62e - Fix erasure of the qualifier of ## (6 weeks ago) <Jason Zaugg> | / / * | | 76bfb9e - Merge pull request scala#5708 from szeiger/issue/si10194 (13 days ago) <Lukas Rytz> |\ \ \ | * | | 1d22ee4 - SI-10194: Fix abstract type resolution for overloaded HOFs (13 days ago) <Stefan Zeiger> | / / * | | dabec1a - Merge pull request scala#5700 from retronym/ticket/10154-refactor (13 days ago) <Lukas Rytz> |\ \ \ | * | | 06eee79 - Refactor implementation of lookupCompanion (2 weeks ago) <Jason Zaugg> | / / * | | 2f1e0c2 - Merge pull request scala#5704 from som-snytt/issue/10190-elide-string (13 days ago) <Lukas Rytz> |\ \ \ | * | | 6fb3825 - SI-10190 Elide string to empty instead of null (3 weeks ago) <Som Snytt> | / / * | | 13f7b2a - Merge pull request scala#5640 from optimizely/repl-import-handler (2 weeks ago) <Adriaan Moors> |\ \ \ | * | | aa7e335 - Fix ImportHandler's reporting of importedNames and importedSymbols (8 weeks ago) <Hao Xia> | * | | c89d821 - Fix SIOOBE in Name#pos for substrings of length 1 (8 weeks ago) <Jason Zaugg> | / / * | | 023a96a - Merge pull request scala#5629 from som-snytt/issue/10120-quote-err (2 weeks ago) <Adriaan Moors> |\ \ \ | * | | 05cc3e2 - SI-10120 ReplReporter handles message indent (7 weeks ago) <Som Snytt> | * | | 939abf1 - SI-10120 Extra advice on unclosed char literal (8 weeks ago) <Som Snytt> | * | | 855492c - SI-10133 Require escaped single quote char lit (8 weeks ago) <Som Snytt> | / / * | | e21ab42 - Merge pull request scala#5660 from som-snytt/issue/9464-spec (2 weeks ago) <Adriaan Moors> |\ \ \ | * | | a6dcceb - SI-9464 Clarify spec on no final trait (6 weeks ago) <Som Snytt> | / / * | | e87a436 - Merge pull request scala#5659 from retronym/ticket/10026 (2 weeks ago) <Adriaan Moors> |\ \ \ | |/ / |/| | | * | 777a0e5 - SI-10026 Fix endless cycle in runtime reflection (2 weeks ago) <Jason Zaugg> | |/ * | 23e5ed9 - Merge pull request scala#5707 from retronym/topic/java9-prepare (2 weeks ago) <Lukas Rytz> |\ \ | * | 96e8e97 - Workaround bug in Scala runtime reflection on JDK 9 (3 weeks ago) <Jason Zaugg> | * | fe2d9a4 - Avoid ambiguous overload on JDK 9 (3 weeks ago) <Jason Zaugg> | * | 6bba8f7 - Adapt to change in ClassLoader in JDK 9 (3 weeks ago) <Jason Zaugg> | * | 8136057 - Bump scala-asm version (3 weeks ago) <Jason Zaugg> | / * | cad3c3d - Merge pull request scala#5709 from adriaanm/sam_wild_bound (2 weeks ago) <Lukas Rytz> |\ \ | * | c396e96 - Ignore BoundedWildcardType in erasure type map (2 weeks ago) <Adriaan Moors> * | | 3e9df41 - Merge pull request scala#5711 from retronym/ticket/jrt (2 weeks ago) <Lukas Rytz> |\ \ \ | * | | 09c7edc - Faster and simpler Java 9 classpath implementation (2 weeks ago) <Jason Zaugg> | / / * | | 7b9d3cc - Merge pull request scala#5713 from janekdb/issue/scalaGH-644/sync-jekyll-README-to-Gemfile (2 weeks ago) <Lukas Rytz> |\ \ \ | * | | 5e5ec9a - scalaGH-644: Expand note regarding Jekyll versions (2 weeks ago) <Janek Bogucki> | / / * | | 144f7e0 - Merge pull request scala#5714 from dragos/issue/usage-sterr-SI-10178 (2 weeks ago) <Lukas Rytz> |\ \ \ | |/ / |/| | | * | 640c85e - SI-10178 Route reporter.echo to stdout (2 weeks ago) <Iulian Dragos> | / * | 2fec08b - Merge pull request scala#5717 from som-snytt/issue/10148-followup (2 weeks ago) <Adriaan Moors> |\ \ | |/ |/| | * f3d271b - SI-10148 Accept verbose zero (2 weeks ago) <Som Snytt> * 147e5dd - Merge pull request scala#5716 from adriaanm/i296 (2 weeks ago) <Jason Zaugg> * 12437a0 - Ensure ordering for args to `choose` in DurationTest (2 weeks ago) <Adriaan Moors>

scala-jenkins added this to the 2.12.2 milestone Feb 2, 2017

retronym self-assigned this Feb 2, 2017

retronym added the WIP label Feb 2, 2017

retronym force-pushed the topic/stubby-2 branch 2 times, most recently from 3537167 to d070c3f Compare February 4, 2017 18:08

retronym force-pushed the topic/stubby-2 branch 2 times, most recently from 17122a0 to db7a1db Compare February 4, 2017 20:40

retronym force-pushed the topic/stubby-2 branch from db7a1db to 3173007 Compare February 5, 2017 06:30

retronym commented Feb 5, 2017

View reviewed changes

retronym force-pushed the topic/stubby-2 branch from 3173007 to 4743632 Compare February 5, 2017 23:53

lrytz reviewed Feb 8, 2017

View reviewed changes

adriaanm modified the milestones: 2.12.2, WIP Feb 16, 2017

retronym force-pushed the topic/stubby-2 branch from 4743632 to 3a3b544 Compare February 17, 2017 06:28

retronym changed the title ~~WIP: more stub sensitivity~~ Avoid compiler crash with missing transitive dependencies Feb 17, 2017

retronym removed the WIP label Feb 17, 2017

retronym force-pushed the topic/stubby-2 branch from 3a3b544 to be793c5 Compare February 17, 2017 06:40

lrytz approved these changes Feb 17, 2017

View reviewed changes

retronym force-pushed the topic/stubby-2 branch 2 times, most recently from 71edb79 to 415fdc7 Compare February 19, 2017 05:22

retronym force-pushed the topic/stubby-2 branch 4 times, most recently from 99801a3 to 7f2d1fa Compare February 20, 2017 05:39

retronym modified the milestones: 2.12.2, WIP Feb 23, 2017

lrytz requested changes Feb 24, 2017

View reviewed changes

Remove non-essential fix for stub symbol failure

ad13063

Given that we correctly setup the flags on the stub symbol, we no longer trip an assertion in ModuleTypeRef's constructor.

lrytz approved these changes Mar 3, 2017

View reviewed changes

lrytz merged commit f771395 into scala:2.12.x Mar 3, 2017

scabug mentioned this pull request Apr 7, 2017

Regression issue: compiler crashes with "java.lang.IllegalArgumentException: requirement failed: package parser" scala/bug#10171

Closed

Avoid compiler crash with missing transitive dependencies #5671

Avoid compiler crash with missing transitive dependencies #5671

Uh oh!

Conversation

retronym commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

retronym commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

retronym commented Feb 4, 2017

Uh oh!

retronym commented Feb 4, 2017

Uh oh!

retronym commented Feb 4, 2017

Uh oh!

retronym commented Feb 5, 2017

Uh oh!

retronym commented Feb 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

retronym commented Feb 17, 2017

Uh oh!

dwijnand commented Feb 22, 2017

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

retronym commented Feb 2, 2017 •

edited

Loading

retronym commented Feb 3, 2017 •

edited

Loading