Add plugin for scalafix #4635

stuhood · 2017-05-31T09:30:36Z

Problem

scalafix supports a bunch of type-aware rewrites of scala code using scalameta, but pants does not currently support it.

Solution

Add support for scalafix in core, with support for semantic rewrites disabled by default. To enable semantic rewrites, it requires an scala compiler plugin, and so enabling it requires extra setup documented here in the README.md for scala.

Result

Running something like:

./pants fmt.scalafix --semantic --rules=RemoveUnusedImports src/scala/org/pantsbuild/zinc/::

would strip unused imports and results in diffs like:

 import sbt.util.{
-  AbstractLogger,
+
   Level,
   Logger
 }

stuhood · 2017-05-31T09:31:10Z

This will need a few more updates (namely a fix for scalacenter/scalafix#176) before landing, so not adding reviewers yet.

stuhood · 2017-06-21T18:47:50Z

This is now reviewable. Will add reviewers once I have green CI.

xeno-by · 2017-06-22T01:14:02Z

BUILD.tools

+jar_library(
+  name = 'scalac-plugin-dep',
+  jars = [jar(org='org.scalameta', name='scalahost_{}'.format(SCALA_REV), rev='1.8.0')],
+)


Would it be possible to experiment with pre-release versions of scalameta without forking pants?

Yes. Tool definitions can be reconfigured on a repo by repo basis.

After our recent release, this should now be jars = [jar(org='org.scalameta', name='scalac-semanticdb_{}'.format(SCALA_REV), rev='2.0.0')],.

xeno-by · 2017-06-22T01:14:58Z

contrib/scalafix/src/python/pants/contrib/scalafix/BUILD

+    'src/python/pants/goal:task_registrar',
+  ],
+  distribution_name='pantsbuild.pants.contrib.scalafix',
+  description='scalafix support for pants.',


@olafurpg What is the appropriate capitalization of "scalafix" here?

I use capital S at the start of sentences and lower-case inside sentences, same as we do with scalameta.

xeno-by · 2017-06-22T01:19:18Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+    args.append('--sourceroot={}'.format(results_dir))
+    args.append('--classpath={}'.format(':'.join(classpath)))
+    if self.get_options().config:
+      args.append('--config={}'.format(self.get_options().config))


@olafurpg What does take precedence: --config or --sourceroot/--classpath?

--config does not have --classpath or --sourceroot, see
https://github.com/scalacenter/scalafix/blob/9b1b07021ba48a56906bfb26ec86c7cf209d32c7/scalafix-core/src/main/scala/scalafix/config/ScalafixConfig.scala#L14
and
https://github.com/scalacenter/scalafix/blob/9b1b07021ba48a56906bfb26ec86c7cf209d32c7/scalafix-cli/src/main/scala/scalafix/cli/ScalafixOptions.scala#L32

I admit it's a bit confusing to have different systems for cli options and the scalafix configuration.

xeno-by · 2017-06-22T01:26:15Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+  def _run(self, target, results_dir, classpath):
+    # We always operate on copies of the files, so we execute in place.
+    args = ['--in-place']
+    args.append('--sourceroot={}'.format(results_dir))


--sourceroot should point to the root of the build. Is this what results_dir stands for?

This is when we are consuming a database, rather than when we're creating it. We're executing the rewrites on files that have been cloned elsewhere (into a private result_dir), rather that directly in the repo. The sourceroot at compile time (database creation time) is relative to the root of the repo, but here it is being consumed on separate copies of the files.

This allows for both ScalaFixCheck and ScalaFixFix to be implemented with the same codepaths.

xeno-by · 2017-06-22T01:27:00Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+    if self.get_options().rewrites:
+      args.append('--rewrites={}'.format(self.get_options().rewrites))
+    if self.get_options().level == 'debug':
+      args.append('--verbose')


Scalafix expects (or is supposed to expect) input files to be specified explicitly via the -f option. Is this option set anywhere in the current implementation?

-f or --files is optional, unbound trailing arguments are interpreted as files scalafix foo.scala

xeno-by · 2017-06-22T01:30:35Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+    if self.get_options().rewrites:
+      args.append('--rewrites={}'.format(self.get_options().rewrites))
+    if self.get_options().level == 'debug':
+      args.append('--verbose')


Most of scalafix rewrites need a semantic db to be generated for target files. Is it possible / does it make sense to provide sanity checks for that here?

can you elaborate?

Yes, this was somewhat ambiguous. What I meant to ask is whether it'd make sense to provide additional safeguards on the Pants side. Iirc, scalafix HEAD says "something something please recompile" if it doesn't find semantic dbs, but this message isn't going to help a Pants plugin user who tries to run scalafix without having a scalahost plugin in the first place.

I haven't actually seen the error, so not sure how good it is right now... but would love for the error from scalafix to be sufficiently helpful that I don't need to check for existence here.

Some sort of error message saying "make sure you set up the scalameta plugin using the blahblah BUILD.tools target or the blahblah option" would be helpful though. And that can't come from scalafix.

scalafix will still in some cases silently fail on misconfigured semanticdbs, scalacenter/scalafix#264 This is a bug, scalafix should report helpful error messages when things go wrong and report the appropriate exit code to accompany it. For example, scalafix will exit with code 32 when it encounters a "stale semanticdb" (file contents doesn't match semanticdb contents).

xeno-by · 2017-06-22T01:31:22Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+
+  def execute(self):
+    classpaths = self.context.products.get_data('runtime_classpath')
+    # NB: This task uses only the literal classpath of each target, so does not need


What's literal classpath?

Well... it's a thing that I don't have any other name for, frankly.

"transitive": All transitive dependencies of a compile unit.
"direct"/"strict": Only the directly declared dependencies of a compile unit.
"literal"/???: Only the classfiles for the compile unit, and none of its dependencies.

If you have something else that you call that, happy to rename.

Got it. I just did a bit of googling and didn't find a definition, so decided to ask you to better understand what's going on.

Something like "uses only the classes directly generated from the sources of the target"?

benjyw

Looks good, as far as I can tell (no scalafix expertise here...)

It would be nice to have a README.md explaining how to set this up, including the BUILD.tools and pants.ini changes required.

benjyw · 2017-06-22T20:18:06Z

contrib/scalafix/src/python/pants/contrib/scalafix/BUILD

+contrib_plugin(
+  name='plugin',
+  dependencies=[
+    'contrib/node/src/python/pants/contrib/node:plugin',


Derp, thanks.

benjyw · 2017-06-22T20:22:20Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+
+  def execute(self):
+    classpaths = self.context.products.get_data('runtime_classpath')
+    # NB: This task uses only the literal classpath of each target, so does not need


Something like "uses only the classes directly generated from the sources of the target"?

benjyw · 2017-06-22T20:24:04Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+    if self.get_options().rewrites:
+      args.append('--rewrites={}'.format(self.get_options().rewrites))
+    if self.get_options().level == 'debug':
+      args.append('--verbose')


Some sort of error message saying "make sure you set up the scalameta plugin using the blahblah BUILD.tools target or the blahblah option" would be helpful though. And that can't come from scalafix.

stuhood · 2017-06-22T23:13:53Z

Hah. It looks like (some) of the CI failures are due to duels happening between scalafix and scalafmt, where scalafmt re-writes some files, and then scalafix rewrites them again from a cached copy which reverts the changes. Thinking about how best to address that (hopefully not by disabling caching for the fmt.scalafix task).

olafurpg · 2017-06-23T07:21:39Z

@stuhood I've considered consolidating the two into a single cli tool. It should be possible to model scalafmt as a syntactic scalafix rewrite and add support to run scalafmt at the end. Do you think something like that might be useful?

olafurpg · 2017-06-23T12:46:34Z

Heads up @stuhood, I've been polishing the cli lately to make it (hopefully) more intuitive. The main changes relate to how scalafix finds files. In a nutshell, scalafix will no longer infer the .scala files from the --classpath. Instead, you tell scalafix which files to fix and provide a --classpath to tell scalafix where to look for .semanticdb files. This change makes --include unnecessary, since the file path args act now as --include for both syntactic and semantic rewrites.

mateor

It would be nice to ship a test with this but I am okay with it as-is. Excited to try this out.

olafurpg · 2017-06-30T17:50:39Z

I'm hoping to release scalafix v0.5 around next week Tuesday. The v0.5 milestone has a few simple remaining issues https://github.com/scalacenter/scalafix/milestone/3 I have still yet to document the main changes, but they mostly are aimed to increase the quality-of-life for the cli so it might be worth the wait for this PR

olafurpg

scalafix 0.5.0 is just around the corner, I'm in the process of writing the changelog. Please don't hesitate to ask questions if anything is unclear. Most people in the OSS community use sbt-scalafix so there may be missing details about direct cli usage in the docs.

I can say upfront however that the cli still needs more work on error reporting, esp. related to semanticdb loading.

olafurpg · 2017-08-11T17:10:56Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+    if self.get_options().rewrites:
+      args.append('--rewrites={}'.format(self.get_options().rewrites))
+    if self.get_options().level == 'debug':
+      args.append('--verbose')


scalafix will still in some cases silently fail on misconfigured semanticdbs, scalacenter/scalafix#264 This is a bug, scalafix should report helpful error messages when things go wrong and report the appropriate exit code to accompany it. For example, scalafix will exit with code 32 when it encounters a "stale semanticdb" (file contents doesn't match semanticdb contents).

stuhood · 2017-08-14T22:23:51Z

We learned a bit more from b6cf3e5 about how formatters should work, so I've started down the path of implementing a shared base class for scalafmt and scalafix: now added here.

The lint/check versions of the tools need to run as a batch with xargs in order for the performance to not be atrocious (as scalafmt now does). Additionally, we need caching for the checks. But the fmt/rewrite versions of the tools can't use invalidation at all, for the reasons mentioned above: our fingerprinting doesn't react well to files changing out from under it.

The final interesting bit that I want to include in this change is that scalafix needs a classpath for some cases: it's become obvious internally that we cannot "always" require a classpath for lint, as it would make it way too slow to require compiling everything before running the checks. So we'll need to add a task-level option that enables or disables passing the classpath to scalafix (similar to 4b34617): that will allow the task to run either with or without a dependency on the user classpath. That way you can install cheaper scalafix checks in lint, and more expensive checks elsewhere.

Unfortunately, I'm out until September 10th, so there won't be any movement from me until at least then.

olafurpg · 2017-09-12T09:51:32Z

Scalafix 0.5.0-RC3 is out http://www.scala-lang.org/blog/2017/09/11/scalafix-v0.5.html, changelog https://github.com/scalacenter/scalafix/releases/tag/v0.5.0-RC3 I think the api (both library and cli flags) will remain stable for a while now, at least I don't see any breaking changes coming up anytime soon.

xeno-by

Thanks a lot for your work! I've made some change requests based on our recent Scalameta release.

xeno-by · 2017-09-12T21:37:43Z

BUILD.tools

+jar_library(
+  name = 'scalac-plugin-dep',
+  jars = [jar(org='org.scalameta', name='scalahost_{}'.format(SCALA_REV), rev='1.8.0')],
+)


After our recent release, this should now be jars = [jar(org='org.scalameta', name='scalac-semanticdb_{}'.format(SCALA_REV), rev='2.0.0')],.

xeno-by · 2017-09-12T21:38:45Z

contrib/scalafix/README.md

+Unlike `scalafmt` (which is included in the default distribution of pants), `scalafix` requires a
+compiler plugin, and is thus distributed as a contrib module with extra setup steps.
+
+The [scalameta](http://scalameta.org/) compiler plugin extracts semantic information at compile


The [semanticdb](http://scalameta.org/) compiler plugin...

xeno-by · 2017-09-12T21:39:08Z

contrib/scalafix/README.md

+  name = 'scalac-plugin-dep',
+  jars = [jar(org='org.scalameta', name='scalahost_{}'.format(SCALA_REV), rev='1.8.0')],
+)
+```


See the suggested change to scalac-plugin-dep above.

xeno-by · 2017-09-12T21:39:22Z

contrib/scalafix/README.md

+  ]
+
+scalac_plugins: [
+    'scalahost',


'semanticdb-scalac',

xeno-by · 2017-09-12T21:41:15Z

contrib/scalafix/README.md

+
+## Usage
+
+Tasks for both the `fmt` and `lint` goals are provided, each with the same set of options.


What do you think about calling these goals fix and check in accordance with how these actions are called in Scalafix? https://github.com/scalacenter/scalafix/blob/master/scalafix-core/shared/src/main/scala/scalafix/rule/Rule.scala.

xeno-by · 2017-09-12T21:42:04Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+  def register_options(cls, register):
+    super(ScalaFix, cls).register_options(register)
+    register('--skip', type=bool, fingerprint=True,
+             help='Skip running scalafix.')


Why do we need this option?

xeno-by · 2017-09-12T21:42:34Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+    classpaths = self.context.products.get_data('runtime_classpath')
+    # NB: While this task uses only the classes directly generated from the sources of each
+    # target, the content of the generated semantic DBs will be affected by changes to
+    # dependencies. Thus, we use invalidate_dependents=True.


Very good point!

xeno-by · 2017-09-12T21:43:11Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+                          invalidate_dependents=True) as invalidation_check:
+      for vt in invalidation_check.all_vts:
+        if not vt.valid:
+          self.context.log.debug('Fixing {}...'.format(vt.target.address.spec))


May I suggest changing the verb here depending on the goal that is being executed?

xeno-by · 2017-09-12T21:43:56Z

pants.ini

  ]
 no_warning_args: [
    '-S-nowarn',
  ]

+scalac_plugins: [
+    'scalahost',


'semanticdb-scalac',

xeno-by · 2017-09-12T21:44:43Z

src/python/pants/backend/jvm/tasks/scalafmt.py

+                          JarDependency(org='com.geirsson',
+                                        name='scalafmt-cli_2.11',
+                                        rev='1.0.0-RC4')
+                          ])


Looks like an outdated version.

xeno-by · 2017-09-17T15:40:57Z

contrib/scalafix/src/python/pants/contrib/scalafix/tasks/scalafix.py

+      shutil.copy(src, dst)
+
+    # Execute.
+    result = self.runjava(classpath=self.tool_classpath('scalafix'),


Am I right in thinking that scalafix is supposed to be bundled with Pants?

stuhood · 2017-09-21T21:52:38Z

Alright, I've made a bunch of changes:

scalafmt and scalafix now extend a ScalaRewriteBase class to provide the generic harnessy bits
added a --transitive option on ScalaRewriteBase
added a --semantic flag for scalafix, to toggle whether the user classpath (and thus semanticdbs) are required.

I'm going to apply @xeno-by's feedback with regard to versioning. I also think that I can remove the contrib module and move this into core by changing the default of the --semantic flag to False: that would allow the tasks to be installed by default in a mode that doesn't require the compiler plugins (or compilation at all)... parties interested in enabling semantic rewrites would still need to install the semanticdb-scalac plugin as documented.

…actual resolution from an ivy-style repository.

…eaking in.

…ests when scalafix is skipped.

…led).

…ermetic tests to pass.

…ight still be exploding on a bad match...?

stuhood · 2017-10-12T22:48:38Z

Test flakiness. Merging.

stuhood force-pushed the stuhood/scalafix branch from c942fb8 to 5ec8ed0 Compare June 10, 2017 01:44

stuhood requested review from baroquebobcat, benjyw and mateor June 22, 2017 01:06

xeno-by reviewed Jun 22, 2017

View reviewed changes

benjyw approved these changes Jun 22, 2017

View reviewed changes

mateor approved these changes Jun 30, 2017

View reviewed changes

stuhood mentioned this pull request Jul 28, 2017

Pass JVM options to scalafmt #4774

Merged

olafurpg reviewed Aug 11, 2017

View reviewed changes

xeno-by suggested changes Sep 12, 2017

View reviewed changes

xeno-by reviewed Sep 17, 2017

View reviewed changes

stuhood mentioned this pull request Oct 4, 2017

Add intransitive modes to fmt subtasks #4838

Closed

stuhood added 8 commits October 9, 2017 12:22

Initial commit, still with a bunch of vestigial copypasta.

cc814c0

Get the skeleton of the task in place, and install it.

14d5b43

Clone files to the destination and then operate in-place.

a284e8b

Get a patched version of scalafix working. Two new issues discovered.

6868c67

Bump to a version of scalafix that depends on scalahost-nsc instead.

047cb33

Enable scalahost.

cf4346c

Drop the conf=default line in our ivy.xml templates, which prevent …

44a9d82

…actual resolution from an ivy-style repository.

Update for 0.4.0 release of scalafix

1c778a5

stuhood added 16 commits October 9, 2017 12:22

Mark scala integration tests hermetic, to avoid pants` own settings l…

d686978

…eaking in.

Excess argument.

0594e06

Update scalafix version.

ed2ddd6

Extract a base class for scalafmt and scalafix.

c30b1e0

Fix test cleanup and modernize override config: scalafmt passes all t…

1cb0f69

…ests when scalafix is skipped.

Conditionally enable invalidation for non-sideeffecting tasks.

5a3f3ff

Extract ScalaRewriteBase as an independent module.

c745dd6

Port scalafix to ScalaRewriteBase (currently with the classpath disab…

6ae653a

…led).

Add conditional usage of the classpath if semantic=True is enabled.

015acde

Expand documentation.

f60cdad

Remove the scalafix contrib module, and install in core instead.

f280d4a

Modern scalafix

d4a1746

Add a default scalafix rule, to sidestep the fact they are required.

9dcb565

Skip both scalafix and scalafmt, but reformat test sources to allow h…

b9895ac

…ermetic tests to pass.

Fix hermetic=True scalastyle tests.

95b260c

Use isolated testproject targets to test the repl.

faa5ae4

stuhood force-pushed the stuhood/scalafix branch from 65fb5f4 to faa5ae4 Compare October 9, 2017 21:24

stuhood added 2 commits October 9, 2017 16:34

WIP: Bump to latest version of the compiler plugin... seems like it m…

fe5f6d8

…ight still be exploding on a bad match...?

Remove default usage of semanticdbs.

8a92954

stuhood force-pushed the stuhood/scalafix branch from 5a0ed74 to 8a92954 Compare October 9, 2017 23:58

stuhood added 2 commits October 10, 2017 10:29

Add formatting and linting sections to the scala readme.

03cb327

Add a scalafix test, and actually pass relevant args.

cd5548e

stuhood changed the title ~~Add contrib plugin for scalafix~~ Add plugin for scalafix Oct 10, 2017

stuhood closed this Oct 10, 2017

stuhood deleted the stuhood/scalafix branch October 10, 2017 19:04

stuhood reopened this Oct 10, 2017

stuhood added 2 commits October 10, 2017 12:14

Whitespace to trigger travis.

b03f1a3

Merge branch 'master' into stuhood/scalafix

8ed90de

stuhood merged commit 1889b93 into pantsbuild:master Oct 12, 2017


		## Usage

		Tasks for both the `fmt` and `lint` goals are provided, each with the same set of options.

Add plugin for scalafix #4635

Add plugin for scalafix #4635

Conversation

stuhood commented May 31, 2017 • edited Loading

Problem

Solution

Result

stuhood commented May 31, 2017

stuhood commented Jun 21, 2017

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

stuhood Jun 22, 2017 • edited Loading

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

xeno-by Jun 22, 2017 • edited Loading

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

stuhood Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw left a 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

stuhood commented Jun 22, 2017

olafurpg commented Jun 23, 2017

olafurpg commented Jun 23, 2017

mateor left a comment

Choose a reason for hiding this comment

olafurpg commented Jun 30, 2017

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Aug 14, 2017 • edited Loading

olafurpg commented Sep 12, 2017

xeno-by left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Sep 21, 2017

stuhood commented Oct 12, 2017

stuhood commented May 31, 2017 •

edited

Loading

stuhood Jun 22, 2017 •

edited

Loading

xeno-by Jun 22, 2017 •

edited

Loading

stuhood Jun 22, 2017 •

edited

Loading

stuhood commented Aug 14, 2017 •

edited

Loading