From 001509b3b8710de30b748aae2f83993dc4c06783 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Wa=C5=9Bko?= Date: Thu, 2 Jul 2020 13:39:42 +0200 Subject: [PATCH] Fix missing warning for unused arguments (#960) --- docs/CONTRIBUTING.md | 5 +- .../scala/org/enso/compiler/core/IR.scala | 8 --- .../compiler/pass/desugar/ComplexType.scala | 2 +- .../desugar/LambdaShorthandToLambda.scala | 15 ++--- .../pass/desugar/NestedPatternMatch.scala | 4 +- .../pass/desugar/OperatorToFunction.scala | 4 +- .../pass/desugar/SectionsToBinOp.scala | 8 +-- .../pass/lint/ShadowedPatternFields.scala | 4 +- .../compiler/pass/lint/UnusedBindings.scala | 11 ++-- .../pass/optimise/ApplicationSaturation.scala | 7 +-- .../pass/optimise/LambdaConsolidate.scala | 13 ++--- .../optimise/UnreachableMatchBranches.scala | 4 +- .../pass/resolve/IgnoredBindings.scala | 40 ++++++++++---- .../compiler/pass/resolve/TypeFunctions.scala | 12 ++-- .../pass/analyse/GatherDiagnosticsTest.scala | 7 ++- .../test/pass/lint/UnusedBindingsTest.scala | 55 ++++++++++++++++++- .../searcher/sql/SuggestionsRepoTest.scala | 8 +-- 17 files changed, 127 insertions(+), 80 deletions(-) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 0826354a28..4515f9404d 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -236,9 +236,6 @@ a vanilla GraalVM distribution. To run it, use: JAVA_HOME= ./enso.jar ``` -If you decide not to use the default launcher script, make sure to pass -the `-XX:-UseJVMCIClassLoader` option to the `java` command. - #### Installing the Jupyter kernel Enso has a highly experimental and not-actively-maintained Jupyer Kernel. To run it: @@ -411,7 +408,7 @@ Below are options uses by the Language Server: To run the Language Server on 127.0.0.1:8080 type: ```bash -java -jar enso.jar \ +./enso.jar \ --server \ --root-id 3256d10d-45be-45b1-9ea4-7912ef4226b1 \ --path /tmp/content-root diff --git a/engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala b/engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala index c5e1fc0a94..95186cd5a4 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala @@ -350,14 +350,6 @@ object IR { override def children: List[IR] = imports ++ bindings - def transformExpressions( - fn: PartialFunction[Expression, Expression] - ): Module = { - copy( - bindings = bindings.map(_.mapExpressions(_.transformExpressions(fn))) - ) - } - override def toString: String = s""" |IR.Module( diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/ComplexType.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/ComplexType.scala index 9a72fcd2ed..59dcd60b74 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/ComplexType.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/ComplexType.scala @@ -266,7 +266,7 @@ case object ComplexType extends IRPass { val binding = Method.Binding( methodRef.duplicate(), - args, + args.map(_.duplicate()), body.duplicate(), location, passData.duplicate, diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/LambdaShorthandToLambda.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/LambdaShorthandToLambda.scala index 9e6885f023..2366fbeaac 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/LambdaShorthandToLambda.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/LambdaShorthandToLambda.scala @@ -65,15 +65,12 @@ case object LambdaShorthandToLambda extends IRPass { ir: IR.Module, moduleContext: ModuleContext ): IR.Module = - ir.transformExpressions { - case x => - x.mapExpressions( - runExpression( - _, - InlineContext(freshNameSupply = moduleContext.freshNameSupply) - ) - ) - } + ir.mapExpressions( + runExpression( + _, + InlineContext(freshNameSupply = moduleContext.freshNameSupply) + ) + ) /** Desugars underscore arguments to lambdas for an arbitrary expression. * diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/NestedPatternMatch.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/NestedPatternMatch.scala index d846a4454d..eb2925fcf5 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/NestedPatternMatch.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/NestedPatternMatch.scala @@ -118,9 +118,7 @@ case object NestedPatternMatch extends IRPass { ) ) - ir.transformExpressions { - case x => desugarExpression(x, freshNameSupply) - } + ir.mapExpressions(desugarExpression(_, freshNameSupply)) } /** Desugars nested pattern matches in an expression. diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/OperatorToFunction.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/OperatorToFunction.scala index a282657532..4f63a01286 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/OperatorToFunction.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/OperatorToFunction.scala @@ -43,9 +43,7 @@ case object OperatorToFunction extends IRPass { ir: IR.Module, moduleContext: ModuleContext ): IR.Module = - ir.transformExpressions({ - case x => runExpression(x, new InlineContext()) - }) + ir.mapExpressions(runExpression(_, new InlineContext())) /** Executes the conversion pass in an inline context. * diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/SectionsToBinOp.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/SectionsToBinOp.scala index 332de29df5..1d80ee3501 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/SectionsToBinOp.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/SectionsToBinOp.scala @@ -43,13 +43,13 @@ case object SectionsToBinOp extends IRPass { override def runModule( ir: IR.Module, moduleContext: ModuleContext - ): IR.Module = ir.transformExpressions { - case x => + ): IR.Module = + ir.mapExpressions( runExpression( - x, + _, new InlineContext(freshNameSupply = moduleContext.freshNameSupply) ) - } + ) /** Performs section to binary operator conversion on an IR expression. * diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/ShadowedPatternFields.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/ShadowedPatternFields.scala index 5845f316dd..9b5395ee57 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/ShadowedPatternFields.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/ShadowedPatternFields.scala @@ -56,9 +56,7 @@ case object ShadowedPatternFields extends IRPass { ir: IR.Module, @unused moduleContext: ModuleContext ): IR.Module = { - ir.transformExpressions { - case x => lintExpression(x) - } + ir.mapExpressions(lintExpression) } /** Lints for shadowed pattern fields. diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala index 89d091e1c8..387bc6535b 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala @@ -47,9 +47,7 @@ case object UnusedBindings extends IRPass { ir: IR.Module, moduleContext: ModuleContext ): IR.Module = - ir.transformExpressions { - case x => x.mapExpressions(runExpression(_, InlineContext())) - } + ir.mapExpressions(runExpression(_, InlineContext())) /** Lints an arbitrary expression. * @@ -97,10 +95,9 @@ case object UnusedBindings extends IRPass { val isUsed = aliasInfo.graph.linksFor(aliasInfo.id).nonEmpty if (!isIgnored && !isUsed) { - binding.copy( - expression = runExpression(binding.expression, context) - ) - binding.addDiagnostic(IR.Warning.Unused.Binding(binding.name)) + binding + .copy(expression = runExpression(binding.expression, context)) + .addDiagnostic(IR.Warning.Unused.Binding(binding.name)) } else { binding.copy( expression = runExpression(binding.expression, context) diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/ApplicationSaturation.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/ApplicationSaturation.scala index d627524d72..ce9db5321d 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/ApplicationSaturation.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/ApplicationSaturation.scala @@ -52,10 +52,9 @@ case object ApplicationSaturation extends IRPass { moduleContext: ModuleContext ): IR.Module = { val passConfig = moduleContext.passConfiguration - ir.transformExpressions({ - case x => - runExpression(x, new InlineContext(passConfiguration = passConfig)) - }) + ir.mapExpressions( + runExpression(_, new InlineContext(passConfiguration = passConfig)) + ) } /** Executes the analysis pass, marking functions with information about their diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/LambdaConsolidate.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/LambdaConsolidate.scala index eca3730793..8d8cec50d6 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/LambdaConsolidate.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/LambdaConsolidate.scala @@ -76,13 +76,12 @@ case object LambdaConsolidate extends IRPass { ir: IR.Module, moduleContext: ModuleContext ): IR.Module = - ir.transformExpressions { - case x => - runExpression( - x, - new InlineContext(freshNameSupply = moduleContext.freshNameSupply) - ) - } + ir.mapExpressions( + runExpression( + _, + new InlineContext(freshNameSupply = moduleContext.freshNameSupply) + ) + ) /** Performs lambda consolidation on an expression. * diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/UnreachableMatchBranches.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/UnreachableMatchBranches.scala index 762e4da8bd..57c3eb0a41 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/UnreachableMatchBranches.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/UnreachableMatchBranches.scala @@ -69,9 +69,7 @@ case object UnreachableMatchBranches extends IRPass { ir: IR.Module, @unused moduleContext: ModuleContext ): IR.Module = { - ir.transformExpressions { - case x => optimizeExpression(x) - } + ir.mapExpressions(optimizeExpression) } /** Runs unreachable branch optimisation on an expression. diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/IgnoredBindings.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/IgnoredBindings.scala index cf617c646b..04186a120b 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/IgnoredBindings.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/IgnoredBindings.scala @@ -58,15 +58,12 @@ case object IgnoredBindings extends IRPass { ir: IR.Module, moduleContext: ModuleContext ): IR.Module = - ir.transformExpressions { - case x => - x.mapExpressions( - runExpression( - _, - InlineContext(freshNameSupply = moduleContext.freshNameSupply) - ) - ) - } + ir.mapExpressions( + runExpression( + _, + InlineContext(freshNameSupply = moduleContext.freshNameSupply) + ) + ) /** Desugars ignored bindings for an arbitrary expression. * @@ -172,7 +169,8 @@ case object IgnoredBindings extends IRPass { } } - /** Generates a new argument name for `arg` if `isIgnored` is true. + /** Generates a new argument name for `arg` if `isIgnored` is true and updates + * all arguments metadata with their 'ignored' status. * * It also handles recursing through the default values. * @@ -187,6 +185,20 @@ case object IgnoredBindings extends IRPass { freshNameSupply: FreshNameSupply ): IR.DefinitionArgument = { arg match { + case spec @ IR.DefinitionArgument.Specified( + IR.Name.This(_, _, _), + _, + _, + _, + _, + _ + ) => + // Note [Ignored `this` Argument] + spec + .copy(defaultValue = + spec.defaultValue.map(resolveExpression(_, freshNameSupply)) + ) + .updateMetadata(this -->> State.Ignored) case spec: IR.DefinitionArgument.Specified => if (isIgnored) { val newName = freshNameSupply @@ -342,3 +354,11 @@ case object IgnoredBindings extends IRPass { } } } + +/* Note [Ignored `this` Argument] + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + * `this` is implicitly added to all methods in the GenerateMethodBodies pass. + * It may however not be used in the method body and this should not emit an + * unused warning. So when processing function arguments, `this` should be + * marked as ignored to avoid warnings from the UnusedBindings pass. + */ diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeFunctions.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeFunctions.scala index 87bca33efe..3356e47d5a 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeFunctions.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeFunctions.scala @@ -53,9 +53,8 @@ case object TypeFunctions extends IRPass { override def runModule( ir: IR.Module, @unused moduleContext: ModuleContext - ): IR.Module = ir.transformExpressions { - case a => resolveExpression(a) - } + ): IR.Module = + ir.mapExpressions(resolveExpression) /** Performs typing function resolution on an expression. * @@ -68,9 +67,10 @@ case object TypeFunctions extends IRPass { override def runExpression( ir: IR.Expression, @unused inlineContext: InlineContext - ): IR.Expression = ir.transformExpressions { - case a => resolveExpression(a) - } + ): IR.Expression = + ir.transformExpressions { + case a => resolveExpression(a) + } // === Pass Internals ======================================================= diff --git a/engine/runtime/src/test/scala/org/enso/compiler/test/pass/analyse/GatherDiagnosticsTest.scala b/engine/runtime/src/test/scala/org/enso/compiler/test/pass/analyse/GatherDiagnosticsTest.scala index bc4173da4b..33b8f05a06 100644 --- a/engine/runtime/src/test/scala/org/enso/compiler/test/pass/analyse/GatherDiagnosticsTest.scala +++ b/engine/runtime/src/test/scala/org/enso/compiler/test/pass/analyse/GatherDiagnosticsTest.scala @@ -117,8 +117,11 @@ class GatherDiagnosticsTest extends CompilerTest { val diagnostics = result .unsafeGetMetadata(GatherDiagnostics, "Impossible") .diagnostics - diagnostics should have length 1 - diagnostics.head.message shouldEqual "Unused variable unused." + diagnostics should have size 2 + diagnostics.map(_.message).toSet shouldEqual Set( + "Unused variable unused.", + "Unused function argument x." + ) } } } diff --git a/engine/runtime/src/test/scala/org/enso/compiler/test/pass/lint/UnusedBindingsTest.scala b/engine/runtime/src/test/scala/org/enso/compiler/test/pass/lint/UnusedBindingsTest.scala index 1b29963c7d..c4a12b29e2 100644 --- a/engine/runtime/src/test/scala/org/enso/compiler/test/pass/lint/UnusedBindingsTest.scala +++ b/engine/runtime/src/test/scala/org/enso/compiler/test/pass/lint/UnusedBindingsTest.scala @@ -1,7 +1,7 @@ package org.enso.compiler.test.pass.lint import org.enso.compiler.Passes -import org.enso.compiler.context.{FreshNameSupply, InlineContext} +import org.enso.compiler.context.{FreshNameSupply, InlineContext, ModuleContext} import org.enso.compiler.core.IR import org.enso.compiler.core.IR.Pattern import org.enso.compiler.pass.PassConfiguration._ @@ -11,8 +11,9 @@ import org.enso.compiler.pass.optimise.ApplicationSaturation import org.enso.compiler.pass.{IRPass, PassConfiguration, PassManager} import org.enso.compiler.test.CompilerTest import org.enso.interpreter.runtime.scope.LocalScope +import org.scalatest.Inside -class UnusedBindingsTest extends CompilerTest { +class UnusedBindingsTest extends CompilerTest with Inside { // === Test Setup =========================================================== @@ -57,6 +58,31 @@ class UnusedBindingsTest extends CompilerTest { ) } + /** Adds an extension method for running linting on the input IR. + * + * @param ir the IR to lint + */ + implicit class LintModule(ir: IR.Module) { + + /** Runs unused name linting on [[ir]]. + * + * @param moduleContext the inline context in which the desugaring takes + * place + * @return [[ir]], with all unused names linted + */ + def lint(implicit moduleContext: ModuleContext): IR.Module = { + UnusedBindings.runModule(ir, moduleContext) + } + } + + /** Makes a module context. + * + * @return a new inline context + */ + def mkModuleContext: ModuleContext = { + ModuleContext(freshNameSupply = Some(new FreshNameSupply)) + } + // === The Tests ============================================================ "Unused bindings linting" should { @@ -78,6 +104,31 @@ class UnusedBindingsTest extends CompilerTest { lintMeta.head.name.name shouldEqual "x" } + "attach a warning to an unused top-level function argument" in { + implicit val ctx: ModuleContext = mkModuleContext + + val ir = + """ + |f = x -> 10 + |main = + | f 0 + |""".stripMargin.preprocessModule.lint + + inside(ir.bindings.head) { + case definition: IR.Module.Scope.Definition.Method.Explicit => + inside(definition.body) { + case f: IR.Function.Lambda => + val lintMeta = f.arguments(1).diagnostics.collect { + case u: IR.Warning.Unused.FunctionArgument => u + } + + lintMeta should not be empty + lintMeta.head shouldBe an[IR.Warning.Unused.FunctionArgument] + lintMeta.head.name.name shouldEqual "x" + } + } + } + "not attach a warning to an unused function argument if it is an ignore" in { implicit val ctx: InlineContext = mkInlineContext diff --git a/lib/searcher/src/test/scala/org/enso/searcher/sql/SuggestionsRepoTest.scala b/lib/searcher/src/test/scala/org/enso/searcher/sql/SuggestionsRepoTest.scala index 46f412ac75..991d0cb2e9 100644 --- a/lib/searcher/src/test/scala/org/enso/searcher/sql/SuggestionsRepoTest.scala +++ b/lib/searcher/src/test/scala/org/enso/searcher/sql/SuggestionsRepoTest.scala @@ -1,6 +1,6 @@ package org.enso.searcher.sql -import org.enso.jsonrpc.test.RetrySpec +import org.enso.jsonrpc.test.FlakySpec import org.enso.searcher.Suggestion import org.scalatest.BeforeAndAfterAll import org.scalatest.matchers.should.Matchers @@ -13,7 +13,7 @@ import scala.concurrent.duration._ class SuggestionsRepoTest extends AnyWordSpec - with RetrySpec + with FlakySpec with Matchers with BeforeAndAfterAll { @@ -35,7 +35,7 @@ class SuggestionsRepoTest "SuggestionsDBIO" should { - "select suggestion by id" in { + "select suggestion by id" taggedAs Flaky in { val action = for { id <- db.run(repo.insert(suggestion.atom)) @@ -45,7 +45,7 @@ class SuggestionsRepoTest Await.result(action, Timeout) shouldEqual Some(suggestion.atom) } - "find suggestion by returnType" taggedAs Retry() in { + "find suggestion by returnType" taggedAs Flaky in { val action = for { _ <- db.run(repo.insert(suggestion.local))