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

-explaintypes got too verbose #6123

Closed
scabug opened this issue Jul 22, 2012 · 18 comments
Closed

-explaintypes got too verbose #6123

scabug opened this issue Jul 22, 2012 · 18 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Jul 22, 2012

With 2.10.0-M5 -explaintypes got much more verbose, compared to (for instance) Scala 2.9.x -- it produces extra output even when compilation does not fail.
While this might be done to help debugging, I don't think this is justified even for an intermediate release - debugging stuff should have separate options.

This problem was already briefly discussed on scala-language; Daniel Sobral suggested a bug report.
https://groups.google.com/forum/?fromgroups#!topic/scala-language/XG5SsELdwAg

@scabug
Copy link
Author

scabug commented Jul 22, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6123?orig=1
Reporter: @Blaisorblade
Affected Versions: 2.10.0-M5, 2.10.0-M6
Attachments:

@scabug
Copy link
Author

scabug commented Jul 22, 2012

@retronym said:
Can you provide a small sample program, and a diff of the output?

@scabug
Copy link
Author

scabug commented Jul 22, 2012

@Blaisorblade said (edited on Jul 22, 2012 11:05:30 PM UTC):
I found one available - it can be probably reduced further, but it's already just 100 lines. Please don't be confused by the complexity of code, I don't think it's necessary - I selected it from my codebase since it's self-contained and because for most warnings there's just no way to see where they come from, as you'll notice.

Normal output (Scala 2.9.1/.2): empty, as it's supposed to be. In particular, -explaintypes should never produce additional output on correct programs.

$ scalac -explaintypes src/main/scala/ivm/expressiontree/ImplicitBugReport.scala
$

With the new release:

$ ~/opt/scala-2.10.0-M5/bin/scalac -explaintypes src/main/scala/ivm/expressiontree/ImplicitBugReport.scala
Nothing <: ivm.expressiontree.ImplicitBugReport.Exp[Int]?
true
ivm.expressiontree.ImplicitBugReport.Exp[Int] <: ivm.expressiontree.ImplicitBugReport.Exp[T]?
false
Nothing <: ivm.expressiontree.ImplicitBugReport.Exp[Int]?
true
ivm.expressiontree.ImplicitBugReport.Exp[Int] <: ivm.expressiontree.ImplicitBugReport.Exp[T]?
  ivm.expressiontree.ImplicitBugReport.type = ivm.expressiontree.ImplicitBugReport.type?
  true
false
Unit <: ivm.expressiontree.ImplicitBugReport.CanBuildExp[Int,?]?
false
Nothing <: ivm.expressiontree.ImplicitBugReport.Exp[Int]?
true
ivm.expressiontree.ImplicitBugReport.Exp[Int] <: ivm.expressiontree.ImplicitBugReport.Exp[T]?
false
Nothing <: ivm.expressiontree.ImplicitBugReport.Exp[Int]?
true
ivm.expressiontree.ImplicitBugReport.Exp[Int] <: ivm.expressiontree.ImplicitBugReport.Exp[T]?
  ivm.expressiontree.ImplicitBugReport.type = ivm.expressiontree.ImplicitBugReport.type?
  true
false
Unit <: ivm.expressiontree.ImplicitBugReport.CanBuildExp[Int,?]?
false

I believe that you can take any sufficiently wide codebase of yours, activate -explaintypes and see tons of such warnings arise out of nothing. I had the same problem porting https://github.com/Blaisorblade/BAT to 2.10, even though the code base does not use advanced Scala features as much as mine (as far as I know).

@scabug
Copy link
Author

scabug commented Aug 5, 2012

@retronym said:
Regression between 2.10.0-M1 and -M2.

@scabug
Copy link
Author

scabug commented Aug 25, 2012

@Blaisorblade said:
For what it's worth, things got even worse between -M6 and -M7 -- output started appearing for
https://github.com/ps-mr/LinqOnSteroids/blob/a2546ff205d56c82baeedfdf02a12043a198386f/scratch/macros/Macros.scala. Sample output (maybe from a slightly different version):

scala.reflect.macros.Context <: scala.reflect.macros.Context?
true
c.universe.Expr[Any] <: c.universe.Expr[Any]?
true
c.universe.Expr[String] <: c.universe.Expr[String]?
true
scala.reflect.macros.Context <: scala.reflect.macros.Context?
true
c.universe.Expr[Any] <: c.universe.Expr[Any]?
true
c.universe.Expr[Unit] <: c.universe.Expr[Unit]?
true
scala.reflect.macros.Context <: scala.reflect.macros.Context?
true
c.universe.Expr[String] <: c.universe.Expr[String]?
true
c.universe.Expr[Any]* <: c.universe.Expr[Any]*?
true
c.universe.Expr[Unit] <: c.universe.Expr[Unit]?
true
scala.reflect.macros.Context <: scala.reflect.macros.Context?
true
c.universe.Expr[Any] <: c.universe.Expr[Any]?
true
c.universe.Expr[Any] <: c.universe.Expr[Any]?
true
scala.reflect.macros.Context <: scala.reflect.macros.Context?
true
c.universe.Expr[Any] <: c.universe.Expr[Any]?
true
c.universe.Expr[Any] <: c.universe.Expr[Any]?
true
scala.reflect.macros.Context <: scala.reflect.macros.Context?
true
c.universe.Expr[Any] <: c.universe.Expr[Any]?
true
c.universe.Expr[Any] <: c.universe.Expr[Any]?
true

