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

Parse zinc args and pass scalac options into scalafix #8091

Merged
merged 2 commits into from Jul 25, 2019

Conversation

@wiwa
Copy link
Contributor

commented Jul 22, 2019

Problem

#7914

Solution

Extract scalac options from zinc args, akin to ExtractJava,
and feed them to scalafix args.

Result

Allows for usage of scalafix rules which require certain compiler options,
such as RemoveUnused, which requires a -Ywarn-unused option.

@wiwa wiwa force-pushed the wiwa:f/scalac-options branch 2 times, most recently from 4feaa01 to 12d1cd0 Jul 22, 2019

@@ -50,9 +50,30 @@ def source_extension(cls):
@classmethod
def prepare(cls, options, round_manager):
super().prepare(options, round_manager)
round_manager.require_data('zinc_args')

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

The zinc_args product is produced by compilation, similar to the runtime_classpath product. So this needs to be under the if options.semantic check.

This is causing the CI failures probably.

@wiwa wiwa force-pushed the wiwa:f/scalac-options branch from 12d1cd0 to a31d0f7 Jul 24, 2019

@stuhood
Copy link
Member

left a comment

The mutable property is pretty odd: would you mind switching that to use memoized_property instead?

round_manager.require_data('runtime_classpath')

def execute(self):
self.scalac_args = []

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 24, 2019

Member

Rather than using a mutable property pre-execute like this, please swap this to using @memoized_property (approximately Scala's lazy val) and then lazily computing the value:

@memoized_property
def _scalac_args(self):
  if self.get_options().semantic:
  ...

This comment has been minimized.

Copy link
@wiwa

wiwa Jul 24, 2019

Author Contributor

Changed

@ity

ity approved these changes Jul 24, 2019

Copy link
Contributor

left a comment

looks good, sans Stu's comment on lazy val

@@ -79,6 +101,12 @@ def invoke_tool(self, absolute_root, target_sources):
args.append('--rules={}'.format(self.get_options().rules))
if self.get_options().level == 'debug':
args.append('--verbose')

# This is how you pass a list of strings to a single arg key

This comment has been minimized.

Copy link
@ity

ity Jul 24, 2019

Contributor

i still cant believe what a rabbit hole this was!

@@ -53,6 +53,35 @@ def test_scalafix_disabled(self):
test_fix = self.run_pants(['lint', target], options)
self.assert_success(test_fix)

def test_scalafix_scalacoptions(self):

This comment has been minimized.

Copy link
@ity

ity Jul 24, 2019

Contributor

thanks for adding the test!

@stuhood
Copy link
Member

left a comment

Thanks!

@stuhood stuhood force-pushed the wiwa:f/scalac-options branch from 33edbe3 to ccb2fa2 Jul 25, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

I think that at least some of the CI failures were due to a race with changes landing in master. Have rebased.

@stuhood stuhood merged commit 89a6425 into pantsbuild:master Jul 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.