Second steps towards scalafix.v1 #809

olafurpg · 2018-08-22T21:21:43Z

This PR makes progress towards migrating the codebase to use scalafix.v1 instead of scalafix.v0

Migrate most syntactic rules to v1 API, the diff is minimal
s/v1.Sym/v1.Symbol/ now that scala.meta.Symbol is gone
New kids on the block: v1.Type and v1.Signature. Binary stable front-ends for their protobuf equivalents. Their only divergence from the protobuf data structures is that Scope is replaced with List[SymbolInfo] which is possible thanks to having a symbol table around. Most of the pain was writing equals and hashCode by hand since I think Type and Signature should be "data classes" with reasonable equality
Remove dead rules: xml literals, no finalize, no infer, disable unless
Remove references to v0 in v1 package

TODO:

Now that scala.meta.Symbol no longer exists this rename is safe

I don't think rules should be both rewrites and linters.

codecov · 2018-08-22T21:31:07Z

Codecov Report

Merging #809 into master will decrease coverage by 5.17%.
The diff coverage is 50.26%.

@@            Coverage Diff             @@
##           master     #809      +/-   ##
==========================================
- Coverage   72.55%   67.37%   -5.18%     
==========================================
  Files         158      166       +8     
  Lines        3869     4067     +198     
  Branches      331      332       +1     
==========================================
- Hits         2807     2740      -67     
- Misses       1062     1327     +265

Impacted Files	Coverage Δ
...c/main/scala/scalafix/testkit/DiffAssertions.scala	`28.57% <ø> (ø)`	⬆️
...c/main/scala/scalafix/internal/v0/LegacyRule.scala	`75% <ø> (ø)`
...x-core/src/main/scala/scalafix/v0/Denotation.scala	`83.33% <ø> (ø)`	⬆️
...rc/main/scala/scalafix/internal/rule/Disable.scala	`100% <ø> (ø)`	⬆️
...ts/unit/src/main/scala/scalafix/test/NoDummy.scala	`100% <ø> (ø)`	⬆️
...scalafix/internal/config/DisableSyntaxConfig.scala	`100% <ø> (ø)`	⬆️
...ala/scalafix/internal/reflect/RuleDecoderOps.scala	`61.9% <ø> (ø)`	⬆️
...calafix-core/src/main/scala/scalafix/package.scala	`91.66% <ø> (ø)`	⬆️
...a/scalafix/internal/patch/DeprecatedPatchOps.scala	`52.63% <ø> (-15.79%)`	⬇️
...cala/scalafix/internal/config/ScalafixConfig.scala	`100% <ø> (ø)`	⬆️
... and 95 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d9d610...87c3276. Read the comment docs.

xeno-by · 2018-08-22T21:29:55Z

scalafix-core/src/main/scala/scalafix/v1/SymbolInfo.scala

+  def owner: Symbol = Symbol(info.symbol).owner
+  def name: String = info.name
+  def kind: SymbolKind = new SymbolKind(info)
+  def props: SymbolProperties = new SymbolProperties(info.properties)


How about inlining kind.isXXX and props.isXXX into SymbolInfo, so that it's info.isFinal and info.isMethod instead of info.props.isFinal and info.kind.isMethod?

scalameta/scalameta#1750

xeno-by · 2018-08-22T21:31:09Z

scalafix-core/src/main/scala/scalafix/v1/SymbolAccess.scala

+    case _ => scala.None
+  }
+  def isPublic: Boolean = a.isInstanceOf[s.PublicAccess]
+  def isNone: Boolean = a == s.NoAccess


An alternative approach to this API would be to replace privateWithin and protectedWithin with isPrivateWithin, isProtectedWithin and within. I think I like that better than the current API.

scalameta/scalameta#1750

xeno-by · 2018-08-22T21:31:56Z

scalafix-core/src/main/scala/scalafix/v1/SymbolProperties.scala

+    if (isVal) buf += "val"
+    if (isVar) buf += "var"
+    buf.mkString("SymbolProperties(", " ", ")")
+  }


I wonder if we can use metap for that.

We can, but it requires making metap more flexible: scalameta/scalameta#1751

xeno-by · 2018-08-22T21:32:35Z

scalafix-core/src/main/scala/scalafix/internal/v1/FromProtobuf.scala

+//  def constant(c: s.Constant): Constant = c match {
+//    case s.NoConstant => throw new IllegalArgumentException(c.toString)
+//    case s.NoConstant => throw new IllegalArgumentException(c.toString)
+//  }


Is this used anywhere?

It's not, removed.

xeno-by · 2018-08-22T21:35:33Z

scalafix-core/src/main/scala/scalafix/v1/Symbol.scala

@@ -9,7 +9,7 @@ final class Symbol private (val value: String) {
  def isGlobal: Boolean = value.isGlobal
  def isLocal: Boolean = value.isLocal
  def owner: Symbol = Symbol(value.owner)
-  def info(doc: SemanticDoc): SymbolInfo = doc.info(this)
+  def info(doc: SemanticDoc): Option[SymbolInfo] = doc.info(this)


Looks like there are two ways of doing the same thing: doc.info and symbol.info. How about we keep only the former?

I agree having two ways to do the same thing is a smell, I will go for Symbol.info with an implicit SemanticDoc instead. SemanticDoc is already implicit so that it can be passed to unapply methods.

We should update to latest scalafmt to automatically fix this with `trailingCommas=never`

This makes it possible to experiement with different kinds of symbol matchers: - prefix trees for instant lookups - combined symbol matchers

This commit refactors Doc.tree to be computed on demand. This makes it possible to implement rules that don't even parse the source file, which could save a lot of redundant work when running on large codebases for example when only targeting files that reference a single symbol.

This required exposing messages in the API

This aligns the scalafix API with semanticdb/lsp terminology. It was always weird to write `messsage.message` to get the string message from a `LintMessage`. The `LintMessage` name remains accessible via forwarders for v0.

- LintMessage and LintCategory belong in the v0 package. Refactor v1 rules to use Diagnostic directly instead. - Diagnostic.apply is a simple constructor for simple use-cases - LintDiagnostic doesn't need to be a trait, fewer extension points is better. - remove dead code

olafurpg · 2018-08-24T12:23:35Z

Will merge before this grows any bigger. All rules except Disable are now using scalafix.v1. The v1 API has been polished in several ways

Doc and SemanticDoc are small easy-to-understand classes and implementation details have moved to InternalDoc and InternalSemanticDoc that are in the scalafix.internal.v1 package.
Doc.tree is now lazy, meaning it's possible skip parsing a source file if the Input or TextDocument is not interesting. For example, if renaming a single symbol then it's possible to skip parsing files that don't reference that symbols. For large corpora, this can make a big difference.

These were originally copy-pasted before M10 was released.

This required introducing a new trait scalafix.v1.Symtab since - we need access to the symtab in order to pretty-print symbols - if we use SemanticDoc then we'd need to construct bogus Tree instances in order to construct a basic SymbolInfo. We should not let `SemanticDoc` creep in everywhere as a dependency to even the most trivial functionality.

olafurpg · 2018-08-24T13:13:36Z

Gonna merge to unblock a PR from @MasseGuillaume on the website, happy to iterate further on this.

I believe it will be more interesting to discuss the new v1 API once the docs are in, which is next on my queue :)

olafurpg added 11 commits August 22, 2018 21:48

Rename v1.Sym to v1.Symbol

4981ff1

Now that scala.meta.Symbol no longer exists this rename is safe

Break down v1.Symbol.{Info,Kind,...} into separate top-levels

39aaa37

Implement missing methods in v1 symbol API

7394d38

Implement Signature

4a191e1

Complete rest of semanticdb data structures

e1c4316

Implement conversions from protobuf to scalafix.v1

65f9081

Remove several references to v0 from v1 package

c216efb

Make info(Symbol) return Option[SymbolInfo]

839ad33

Migrate DisableSyntax to v1

0595d7f

Migrate syntactic rules to v1

f934943

Make DisableSyntax.noFinalVal a linter instead of rewrite

7198487

I don't think rules should be both rewrites and linters.

olafurpg requested a review from xeno-by August 22, 2018 21:21

Add scala-xml

52062c6

xeno-by reviewed Aug 22, 2018

View reviewed changes

olafurpg added 15 commits August 23, 2018 09:14

Remove trailing commas

eb376ab

We should update to latest scalafmt to automatically fix this with `trailingCommas=never`

Clean up SemanticDoc by moving private[scalafix] to internal class

81cc01b

Make SymbolMatcher a trait defined by single Symbol => Boolean method

31cf36d

This makes it possible to experiement with different kinds of symbol matchers: - prefix trees for instant lookups - combined symbol matchers

Add sanity test suite for lazy value

d08dac0

Assert values in new test suite

82bfc0e

Add an integration test to demonstrate that LazyValue[Tree] works

9fe94d8

Upgrade to latest scalameta milestone

6eba621

Migrate message-based rules to v1

e7aee9c

This required exposing messages in the API

s/LintMessage/Diagnostic/

93c993a

This aligns the scalafix API with semanticdb/lsp terminology. It was always weird to write `messsage.message` to get the string message from a `LintMessage`. The `LintMessage` name remains accessible via forwarders for v0.

Use clearer name for suppression skipping

535a233

Improve utilities for v1.Symbol

25a9603

Upgrade ExplicitResultTypes to v1

9e40b55

Migrate MissingFinal to v1

ea560e1

olafurpg added 2 commits August 24, 2018 13:41

Migrate remaining syntactic rules

595f249

Rename v1.Type to v1.Tpe to avoid conflicts with scala.meta.Type

d60f8c2

olafurpg added 3 commits August 24, 2018 14:27

Migrate DottyVarArgPattern and DottyKeywords to v1

a0217b2

Use new isX methods from scalameta M10

868ff20

These were originally copy-pasted before M10 was released.

olafurpg merged commit efa85ee into scalacenter:master Aug 24, 2018

olafurpg deleted the apis branch August 24, 2018 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Second steps towards scalafix.v1 #809

Second steps towards scalafix.v1 #809

olafurpg commented Aug 22, 2018 •

edited

Loading

codecov bot commented Aug 22, 2018 •

edited

Loading

xeno-by Aug 22, 2018

xeno-by Aug 23, 2018

xeno-by Aug 22, 2018

xeno-by Aug 23, 2018

xeno-by Aug 22, 2018

olafurpg Aug 23, 2018

xeno-by Aug 22, 2018

olafurpg Aug 23, 2018

xeno-by Aug 22, 2018

olafurpg Aug 23, 2018

olafurpg commented Aug 24, 2018

olafurpg commented Aug 24, 2018

Second steps towards scalafix.v1 #809

Second steps towards scalafix.v1 #809

Conversation

olafurpg commented Aug 22, 2018 • edited Loading

codecov bot commented Aug 22, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Aug 24, 2018

olafurpg commented Aug 24, 2018

olafurpg commented Aug 22, 2018 •

edited

Loading

codecov bot commented Aug 22, 2018 •

edited

Loading