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

Scalafmt 1.3, sbt-scalafmt 1.14 #443

Closed
wants to merge 22 commits into from
Closed

Scalafmt 1.3, sbt-scalafmt 1.14 #443

wants to merge 22 commits into from

Conversation

dwijnand
Copy link
Member

and merge 1.0.x into 1.x.

dwijnand and others added 22 commits September 5, 2017 15:24
Follow-up on sbt#314 - I _still_ misinterpreted..

Turns out the ".asInstanceOf[AnyRef].getClass.getName" implementation
was the _original_ implementation. Then Mark switched to using bindValue
in sbt/sbt@4b8f0f3.

Since Scala 2.11.0 (scala/scala#1648 in particular) bindValue was
removed. So we'll use NamedParam and quietBind, both which exist since
Scala 2.9.0.

Fixes sbt/sbt#2884, tested with local releases.
Fix ConsoleInterface binding things properly^2
I get NPE when I extend ManagedLoggedReporter if I don't do this.
neo-sbt-scalafmt and making LoggedReporter extensible
xsbti Java classes were ported into compiler bridge, instead of the compiler interface by mistake.
Since there's not code utilizing this interface yet, this was never caught.
Move REPL related xsbti Java classes to the correct module
sealed test used to be implemented with `-> compile` in sbt/sbt but was marked pending in 8e65c00.
This changes back to the original state.
This commit proves that the error only exists with 2.11.x.
### background

In sbt 0.13 days, we could ignore the relationship between two classes defined
in the same `*.scala` source file, because they will be compiled anyway, and
the invalidation was done at the source file level. With class-based namehashing,
the invalidation is done at the class level, so we can no longer ignore inheritance
relationship coming from the same source, but we still have old assumptions scattered
around the xsbt-dependency implementation.

### what we see without the fix

```
[info] Compiling 1 Scala source to ...
....
[debug] [inv]   internalDependencies:
[debug] [inv]     DependencyByInheritance Relation [
[debug] [inv]     xx.B -> gg.table.A
[debug] [inv]     xx.Foo -> xx.C
[debug] [inv] ]
[debug] [inv]     DependencyByMemberRef Relation [
[debug] [inv]     xx.B -> gg.table.A
[debug] [inv]     xx.Hello -> gg.table.A
[debug] [inv]     xx.Foo -> xx.C
[debug] [inv] ]
....
Caused by: java.lang.AbstractMethodError: xx.Foo.buildNonemptyObjects(II)V
```

First, we see that `xx.C -> xx.B DependencyByInheritance` relationship is missing. Second, the error message seen is `java.lang.AbstractMethodError` happening on `xx.Foo`.

### what this changes

This change changes two if expressions that was used to filter out dependency info coming from the same source.

One might wonder why it's necessary to keep the local inheritance info, if two classes involved are compiled together anyways. The answer is transitive dependencies.
Here's likely what was happening:

1. `gg.table.A` was changed,
2. causing `xx.B` to invalidate.
3. However, because of the missing same-source inheritance, it did not invalidate `xx.C`.
4. This meant that neither `xx.Foo` was invalidated.
5. Calling transform method on a new `xx.Foo` causes runtime error.

By tracking same-source inheritance, we will now correctly invalidate `xx.C` and `xx.Foo`.
I think the assumption that's broken here is that "we don't need to track inheritance that is happening between two classes in a same source."

### Is this 2.11 only issue?

No. The simple trait-trait inheritance reproduction alone will not cause problem in Scala 2.12
because of the [compile-to-interface](http://www.scala-lang.org/news/2.12.0/#traits-compile-to-interfaces) traits.
However, not all traits will compile to interface.
This means that if we want to take advantage of the compile-to-interface traits,
we still should keep track of the same-source inheritance, but introduce some more
logic to determine whether recompilation is necessary.

Fixes sbt#417
`source-dependencies / patMat-scope` passes locally for me.
The invalidation on the CI is likely caused by:

```
[debug] Recompiling all 3 sources: invalidated sources (2) exceeded 50.0% of all sources
```

This attempts to workaround that by adding more source. This does not affect the fidelity of the original test.
Fixes undercompilation on inheritance on same source
It looks like scalac encodes access rights of objects in their names. To
make sure that we get the right simple names, we need to use
`unexpandedName` instead of `name` which will decipher these access
rights and return their simple names insted (with all the previous `$$`
prefixes stripped out).
* 1.0.x:
  Fix sbt#127: Use `unexpanded` name instead of `name`
  Add pending test case for issue/127
  source-dependencies / patMat-scope workaround
  Fixes undercompilation on inheritance on same source
  Add real reproduction case for sbt#417
  Add trait-trait-212 for Scala 2.12.3
  Fix source-dependencies/sealed
  Import statement no longer needed
  Move mima exclusions to its own file
  Move REPL related xsbti Java to correct module
  make LoggedReporter fields lazy
  sbt.version=1.0.2
  use neo-sbt-scalafmt
  Fix ConsoleInterface binding things properly^2
@jvican
Copy link
Member

jvican commented Oct 24, 2017

Can you remove bb7112c? I think that's conflicting with my changes to the build, and I would prefer if I don't have to rebase it, we can incorporate it later.

The merge of 1.0.x to 1.x looks good.

@eed3si9n
Copy link
Member

@dwijnand It seems that the Scalafmt thing is blocking the other useful "and merge 1.0.x into 1.x.", could we split this into two PRs, plz?

@dwijnand
Copy link
Member Author

Merge 1.0.x into 1.x PR: #455

I'll rebase this PR once that's merged.

@dwijnand dwijnand self-assigned this Nov 22, 2017
@dwijnand dwijnand removed their assignment Jan 10, 2018
@dwijnand dwijnand closed this Jan 15, 2018
@dwijnand dwijnand deleted the scalafmt branch January 15, 2018 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants