Warn-unused uses type alias in nesting check by som-snytt · Pull Request #6190 · scala/scala

som-snytt · 2017-11-22T21:33:08Z

A reference from the body of a class is not a usage
of the class, but a reference to a local type alias of it
is a usage of the type alias.

Also don't warn about filter for refutable pattern.

Also classOf[C] is a usage of C.

Simplify message for a var that isn't mutated.

Fixes scala/bug#10623
Fixes scala/bug#10394
Fixes scala/bug#9058
Fixes scala/bug#10282
Fixes scala/bug#10448

som-snytt · 2017-11-23T19:43:58Z

src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala

Is this the best one can do?

tp.typeSymbolDirect.isAliasType?

That pulls in too much, it turns out. I tried various isNonClassType etc, so mostly I don't actually understand the API.

That pulls in too much

Can you give an example?

In constructors you see:

[log typer] ALIAS C.super.type/class scala.reflect.internal.Types$UniqueSuperType/class C/type AnyRef primary constructor C [log typer] C.super.type referenced from primary constructor C

with the false negative for object X { private class C }. So the AnyRef is the Direct.isAliasType, defined as an alias of Object.

I guess it's sufficient to also check isLocal || isPrivate.

A reference from the body of a class is not a usage of the class, but a reference to a local type alias of it is a usage of the type alias.

The call has the form, `qual.withFilter(check$refutable => body)` where the body is `{ case mypat => true }` which must always spuriously warn about any pattern variables.

Notice ConstantType and record it under -Ywarn-unused.

Although vars have setters, it's more uniform to report that the private var is not updated. (It's not possible that the setter is overriding a synthetic setter for a var, so there can be no ambiguity.)

Incremental compilation while erroring can result in unpositioned trees. Parser preserves the escape hatch attachment under patdef transform. Both casedef and valdef can test immediately if escape hatch was requested. Add attachment for valdefs resulting from patvardefs. When checking for redundant unused setters, try to compare using accessed, otherwise compare names.

SethTisue · 2018-02-02T21:09:36Z

this seems to have been forgotten about. can you suggest a reviewer?

som-snytt · 2018-02-02T21:20:42Z

@SethTisue I believe the community that wants the fix will step up to review it! I believe in the community!

hrhino

Just because you asked.

It might be cool to add the ticket references in somewhere for future retrospective capabilities.

hrhino · 2018-02-02T21:42:21Z

src/reflect/scala/reflect/internal/TreeGen.scala

I assume this is UNIX for copy? It seems strange that cpAtBoundAttachment only adds the attachment if from has it but cpPatVarDefAttachments unconditionally adds PatVarDefAttachment.

One propagates whether the syntax was x @ _, the other adds that this is a patvar. There is doc over in TypeDiag.

I would justify cp by persistence of mk for make and similar, which are not due to a subculture. I do resist long names, and these are long.

I think I will shave my beard now and maybe go get a hair cut.

Don't add attachments when not warning. Avoid warning when already in error.

SethTisue

this all looks plausible to me, but maybe one more pair of eyes on the attachments stuff.

SethTisue · 2018-02-16T01:44:56Z

src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala

 import scala.util.control.Exception.ultimately
 import symtab.Flags._
-import PartialFunction._
+import PartialFunction.{condOpt => whenever}


Seth's nit: I know, condOpt isn't the greatest name, but it's the name we have, I don't think we should indulge ourselves in arbitrary local renamings like this

I know what you mean. Paulp once said those methods where the most underlooked of his contributions, and I think it's partly the names. Maybe we can fix that.

som-snytt · 2018-02-16T03:12:57Z

I need another session to exercise the last commit. While debugging the underlying issue, I realized I know nothing.

lrytz

🏅

hrhino · 2018-02-22T15:42:50Z

And we're back under 2,100 issues in scala/bug!

lrytz · 2018-02-22T15:56:15Z

🙀

som-snytt · 2018-02-22T17:42:34Z

I'll create a few new issues to address @SethTisue 's nits. Is there a "nit" label?

SethTisue · 2018-02-22T17:58:08Z

very glad to see this crossing the finish line! 🎉 thanks Som, greatly appreciate your ongoing commitment to this unused warnings work, it's a real boon for code quality community-wide

som-snytt · 2018-02-22T18:14:39Z

I won't rest until all the code is used.

SethTisue · 2018-02-22T18:25:44Z

no child (in any abstract syntax tree) left behind

som-snytt · 2018-02-22T18:29:40Z

"I speak for the trees." - The Lorax.

adriaanm · 2018-03-08T10:27:49Z

Looks like this regressed for a local val captured in a CBN closure:

class Test {
  def timed[T](body: => T) = body

  def run(): Boolean = {
    val grouped = List(("bla","bla"))


    val (_, _) = timed {
      for ((kind, paths) <- grouped) {
        println("")
      }
      (1, 2)
    }
    
    false
  }
}

nok:

➜  Desktop sbt 'set resolvers += "pr-integration" at "https://scala-ci.typesafe.com/artifactory/scala-integration"' 'set scalaVersion := "2.12.5-bin-56918e4"' 'set scalacOptions ++= Seq("-Xfatal-warnings", "-Xlint:unused")' compile
[info] Compiling 1 Scala source to /Users/adriaan/Desktop/target/scala-2.12/classes...
[error] /Users/adriaan/Desktop/unused_captured.scala:5: local val grouped in method run is never used
[error]     val grouped = List(("bla","bla"))
[error]         ^
[error] one error found
[error] (compile:compileIncremental) Compilation failed
[error] Total time: 2 s, completed Mar 8, 2018 11:25:25 AM

ok:

➜  Desktop sbt 'set resolvers += "pr-integration" at "https://scala-ci.typesafe.com/artifactory/scala-integration"' 'set scalaVersion := "2.12.5-bin-f78f517"' 'set scalacOptions ++= Seq("-Xfatal-warnings", "-Xlint:unused")' compile
[info] Compiling 1 Scala source to /Users/adriaan/Desktop/target/scala-2.12/classes...
[success] Total time: 3 s, completed Mar 8, 2018 11:25:49 AM

fommil · 2018-03-20T23:19:30Z

thanks @som-snytt ... ~~I'm still getting false positives in synthetic methods. I don't know how to create a minimisation that doesn't depend on the scalaz-deriving compiler plugin.~~

(Actually this might have been a bug in my compiler plugin...)

* split the Conseil configurations * use typed objects in PlatformDiscovery and separate from the confguration file parsing * use imports to get config readers into scope * create custom readers for platforms * wrap lightbend config for the host pool in a configuration class * fix residuals compilation errors from the merge * upgrade pureconfig version, improving the available api * upgrade scala version to latest, which fix scala/scala#6190

scala-jenkins added this to the 2.12.5 milestone Nov 22, 2017

som-snytt closed this Nov 23, 2017

som-snytt reopened this Nov 23, 2017

som-snytt force-pushed the issue/10623 branch from 0c2c7ec to c16f1ce Compare November 23, 2017 19:40

som-snytt commented Nov 23, 2017

View reviewed changes

Warn-unused uses type alias in nesting check

f223a7e

A reference from the body of a class is not a usage of the class, but a reference to a local type alias of it is a usage of the type alias.

som-snytt force-pushed the issue/10623 branch from c16f1ce to f223a7e Compare November 24, 2017 23:18

som-snytt added 2 commits December 1, 2017 14:34

-Ywarn-unused ignores filter of refutable patterns

3e28d97

The call has the form, `qual.withFilter(check$refutable => body)` where the body is `{ case mypat => true }` which must always spuriously warn about any pattern variables.

Class literal is a usage

bf61c1b

Notice ConstantType and record it under -Ywarn-unused.

som-snytt mentioned this pull request Dec 1, 2017

[SUPERSEDED] Class literal is a usage #6204

Closed

som-snytt added 2 commits December 1, 2017 20:32

Simplify warning for unset private var

0d352b3

Although vars have setters, it's more uniform to report that the private var is not updated. (It's not possible that the setter is overriding a synthetic setter for a var, so there can be no ambiguity.)

hrhino approved these changes Feb 2, 2018

View reviewed changes

Restrict attachments for warnings

a05cd47

Don't add attachments when not warning. Avoid warning when already in error.

SethTisue approved these changes Feb 16, 2018

View reviewed changes

som-snytt mentioned this pull request Feb 16, 2018

Compare getter and setter by .accessed instead of .point. Fixes #10448. #6232

Closed

Test status quo nowarn for vanishing local

149b660

lrytz approved these changes Feb 22, 2018

View reviewed changes

lrytz merged commit 56918e4 into scala:2.12.x Feb 22, 2018

SethTisue added the release-notes worth highlighting in next release notes label Feb 22, 2018

som-snytt deleted the issue/10623 branch February 22, 2018 17:42

adriaanm mentioned this pull request Mar 8, 2018

warn unused regression involving refutability check in for comprehension scala/bug#10763

Closed

lrytz added 2.12 and removed 2.12 labels Mar 14, 2018

SethTisue added release-notes worth highlighting in next release notes and removed release-notes worth highlighting in next release notes labels Mar 15, 2018

ivanopagano mentioned this pull request Dec 27, 2018

Conseil-103: continued work on avoiding partial functions Cryptonomic/Conseil#173

Merged

som-snytt mentioned this pull request Apr 10, 2019

Regression: false warning about non-used variable scala/bug#11478

Closed

renovate bot mentioned this pull request Jun 27, 2021

chore(deps): update dependency scala to v2.12.14 w0rxbend/kzonix-opencv-app#6

Merged

1 task

renovate bot mentioned this pull request Aug 11, 2021

Update dependency scala to v2.12.14 - autoclosed bgoonz/The-Algorithms#7

Closed

1 task

Conversation

som-snytt commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SethTisue commented Feb 2, 2018

Uh oh!

som-snytt commented Feb 2, 2018

Uh oh!

hrhino left a comment

Choose a reason for hiding this comment

Uh oh!

hrhino Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SethTisue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

som-snytt commented Feb 16, 2018

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

hrhino commented Feb 22, 2018

Uh oh!

lrytz commented Feb 22, 2018

Uh oh!

som-snytt commented Feb 22, 2018

Uh oh!

SethTisue commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Feb 22, 2018

Uh oh!

SethTisue commented Feb 22, 2018

Uh oh!

som-snytt commented Feb 22, 2018

Uh oh!

adriaanm commented Mar 8, 2018

Uh oh!

fommil commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments

som-snytt commented Nov 22, 2017 •

edited

Loading

hrhino Feb 2, 2018 •

edited

Loading

SethTisue commented Feb 22, 2018 •

edited

Loading

fommil commented Mar 20, 2018 •

edited

Loading