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

Scalafix full classpath fix #8007

Merged
merged 5 commits into from Jul 12, 2019

Conversation

@wiwa
Copy link
Contributor

commented Jul 3, 2019

Problem

RscCompat is a semantic Scalafix rule and thus requires the full classpath to
build up its symbol table. The current _compute_classpath code only includes
the classpaths of the rewritten targets, but must instead include the classpaths
of the closure.

Solution

Use JvmTask's classpath method to compute the full classpath.

Result

The Scalafix task should now support semantic rules which require use of the
full symbol table, including symbols from libraries depended on by rewritten
targets.

@stuhood
Copy link
Member

left a comment

Thanks! Do you have time to add an integration test for this using https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/backend/jvm/tasks/test_scalafix.py ?

from pants.base.exceptions import TaskError
from pants.java.jar.jar_dependency import JarDependency
from pants.option.custom_types import file_option
from pants.task.fmt_task_mixin import FmtTaskMixin
from pants.task.lint_task_mixin import LintTaskMixin


class ScalaFix(RewriteBase):
class ScalaFix(RewriteBase, JvmTask):

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 3, 2019

Member

This should not be necessary, because RewriteBase already extends NailgunTask, which extends JvmTask.

This comment has been minimized.

Copy link
@wiwa

wiwa Jul 5, 2019

Author Contributor

NailgunTask extends JvmToolTaskMixin which extends JvmToolMixin... which doesn't extend JvmTask?

Also: Exception message: 'ScalaFixGoal_scalafix' object has no attribute 'classpath' (albeit from a slightly older version)

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 10, 2019

Member

Got it.

def _compute_classpath(self, targets):
classpaths = self.context.products.get_data('runtime_classpath')
return [entry for _, entry in classpaths.get_for_targets(targets)]
def _compute_full_classpath(self, targets):

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 3, 2019

Member

Can probably just inline this at this point.

@stuhood stuhood requested a review from cosmicexplorer Jul 3, 2019

@wiwa wiwa force-pushed the wiwa:f/scalafix-fix branch from aa762c0 to f264463 Jul 9, 2019

@stuhood
Copy link
Member

left a comment

The CI failures are suspiciously unrelated, so a rebase is probably in order. Sorry about that.

But there is an actual lint failure in the first shard, so should fix that: https://travis-ci.org/pantsbuild/pants/jobs/556510941

ERROR: /home/travis/build/pantsbuild/pants/tests/python/pants_test/backend/jvm/tasks/test_scalafix.py Imports are incorrectly sorted.
ERROR: /home/travis/build/pantsbuild/pants/src/python/pants/backend/jvm/tasks/scalafix.py Imports are incorrectly sorted.
To fix import sort order, run `"/home/travis/build/pantsbuild/pants/build-support/bin/isort.sh" -f`
from pants.base.exceptions import TaskError
from pants.java.jar.jar_dependency import JarDependency
from pants.option.custom_types import file_option
from pants.task.fmt_task_mixin import FmtTaskMixin
from pants.task.lint_task_mixin import LintTaskMixin


class ScalaFix(RewriteBase):
class ScalaFix(RewriteBase, JvmTask):

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 10, 2019

Member

Got it.


jar_library(
name="rsc-compat",
jars=[jar(org='com.twitter', name='rsc-rules_2.12', rev = "0.0.0-780-e57a1c9c")]

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 10, 2019

Member

This should be a scala_jar.

result = read_file(test_file_name)
result = re.sub(re.escape('object RscCompat {'), 'object RscCompatFixed {', result)
expected = read_file(fixed_file_name)
assert(result == expected)

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 10, 2019

Member

Should switch this to self.assertEqual(result, expected).

result = re.sub(re.escape('object RscCompat {'), 'object RscCompatFixed {', result)
expected = read_file(fixed_file_name)
assert(result == expected)
finally:

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 10, 2019

Member

This try:finally can be replaced with:

with self.with_overwritten_file_content(test_file_name):
  ...

see

@contextmanager
def with_overwritten_file_content(self, file_path, temporary_content=None):
"""A helper that resets a file after the method runs.
It will read a file, save the content, maybe write temporary_content to it, yield, then write the
original content to the file.
:param file_path: Absolute path to the file to be reset after the method runs.
:param temporary_content: Optional content to write into the file.
"""

@wiwa wiwa force-pushed the wiwa:f/scalafix-fix branch 5 times, most recently from f4e5028 to c2d2c7f Jul 10, 2019

@stuhood stuhood requested a review from ity Jul 10, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

The test failures are supremely strange. Do you repro them locally?

@stuhood
Copy link
Member

left a comment

This looks good to me once the test failures are figured out.

# targets.
classpath = self._compute_classpath({target for target, _ in target_sources})
# If semantic checks are enabled, we need the full classpath for these targets.
classpath = self.classpath({target for target, _ in target_sources})

This comment has been minimized.

Copy link
@ity

ity Jul 10, 2019

Contributor

is this doing the right thing when you test? looking at the test failures it looks like that constructed classpath isn't correct, see:
No default jvm platform is defined in https://travis-ci.org/pantsbuild/pants/jobs/556896821#L430 and similar errors in other shard failures.

I would suggest running that ^ test locally and inspecting the classpath generated.

This comment has been minimized.

Copy link
@wiwa

wiwa Jul 11, 2019

Author Contributor

this classpath = ... line should only be be hit when we run scalafix with --semantic, not sure if that happens anywhere.

@wiwa wiwa force-pushed the wiwa:f/scalafix-fix branch from c2d2c7f to 7da20a5 Jul 11, 2019

@wiwa wiwa force-pushed the wiwa:f/scalafix-fix branch 2 times, most recently from 890a951 to d7d00eb Jul 12, 2019

@wiwa wiwa force-pushed the wiwa:f/scalafix-fix branch from d7d00eb to 6411728 Jul 12, 2019

@stuhood
Copy link
Member

left a comment

Thanks!

@stuhood stuhood merged commit ed2b717 into pantsbuild:master Jul 12, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@stuhood stuhood added this to the 1.18.x milestone Jul 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.