I could try minimizing it if really needed.

@scabug
Copy link
Author

scabug commented Aug 25, 2012

@Blaisorblade said:
I tried adding "scala 2.10.0-M7" among versions, but it's not yet there.

@scabug
Copy link
Author

scabug commented Jan 7, 2013

@Blaisorblade said:
Is this not planned to be fixed for the whole 2.10 cycle? That's a bit sad.

@scabug
Copy link
Author

scabug commented Jan 7, 2013

@adriaanm said:
our priority target for major development is 2.11 -- happy to backport it to 2.10.x if possible
better yet, happy to merge a PR fixing and backporting it :-)

@scabug
Copy link
Author

scabug commented Jan 20, 2013

@Blaisorblade said (edited on Jan 20, 2013 4:38:48 PM UTC):
When Scala-IDE works and I'm patient enough for it (so, a lot of patience), a problem like this becomes easy to fix.

The problems are calls to Types.explainTypes when context.reportErrors is false, for instance because they happen during implicit resolution.

Types.explainTypes should thus check reportErrors but has no access to context, and not all calls go through Infer.explainTypes (although they probably should?), so I have to check context.reportErrors in multiple locations. Which means that the only reliable way to fix this problem is to add -explaintypes to scalac flags for all pos tests, and while at it ensure there are neg tests using -explaintypes and checking that it does produce output.

Pull request for the first part coming up soon.

The first problem is in c800d1fec5241ed8c29e5af30465856f9b583246, in particular in

+      def NotWithinBounds(tree: Tree, prefix: String, targs: List[Type],
+                          tparams: List[Symbol], kindErrors: List[String]) = {
+        if (settings.explaintypes.value) {
+          val bounds = tparams map (tp => tp.info.instantiateTypeParams(tparams, targs).bounds)
+          (targs, bounds).zipped foreach ((targ, bound) => explainTypes(bound.lo, targ))
+          (targs, bounds).zipped foreach ((targ, bound) => explainTypes(targ, bound.hi))
+          ()
+        }
+
+        issueNormalTypeError(tree,
+                prefix + "type arguments " + targs.mkString("[", ",", "]") +
+                " do not conform to " + tparams.head.owner + "'s type parameter bounds " +
+                (tparams map (_.defString)).mkString("[", ",", "]"))
+      }

@scabug
Copy link
Author

scabug commented Jan 20, 2013

@Blaisorblade said (edited on Jan 20, 2013 11:30:53 PM UTC):
Well, there's yet more than I thought. The other culprit is 78f9ef3906c78413ff8835fdad3849bfe5516be2, aka v2.10.0-M6-253-g78f9ef3 (I did say that things got worse in -M7). The code in this commit just logs subtype checks done during some parts of macro expansion, whether there is an error or not.
Maybe it's an instance of the broken window theory?

@scabug
Copy link
Author

scabug commented Jan 21, 2013

@Blaisorblade said:
I did not send a pull request yet, since I'm waiting for full test results, and it is not clear what exactly the best fix is. The current code is available here:
https://github.com/Blaisorblade/scala/tree/issue/6123
and fixes the bugs discussed above.

@scabug
Copy link
Author

scabug commented Jan 21, 2013

@hubertp said:
I commented a bit on the commits. One problem I can see is that if your type error gets buffered and all fallbacks fail then we report that buffered error. However at this point no explain types will be called.
Also before submitting PR please rebase your branch and squash to one commit (and it should be possible to add some tests for this particular case).

I am happy to review your stuff so just include me in the PR.

@scabug
Copy link
Author

scabug commented Jan 21, 2013

@Blaisorblade said:
Thanks for your comments.

One problem I can see is that if your type error gets buffered and all fallbacks fail then we report that buffered error. However at this point no explain types will be called.
I see your point; a better fix would be to buffer explainTypes messages, but that seems to call for more extensive refactorings. And as a user, I'd prefer for explainTypes to be usable sometimes (when it does explain me a type error) than not at all (because it explains too much).

On the most important tests, see
https://groups.google.com/d/topic/scala-internals/5D2EJViOMJo/discussion

But I'll also update some neg test.

@scabug
Copy link
Author

scabug commented Jan 21, 2013

@Blaisorblade said (edited on Jan 21, 2013 11:49:13 PM UTC):
Testing that -explaintypes works at all requires that partest inspects the standard output of the programs it runs. Hence I'm marking this as blocked by #7003.

@scabug
Copy link
Author

scabug commented Mar 15, 2013

@adriaanm said:
Un-assigning to foster work stealing, as announced in https://groups.google.com/forum/?fromgroups=#!topic/scala-internals/o8WG4plpNkw

@scabug
Copy link
Author

scabug commented Mar 15, 2013

@Blaisorblade said:
scala/scala#2259 is my current fix - the only problem is that it's hard to test correctly, because of limitations/bugs in partest (#7003 among others).

@scabug
Copy link
Author

scabug commented Mar 23, 2013

@Blaisorblade said:
The pull request was merged, but this should still be backported to 2.10.x. So I'm not closing this just yet.

@scabug
Copy link
Author

scabug commented May 21, 2013

@retronym said:
Please open a followup ticket if you want this backported. I'm closing to show this as fixed in 2.11.0-M3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants