Skip to content

Fix compiler plugin instance duplication #461

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

Merged
merged 5 commits into from
Sep 7, 2016

Conversation

tkroman
Copy link
Contributor

@tkroman tkroman commented Sep 5, 2016

Which was caused by the presence of both normal and source jars on the classpath and led to behaviour like this:

@ interp.load.plugin.ivy("org.scalamacros" % "paradise_2.11.7" % "2.1.0")

@ var x = 1
cmd2.sc:2: both org.scalamacros.paradise.typechecker.AnalyzerPlugins$MacroPlugin$@4e3931 and org.scalamacros.paradise.typechecker.AnalyzerPlugins$MacroPlugin$@409e0e69 want to enter a symbol for this tree
package $sess
        ^
scala.reflect.internal.Types$TypeError: both org.scalamacros.paradise.typechecker.AnalyzerPlugins$MacroPlugin$@4e3931 and org.scalamacros.paradise.typechecker.AnalyzerPlugins$MacroPlugin$@409e0e69 want to enter a symbol for this tree
  scala.tools.nsc.typechecker.Contexts$ThrowingReporter.handleError(Contexts.scala:1402)
  scala.tools.nsc.typechecker.Contexts$ContextReporter.info0(Contexts.scala:1297)
  scala.tools.nsc.typechecker.Contexts$ContextReporter.info0(Contexts.scala:1250)
  scala.reflect.internal.Reporter.error(Reporting.scala:82)
  scala.tools.nsc.typechecker.Contexts$Context.error(Contexts.scala:577)
  scala.tools.nsc.typechecker.AnalyzerPlugins$class.invoke(AnalyzerPlugins.scala:373)
  scala.tools.nsc.typechecker.AnalyzerPlugins$class.pluginsEnterSym(AnalyzerPlugins.scala:423)
  ammonite.runtime.CompilerCompatibility$$anon$2.pluginsEnterSym(CompilerCompatibility.scala:10)
  scala.tools.nsc.typechecker.Namers$Namer.enterSym(Namers.scala:275)
  scala.tools.nsc.typechecker.Analyzer$namerFactory$$anon$1.apply(Analyzer.scala:43)
  scala.tools.nsc.Global$GlobalPhase$$anonfun$applyPhase$1.apply$mcV$sp(Global.scala:440)
  scala.tools.nsc.Global$GlobalPhase.withCurrentUnit(Global.scala:431)
  scala.tools.nsc.Global$GlobalPhase.applyPhase(Global.scala:440)
  scala.tools.nsc.Global$GlobalPhase$$anonfun$run$1.apply(Global.scala:398)
  scala.tools.nsc.Global$GlobalPhase$$anonfun$run$1.apply(Global.scala:398)
  scala.collection.Iterator$class.foreach(Iterator.scala:893)
  scala.collection.AbstractIterator.foreach(Iterator.scala:1336)
  scala.tools.nsc.Global$GlobalPhase.run(Global.scala:398)
  scala.tools.nsc.Global$Run.compileUnitsInternal(Global.scala:1501)
  scala.tools.nsc.Global$Run.compileUnits(Global.scala:1486)
  scala.tools.nsc.Global$Run.compileSources(Global.scala:1481)
  scala.tools.nsc.Global$Run.compileFiles(Global.scala:1571)
  ammonite.runtime.Compiler$$anon$4.compile(Compiler.scala:284)
  ammonite.runtime.Interpreter.compileClass(Interpreter.scala:258)
  ammonite.runtime.Interpreter$$anonfun$evaluateLine$2.apply(Interpreter.scala:278)
  ammonite.runtime.Interpreter$$anonfun$evaluateLine$2.apply(Interpreter.scala:277)
  ammonite.util.Catching.flatMap(Res.scala:108)
  ammonite.runtime.Interpreter.evaluateLine(Interpreter.scala:277)
  ammonite.runtime.Interpreter$$anonfun$processLine$2$$anonfun$apply$10$$anonfun$apply$12.apply(Interpreter.scala:233)
  ammonite.runtime.Interpreter$$anonfun$processLine$2$$anonfun$apply$10$$anonfun$apply$12.apply(Interpreter.scala:223)
  ammonite.util.Res$Success.flatMap(Res.scala:57)
  ammonite.runtime.Interpreter$$anonfun$processLine$2$$anonfun$apply$10.apply(Interpreter.scala:223)
  ammonite.runtime.Interpreter$$anonfun$processLine$2$$anonfun$apply$10.apply(Interpreter.scala:213)
  ammonite.util.Res$Success.flatMap(Res.scala:57)
  ammonite.runtime.Interpreter$$anonfun$processLine$2.apply(Interpreter.scala:213)
  ammonite.runtime.Interpreter$$anonfun$processLine$2.apply(Interpreter.scala:209)
  ammonite.util.Catching.flatMap(Res.scala:108)
  ammonite.runtime.Interpreter.processLine(Interpreter.scala:209)
  ammonite.repl.Repl$$anonfun$action$4$$anonfun$apply$2.apply(Repl.scala:88)
  ammonite.repl.Repl$$anonfun$action$4$$anonfun$apply$2.apply(Repl.scala:87)
  ammonite.repl.Scoped$$anonfun$flatMap$1.apply(Signaller.scala:44)
  ammonite.repl.Signaller.apply(Signaller.scala:29)
  ammonite.repl.Scoped$class.flatMap(Signaller.scala:44)
  ammonite.repl.Signaller.flatMap(Signaller.scala:11)
  ammonite.repl.Repl$$anonfun$action$4.apply(Repl.scala:87)
  ammonite.repl.Repl$$anonfun$action$4.apply(Repl.scala:74)
  ammonite.util.Res$Success.flatMap(Res.scala:57)
  ammonite.repl.Repl.action(Repl.scala:74)
  ammonite.repl.Repl.loop$1(Repl.scala:98)
  ammonite.repl.Repl.run(Repl.scala:117)
  ammonite.Main.run(Main.scala:108)
  ammonite.Main$$anonfun$main$1$$anonfun$apply$1.apply(Main.scala:249)
  ammonite.Main$.ammonite$Main$$ifContinually$1(Main.scala:228)
  ammonite.Main$$anonfun$main$1.apply(Main.scala:230)
  ammonite.Main$$anonfun$main$1.apply(Main.scala:230)
  scala.Option.foreach(Option.scala:257)
  ammonite.Main$.main(Main.scala:230)
  ammonite.Main.main(Main.scala:-1)
Something unexpected went wrong =(

@tkroman
Copy link
Contributor Author

tkroman commented Sep 5, 2016

I deleted the test since it was obviously bogus (since there is no way to specify classifiers like withSources() (or at least I don't know any)).
I don't know if that means anything, but a) I'm sure that this fixes my issue; b) it doesn't break anything and c) in the end it's up to you to decide :)

@lihaoyi
Copy link
Member

lihaoyi commented Sep 6, 2016

I don't understand; if it fixes your issue, can't you put the console trace of your issue-repro in the test?

@tkroman
Copy link
Contributor Author

tkroman commented Sep 6, 2016

@lihaoyi oh, I didn't realise that loading the foo dependency not only causes the foo.jar, but also the foo-sources.jar to be loaded. But I checked with a clean cache and yes it does. This is weird. I returned the test.

@tkroman
Copy link
Contributor Author

tkroman commented Sep 6, 2016

Whaaaat. The JVM crashed. I'm not sure that's my fault. Can you trigger a rebuild?

@Ammonite-Bot
Copy link
Collaborator

ah sure lemme do that when i get back to my computer

On Wednesday, 7 September 2016, Roman Tkalenko notifications@github.com
wrote:

Whaaaat. The JVM crashed. I'm not sure that's my fault. Can you trigger a
rebuild?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#461 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ATpwjIcbFjrmmYnicPGd1OAljoydruOTks5qnfuagaJpZM4J1NoR
.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 7, 2016

@tkroman The tests pass and I can repro locally. Could you explain in a bit more detail why the status quo has this problem, and why this fixes it? It's not clear to me reading the code

@tkroman
Copy link
Contributor Author

tkroman commented Sep 7, 2016

When you load the macro-paradise plugin, 2 jars containing the scalac-plugin.xml are downloaded, e.g. paradise_2.11.8-2.1.0.jar and paradise_2.11.8-2.1.0-sources.jar.
Both jars are considered to be on the pluginClassloader's classpath, hence they both will be detected as plugin-containing ones. Since there are no equality checks (currently), the pair ("macro-paradise", "class of macro-paradise plugin") is stored in the plugins list 2 times.
Plugins are stored in a non-unique collection, so it's possible to add 2 instances of the same plugin.
During the phase of plugins analysis (scala.tools.nsc.typechecker.AnalyzerPlugins#invoke) there is a check that (as far as I understood) no two plugins can simultaneously enter the symbol at the same position at the same time (scala.tools.nsc.typechecker.AnalyzerPlugins#pluginsEnterSym). Since the 2 instances we have are actually the same plugin, it's obvious we'll get this error here.

To be completely honest, I'm looking at all this stuff from a beginner's point of view. I just did what seemed logical after reading the code where the exception happened. The only guess I had to do was about having more than one jar on the classpath, and that's easily checked with the debugger.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 7, 2016

Ok sounds reasonable, thanks!

@lihaoyi lihaoyi merged commit 849e5e7 into com-lihaoyi:master Sep 7, 2016
@tkroman tkroman deleted the bugfix/plugin-dup branch September 7, 2016 06:08
@lihaoyi
Copy link
Member

lihaoyi commented Sep 7, 2016

@tkroman This is available in the latest unstable version COMMIT-849e5e7 http://www.lihaoyi.com/Ammonite/#UnstableVersions

@tkroman
Copy link
Contributor Author

tkroman commented Sep 7, 2016

@lihaoyi awesome, thanks!

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.

3 participants