Fix #442: Name hash of value class should include underlying type#444
Merged
Fix #442: Name hash of value class should include underlying type#444
Conversation
b99066c to
d8ed7db
Compare
Contributor
Author
|
The first time I ran this PR I got the following error: [error] org.scalafmt.Error$MisformattedFile: /drone/src/github.com/sbt/zinc/internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala is mis-formatted.
[error] at org.scalafmt.cli.InputMethod$FileContents.write(InputMethod.scala:51)
[error] at org.scalafmt.cli.Cli$.handleFile(Cli.scala:118)
[error] at org.scalafmt.cli.Cli$.$anonfun$run$1(Cli.scala:175)
[error] at org.scalafmt.cli.Cli$.$anonfun$run$1$adapted(Cli.scala:173)
[error] at scala.collection.Iterator.foreach(Iterator.scala:929)
[error] at scala.collection.Iterator.foreach$(Iterator.scala:929)
[error] at scala.collection.AbstractIterator.foreach(Iterator.scala:1417)
[error] at scala.collection.parallel.ParIterableLike$Foreach.leaf(ParIterableLike.scala:970)
[error] at scala.collection.parallel.Task.$anonfun$tryLeaf$1(Tasks.scala:49)
[error] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:12)
[error] at scala.util.control.Breaks$$anon$1.catchBreak(Breaks.scala:63)
[error] at scala.collection.parallel.Task.tryLeaf(Tasks.scala:52)
[error] at scala.collection.parallel.Task.tryLeaf$(Tasks.scala:46)
[error] at scala.collection.parallel.ParIterableLike$Foreach.tryLeaf(ParIterableLike.scala:967)
[error] at scala.collection.parallel.AdaptiveWorkStealingTasks$WrappedTask.internal(Tasks.scala:156)
[error] at scala.collection.parallel.AdaptiveWorkStealingTasks$WrappedTask.internal$(Tasks.scala:153)
[error] at scala.collection.parallel.AdaptiveWorkStealingForkJoinTasks$WrappedTask.internal(Tasks.scala:440)
[error] at scala.collection.parallel.AdaptiveWorkStealingTasks$WrappedTask.compute(Tasks.scala:146)
[error] at scala.collection.parallel.AdaptiveWorkStealingTasks$WrappedTask.compute$(Tasks.scala:145)
[error] at scala.collection.parallel.AdaptiveWorkStealingForkJoinTasks$WrappedTask.compute(Tasks.scala:440)
[error] at java.util.concurrent.RecursiveAction.exec(RecursiveAction.java:189)
[error] at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
[error] at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
[error] at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
[error] at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)
[error] org.scalafmt.Error$MisformattedFile: /drone/src/github.com/sbt/zinc/internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala is mis-formatted. With zero explanation on how to fix it. This isn't very contributor-friendly, it could at least display a diff so I know what it wants me to change. If this isn't possible currently, then at least document in CONTRIBUTING.md how to deal with scalafmt in this project. /cc @eed3si9n @dwijnand |
Member
|
@smarter Sorry about that. I think on 1.0.x branch I've bumped to using neo-sbt-scalafmt that just formats automatically. Let me make sure to merge the branch onto 1.x. |
Member
Contributor
Author
|
By the way, I guess this could be backported to the zinc 1.0.x branch if more releases are planned. I don't really understand the relationship between zinc versions and sbt versions, is sbt 1.0.x always going to use zinc 1.0.x ? |
Duhemm
previously approved these changes
Nov 8, 2017
Contributor
Duhemm
left a comment
There was a problem hiding this comment.
LGTM, thanks for the quick fix!
Duhemm
added a commit
to Duhemm/dotty
that referenced
this pull request
Nov 8, 2017
Member
|
I think we should rebase this 1.0.x so we can ship it with zinc 1.0.4 and sbt 1.0.4. |
We were not testing what we thought we were testing before, because C used the name "x" which is also the name of the parameter we change in A, so when we changed A we were invalidating C too. Fixed by using a different name than "x" in C, which reveals that there is a bug somewhere since the test doesn't pass anymore.
d8ed7db to
68463fb
Compare
Quoting from 1e7e99e: If the underlying type of a value class change, its name hash doesn't change, but the name hash of <init> change and since every class uses the name <init>, we don't need to do anything special to trigger recompilations either This was true until aca8dfa where we started giving unique names to constructors. This broke the value-class-underlying type but this wasn't noticed because the test was broken in the same commit (and has now been fixed in the previous commit in this PR).
68463fb to
0215c84
Compare
Contributor
Author
|
Rebased on top of 1.0.x |
dwijnand
approved these changes
Nov 8, 2017
Member
|
Cheers @smarter. |
Duhemm
added a commit
to Duhemm/dotty
that referenced
this pull request
Nov 13, 2017
allanrenucci
pushed a commit
to dotty-staging/dotty
that referenced
this pull request
Dec 21, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #442
Review by @Duhemm