Fixed ExplicitResultTypes for implicit members when memberKind and visibility configs are present #1627

rvacaru · 2022-07-01T15:52:00Z

Tests ran after changes:

unit2_13Target2_13/testOnly: OK
unit2_12Target2_12/testOnly: OK

unit2_11Target2_11/testOnly

this one fails with:

  [info] ScalaTest
  [info] Run completed in 1 minute, 5 seconds.
  [info] Total number of tests run: 309
  [info] Suites: completed 36, aborted 0
  [info] Tests: succeeded 297, failed 12, canceled 0, ignored 1, pending 0
  [info] *** 12 TESTS FAILED ***
  [error] Failed: Total 321, Failed 12, Errors 0, Passed 309, Ignored 1
  [error] Failed tests:
  [error]         scalafix.tests.core.PrettyTypeSuite
  [error]         scalafix.tests.v1.SymbolInformationSuite
  [error] (unit2_11Target2_11 / Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
  [error] Total time: 94 s (01:34), completed 1 Jul 2022, 18:12:26

scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala

rvacaru · 2022-07-01T15:55:53Z

scalafix-tests/output/src/main/scala-2/test/explicitResultTypes/ExplicitResultTypesBase.scala

@@ -15,7 +15,7 @@ object ExplicitResultTypesBase {
  private val g = 1
  private def h(a: Int) = ""
  private var i = 22
-  private implicit var j: Int = 1
+  private implicit var j = 1


here the rule should not run, as per the config provided in the input file:

/* rules = ExplicitResultTypes ExplicitResultTypes.memberKind = [Val, Def, Var] ExplicitResultTypes.memberVisibility = [Public, Protected] */

bjaglin

Well done! Can you rebase against master to fix the conflict introduced by #1623 ? Thanks!

scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala

rvacaru · 2022-07-05T14:59:32Z

scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala

+    def matchesMemberKindAndVisibility: Boolean =
+      matchesMemberKind && matchesMemberVisibility
+
+    def evaluateWhenNotImplicitOrFinalLiteralVal: Boolean = {


if you have a better name for this block let me know, I couldn't find a better one not based on the first isImplicitAndNotFinalLiteralVal block

I don't get the OrFinalLiteralVal part here - isn't the return value of this function independant of whether it is a final val literal or not?

the name doesn't reflect what the method does (or the content of the method) but reflects which branch of the logical expression of isRuleCandidate is executed, in relation with the first block in OR. This makes the logical expression more readable but the method itself less readable..it's a trade off and I think it's better if the logical expression is more readable, that's why we extracted the method in the first place

rvacaru · 2022-07-05T15:06:03Z

Running unit2_13Target2_13/testOnly now makes 4 tests fail in scalafix.tests.interfaces.ScalafixSuite, as it fails to fetch some snapshot dependencies (most likely not introduce by this PR):

[info] - fetch & load instance for Scala version 2.12 *** FAILED *** (644 milliseconds)
[info]   scalafix.interfaces.ScalafixException: Failed to fetch [Dependency(Module(ch.epfl.scala, scalafix-cli_2.12.16), 0.10.1+11-574dbdf3+20220705-1644-SNAPSHOT, configuration=runtime)]from [IvyRepository(file:/Users/razvan.vacaru@TMNL.nl/.ivy2/local/[organisation]/[module]/(scala_[scalaVersion]/)(sbt_[sbtVersion]/)[revision]/[type]s/[artifact](-[classifier]).[ext], dropInfoAttributes = true), MavenRepository(https://repo1.maven.org/maven2)]
[info]   at scalafix.internal.interfaces.ScalafixCoursier.fetch(ScalafixCoursier.java:30)
[info]   at scalafix.internal.interfaces.ScalafixCoursier.scalafixCliJars(ScalafixCoursier.java:55)
[info]   at scalafix.interfaces.Scalafix.fetchAndClassloadInstance(Scalafix.java:139)
[info]   at scalafix.tests.interfaces.ScalafixSuite.$anonfun$fetchAndLoad$1(ScalafixSuite.scala:39)
[info]   at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info]   at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
[info]   at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
[info]   at org.scalatest.Transformer.apply(Transformer.scala:22)
[info]   at org.scalatest.Transformer.apply(Transformer.scala:20)
[info]   at org.scalatest.funsuite.AnyFunSuiteLike$$anon$1.apply(AnyFunSuiteLike.scala:189)
[info]   ...
[info]   Cause: coursierapi.error.SimpleResolutionError$1: Error downloading ch.epfl.scala:scalafix-cli_2.12.16:0.10.1+11-574dbdf3+20220705-1644-SNAPSHOT
[info]   not found: /Users/razvan.vacaru@TMNL.nl/.ivy2/local/ch.epfl.scala/scalafix-cli_2.12.16/0.10.1+11-574dbdf3+20220705-1644-SNAPSHOT/ivys/ivy.xml
[info]   not found: https://repo1.maven.org/maven2/ch/epfl/scala/scalafix-cli_2.12.16/0.10.1+11-574dbdf3+20220705-1644-SNAPSHOT/scalafix-cli_2.12.16-0.10.1+11-574dbdf3+20220705-1644-SNAPSHOT.pom
[info]   at coursierapi.error.SimpleResolutionError.of(SimpleResolutionError.java:11)
[info]   at coursierapi.shaded.coursier.internal.api.ApiHelper$.simpleResError(ApiHelper.scala:331)
[info]   at coursierapi.shaded.coursier.internal.api.ApiHelper$.doFetch(ApiHelper.scala:375)
[info]   at coursierapi.shaded.coursier.internal.api.ApiHelper.doFetch(ApiHelper.scala)
[info]   at coursierapi.Fetch.fetchResult(Fetch.java:244)
[info]   at scalafix.internal.interfaces.ScalafixCoursier.fetch(ScalafixCoursier.java:28)
[info]   at scalafix.internal.interfaces.ScalafixCoursier.scalafixCliJars(ScalafixCoursier.java:55)
[info]   at scalafix.interfaces.Scalafix.fetchAndClassloadInstance(Scalafix.java:139)
[info]   at scalafix.tests.interfaces.ScalafixSuite.$anonfun$fetchAndLoad$1(ScalafixSuite.scala:39)
[info]   at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info]   ...

// similar other depencies for different scala versions

bjaglin · 2022-07-06T06:30:32Z

Running unit2_13Target2_13/testOnly now makes 4 tests fail in scalafix.tests.interfaces.ScalafixSuite, as it fails to fetch some snapshot dependencies (most likely not introduce by this PR):

scalafix/scalafix-tests/unit/src/test/scala/scalafix/tests/interfaces/ScalafixSuite.scala

Lines 16 to 22 in c5633c6

    
           /** 
        
            * Tests in this suite require scalafix-cli & its dependencies to be 
        
            * cross-published so that Coursier can fetch them. That is done automatically 
        
            * as part of `sbt unitXTargetY/test`, so make sure to run that once if you want 
        
            * to run the test with testOnly or through BSP. 
        
            */ 
        
           class ScalafixSuite extends AnyFunSuite {

bjaglin · 2022-07-06T08:07:19Z

scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala

-      !matchesSimpleDefinition() &&
-      matchesMemberKind() &&
-      matchesMemberVisibility()
+      !matchesSimpleDefinition()


Not a review comment about this PR, but shouldn't this check be extracted and chained with AND just like matchesMemberKindAndVisibility for both implicit and non-implicit decalaration, so that config.skipSimpleDefinitions is honored everywhere? Maybe worth tracking this in a separate ticket for visbility if confirmed.

will create a ticket for this

Here is the ticket: #1628

bjaglin · 2022-07-06T08:18:30Z

scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala

+    def matchesMemberKindAndVisibility: Boolean =
+      matchesMemberKind && matchesMemberVisibility
+
+    def evaluateWhenNotImplicitOrFinalLiteralVal: Boolean = {


I don't get the OrFinalLiteralVal part here - isn't the return value of this function independant of whether it is a final val literal or not?

scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala

bjaglin · 2022-07-06T08:23:13Z

scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala

@@ -173,13 +173,21 @@ final class ExplicitResultTypes(
    def hasParentWihTemplate: Boolean =
      defn.parent.exists(_.is[Template])

-    isImplicit && !isFinalLiteralVal || {
+    def isImplicitAndNotFinalLiteralVal: Boolean =
+      isImplicit && !isFinalLiteralVal


Not a review comment about this PR, but that isFinalLiteralVal is a bit mysterious to me - it was added in 162db39, but I can't easily see what scenario it covers.

bjaglin

Thanks & congrats for this first PR of the many to come!

Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

rvacaru commented Jul 1, 2022

View reviewed changes

scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala Outdated Show resolved Hide resolved

rvacaru commented Jul 1, 2022

View reviewed changes

bjaglin reviewed Jul 4, 2022

View reviewed changes

scalafix-rules/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala Outdated Show resolved Hide resolved

rvacaru force-pushed the 1216/fix-explicit-result-types-implicit-members branch from ab97dd2 to 574dbdf Compare July 5, 2022 14:23

rvacaru commented Jul 5, 2022

View reviewed changes

bjaglin reviewed Jul 6, 2022

View reviewed changes

rvacaru force-pushed the 1216/fix-explicit-result-types-implicit-members branch from 6e362b2 to 8d12093 Compare July 6, 2022 09:55

bjaglin approved these changes Jul 6, 2022

View reviewed changes

rvacaru mentioned this pull request Jul 6, 2022

ExplicitResultTypes.isRuleCandidate evaluation improvements/fixes #1628

Closed

rvacaru and others added 4 commits July 12, 2022 11:45

all changes squashed - implicit handling with explicitresulttypes config

5daccea

renamed evaluation blocks in isRuleCandidate

1df4777

(chore): fix formatting w/scalafmt

63502d9

renaming predicates of isRuleCandidate based on pr suggestions

fd57526

Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

rvacaru force-pushed the 1216/fix-explicit-result-types-implicit-members branch from 8d12093 to fd57526 Compare July 12, 2022 09:45

bjaglin merged commit 352bad1 into scalacenter:main Jul 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed ExplicitResultTypes for implicit members when memberKind and visibility configs are present #1627

Fixed ExplicitResultTypes for implicit members when memberKind and visibility configs are present #1627

rvacaru commented Jul 1, 2022 •

edited

Loading

rvacaru Jul 1, 2022

bjaglin left a comment

rvacaru Jul 5, 2022 •

edited

Loading

bjaglin Jul 6, 2022

rvacaru Jul 6, 2022

rvacaru commented Jul 5, 2022

bjaglin commented Jul 6, 2022

bjaglin Jul 6, 2022

rvacaru Jul 6, 2022

rvacaru Jul 6, 2022

bjaglin Jul 6, 2022

bjaglin Jul 6, 2022

bjaglin left a comment

Fixed ExplicitResultTypes for implicit members when memberKind and visibility configs are present #1627

Fixed ExplicitResultTypes for implicit members when memberKind and visibility configs are present #1627

Conversation

rvacaru commented Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

bjaglin left a comment

Choose a reason for hiding this comment

rvacaru Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvacaru commented Jul 5, 2022

bjaglin commented Jul 6, 2022

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

bjaglin left a comment

Choose a reason for hiding this comment

rvacaru commented Jul 1, 2022 •

edited

Loading

rvacaru Jul 5, 2022 •

edited

Loading