spurious cyclic error in type selection #5762

adriaanm · 2017-03-08T00:38:17Z

Based on @allisonhb's excellent analysis in milessabin/shapeless#679 (original fix in #5656), here's my proposed fix.

The bug report analysis correctly identified an unexpected cyclic error, which was triggered by implicit search. Implicit search is not expected while type checking the qualifier of a type selection node. Fixing this also removes the cyclic error.

Thus, align typedSelect to be more consistent in dealing with Select and SelectFromTypeTree nodes. The spec is clear on the equivalence between p.T (Select(p, T)) and p.type#T (SelectFromTypeTree(SingletonTypeTree(p), T)).

This bug gets even more intriguing, in that it shows that you can sneak a macro call into a path using selectDynamic. (See the test in next commit.)

Eventually, we should disable applyDynamic in paths. It wasn't explicitly disallowed, since we assumed the stability check would rule out method calls. However, a macro application will dissolve into its rhs before stability is checked...

Fixes scala/bug#10159

ghost · 2017-03-08T02:45:39Z

Cool! I think this is way more sensible than my fix; the equivalence with SelectFromTypeTree(SingletonType(...), ...) makes a lot of sense. ApplyDynamic ever producing a stable path is very suspicious but seems to have worked for a lot of "tricks" in shapeless and elsewhere... potentially you will be making @milessabin sad.

Looks a lot nicer though; thanks! I'll close mine out for now.

som-snytt · 2017-03-08T09:35:56Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+        if ((qualTp eq null) || qualTp.isError) setError(tree)
+        else if (name.isTypeName && qualTp.isVolatile)  // TODO: use same error message for volatileType#T and volatilePath.T?
+          if (tree.isInstanceOf[SelectFromTypeTree]) TypeSelectionFromVolatileTypeError(tree, qual)
+          else UnstableTreeError(qual)


Nested braceless style made me instinctively reach out for something to hold onto.

I really hated my braces as a teenager. I guess it still shows.

milessabin · 2017-03-08T09:49:35Z

To clarify, is this fix going to break shapeless's current use of this mechanism?

ghost · 2017-03-08T13:53:09Z

As I understand it, no... @adriaanm mentioned considering that we could stop considering SelectDynamic to ever be part of a stable path, even during macro expansion. Obviously that wouldn't come around until 2.13.x... plenty of time to think of a workaround :P

adriaanm · 2017-03-08T20:34:00Z

Indeed. At the earliest we would consider this change for 2.13. I can see its utility, though -- not saying we definitely will plug the hole. (From a soundness/IDE perspective, I think we should, but tbd.)

adriaanm · 2017-06-28T16:58:58Z

Rebased.

adriaanm · 2017-06-29T01:28:10Z

Re-rebased.

adriaanm · 2017-06-29T07:08:38Z

no idea why cla check is failing, others are green

retronym · 2017-06-29T10:27:55Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+        // note: on error, we discard the work we did in type checking tree.qualifier into qual
+        // (tree is either Select or SelectFromTypeTree, and qual may be different from tree.qualifier because it has been type checked)
+        val qualTp = qual.tpe
+        if ((qualTp eq null) || qualTp.isError) setError(tree)


Checking qual.tpe.isError for selections from term trees appears to be new. I haven't figured out how that case was handled before. The new formulation (checking early and bailing out) appears sane, but I need to connect a few more dots to feel comfortable.

I haven't found any example where the compiler before/after this change differs in behaviour, so I'm probably just being paranoid. I focussed on error cases.

retronym · 2017-06-29T10:37:40Z

/synch

retronym · 2017-06-29T11:27:56Z

Comparative benchmark results coming soon here

retronym · 2017-07-07T11:20:56Z

Reassigning to 2.12.4. While I think the change looks good, the bug is pretty esoteric, and this touches core part of the typechecker, so I'd rather we merge it after the impending 2.12.3 release.

adriaanm · 2017-09-18T21:49:58Z

Rebased. Ping @retronym. Let's decide this week whether we feel this is ok for 2.12.4

retronym · 2017-09-18T23:38:00Z

merge error in the rebase, it would seem:

[error] /home/jenkins/workspace/scala-2.12.x-validate-publish-core/src/compiler/scala/tools/nsc/typechecker/Typers.scala:4884: ';' expected but 'if' found.
[error]         val sym = tree.symbol orElse member(qual, name) orElse inCompanionForJavaStatic(qual.tpe.prefix, qual.symbol, name) if ((sym eq NoSymbol) && name != nme.CONSTRUCTOR && mode.inAny(EXPRmode | PATTERNmode)) {
[error]                                                                                                                             ^
[warn] one warning found
[error] one error found

adriaanm · 2017-09-19T00:35:51Z

ugh, i've gotten too lazy with IJ's magic "resolve all"

The bug report analysis correctly identified an unexpected cyclic error, which was triggered by implicit search. Implicit search is not expected while type checking the qualifier of a type selection node. Fixing this also removes the cyclic error. Thus, align `typedSelect` to be more consistent in dealing with `Select` and `SelectFromTypeTree` nodes. The spec is clear on the equivalence between `p.T` (`Select(p, T)`) and `p.type#T` (`SelectFromTypeTree(SingletonTypeTree(p), T)`). This bug gets even more intriguing, in that it shows that you can sneak a macro call into a path using `selectDynamic`. (See the test in next commit.) Eventually, we should disable applyDynamic in paths. It wasn't explicitly disallowed, since we assumed the stability check would rule out method calls. However, a macro application will dissolve into its rhs before stability is checked...

NOTE: the fix in the parent was inspired by allisonhb's analysis. This commit was amended to reuse the test case. Original commit message: Previously, while the implicit would rightly be discarded, `typedInternal` would catch the CyclicReference error and call `reportTypeError` before rethrowing, which would cause compilation to fail.

retronym · 2017-09-19T03:10:11Z

Benchmark scheduled.

milessabin · 2017-09-19T07:17:29Z

Let me know if there's anything I can do to help with this ...

adriaanm · 2017-09-19T17:13:51Z

As far as I can tell, benchmarks look unaffected -- right, @retronym?

No conflict, include PRs scala#5762, scala#6034

scala-jenkins added this to the 2.12.2 milestone Mar 8, 2017

adriaanm requested a review from retronym March 8, 2017 00:38

adriaanm force-pushed the t10159 branch from 936f8dc to fcf1fbd Compare March 8, 2017 00:58

ghost mentioned this pull request Mar 8, 2017

SI-10159: Don't report cyclic reference errors found in isCyclicOrErroneous. #5656

Closed

som-snytt reviewed Mar 8, 2017

View reviewed changes

SethTisue mentioned this pull request Apr 4, 2017

Release 2.12.2 scala/scala-dev#331

Closed

adriaanm assigned retronym Apr 4, 2017

adriaanm modified the milestones: 2.12.3, 2.12.2 Apr 4, 2017

scabug mentioned this pull request Apr 7, 2017

Spurious CyclicReference errors occur with shapeless Record type scala/bug#10159

Closed

adriaanm force-pushed the t10159 branch from fcf1fbd to 63d2097 Compare June 28, 2017 16:58

adriaanm force-pushed the t10159 branch from 63d2097 to f7020af Compare June 29, 2017 01:27

retronym reviewed Jun 29, 2017

View reviewed changes

retronym modified the milestones: 2.12.4, 2.12.3 Jul 7, 2017

retronym mentioned this pull request Sep 4, 2017

Fix scala/bug#10490: Allow Java nested class selection #6053

Closed

adriaanm force-pushed the t10159 branch from f7020af to 8953e57 Compare September 18, 2017 21:49

adriaanm and others added 2 commits September 18, 2017 17:52

adriaanm force-pushed the t10159 branch from 8953e57 to aea61e3 Compare September 19, 2017 00:52

retronym approved these changes Sep 19, 2017

View reviewed changes

retronym merged commit 0d81083 into scala:2.12.x Sep 19, 2017

retronym changed the title ~~SI-10159 spurious cyclic error in type selection~~ spurious cyclic error in type selection Sep 19, 2017

adriaanm added a commit to adriaanm/scala that referenced this pull request Sep 27, 2017

Merge 2.12.x into 2.13.x

bb0f3b9

No conflict, include PRs scala#5762, scala#6034

joroKr21 mentioned this pull request Mar 21, 2020

Weird illegal cyclic reference while building on 2.12 milessabin/shapeless#679

Closed

spurious cyclic error in type selection #5762

spurious cyclic error in type selection #5762

Uh oh!

Conversation

adriaanm commented Mar 8, 2017 • edited by retronym Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 8, 2017

Uh oh!

som-snytt Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

adriaanm Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

milessabin commented Mar 8, 2017

Uh oh!

ghost commented Mar 8, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented Mar 8, 2017

Uh oh!

adriaanm commented Jun 28, 2017

Uh oh!

adriaanm commented Jun 29, 2017

Uh oh!

adriaanm commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

retronym Jun 29, 2017

Choose a reason for hiding this comment

Uh oh!

retronym commented Jun 29, 2017

Uh oh!

retronym commented Jun 29, 2017

Uh oh!

retronym commented Jul 7, 2017

Uh oh!

adriaanm commented Sep 18, 2017

Uh oh!

retronym commented Sep 18, 2017

Uh oh!

adriaanm commented Sep 19, 2017

Uh oh!

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

Uh oh!

Uh oh!

milessabin commented Sep 19, 2017

Uh oh!

adriaanm commented Sep 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adriaanm commented Mar 8, 2017 •

edited by retronym

Loading

ghost commented Mar 8, 2017 •

edited by ghost

Loading

adriaanm commented Jun 29, 2017 •

edited

Loading

retronym commented Sep 19, 2017 •

edited

Loading