Add -Xlint:pattern-shadow to lint pattern varids which are backquotable

[som-snytt]

Lint -Xlint:pattern-shadow warns when a pattern has a variable that may have been intended to match a value in scope using "backtick" notation.

def compare(x: Any, y: Any) = x match { case y => true }  // intended case `y` =>

For the common idiom of "refining" a value through pattern matching, there is no warning if the value in scope is used in the selector expression:

def asString(x: Any) = x match { case x: String => x case _ => stringOf(x) }

def f(x: X) = x.alias match { case x => } // does not check that x in pattern conforms to the input parameter

def f(x: X) = g(x) match { case x => } // arbitrary functions in x may extract arbitrary values without incurring a warning

This exception is permissive. For example, tuple arguments need not "align" in the pattern:

def swap(x: Any, y: Any) = (x, y) match { case (y, x) => }

Fixes scala/bug#11850

[som-snytt]

Addresses scalapuzzlers/scalapuzzlers.github.com#154

[som-snytt]

Also patdefs, cf scala/bug#7530

[som-snytt]

Clunky lint would be nicer as lightweight 3rd party linter and lint suppression via -Wconf and @nowarn.

[som-snytt]

Was I supposed to revisit this? Summer '20 was too early in pandemic to be morbidly depressed. My system of adding annotations in the PR title must have broken down.

[lrytz]

This looks useful - did you try it on the compiler / library to see how often it fires?

[lrytz]

Why the NoWarn filter?

[som-snytt]

All questions will be answered after I finish linting "literal boolean args should be named". That will be after I have coffee.

[som-snytt]

The attachment represents user code

case x @ _ =>

for which we don't want to propose

case `x` @ _ =>

Backquotes in that case is useful for

case `type` @ _ =>

[som-snytt]

I have a second branch where I may have fixed some project source, because this feels familiar:

    def mkAnnotation(tree: Tree): Tree = tree match {
      case SyntacticNew(Nil, SyntacticApplied(SyntacticAppliedType(_, _), _) :: Nil, noSelfType, Nil) =>
        tree

where noSelfType is not

Name noSelfType is already introduced in an enclosing scope as object noSelfType in trait Trees. Did you intend to match it using backquoted `noSelfType`?

Why doesn't this come up as an obvious reason not to name all values with lower case?

The other interesting aspect is that the var is unused on the RHS. I may have already considered that as a criterion: surely if I use the var, I intended to use it? unless I unintended the other element.

[som-snytt]

Forgot to restarr locally before pushing, d'oh. This day is already much too long, there will be more days. I almost wrote morose days. I noticed this PR began on my birthday, two days before lockdown. An age ago, so to speak.

Edit: I recognize self alias not ignored, so maybe I was in the middle of mitigating the result on the project at some point.

Also (x, y) match { case (x, y) => }, maybe it's feasible to align the names and not warn. Similarly case (Some(x), Some(y)).

[som-snytt]

20 17 fewer files in the diff. Subproject tweaks are split out again. Accounting for 3 junit files that were overlooked.

[som-snytt]

Now I want it to see that ValEq is an extractor with an apply, and use param names from the apply, but I will not add that tonight.

[lrytz]

I noticed that it's annoying to say "shadowed by value x", it should at least give a line number

👍 and / or tell the owner even if x is a local.

[som-snytt]

I think it would be nice to say parameter x when it is a parameter. The table in Symbols distinguishes the hard-core name for a thing from its pop name. But the breaking tests include ambiguous implicit, where you only care about the types, but to say parameter foo and value bar makes it look like "storage class" is significant. I will reconsider after looking at messages in Scala 3.

Otherwise, just add line number for locals and parameters. Hopefully, user can locate members.

Note that is not necessary to say value x, as we know it is a term (to be a candidate for the pattern) and we know its name, which was already stated. Finally, I restored the phrase because you get object x and also if you have debug turned up, you get -uniqid, which might be necessary to disambiguate some edge case.

[som-snytt]

My phrase for "shows up in scrutinee" is "implicated in selector expression".

I took your suggestion and scrapped the restrictions around the selector at the cost of more false negatives.

This is clearly wrong:

def f(x: Any) = x.toString match { case x => }

The previous enforcement was that the result of x.f must conform to x to intend "don't warn, I'm just refining x". I could still add that back.

I tried adding a type condition on the pat var and the shadowee, but the typical challenge is

def f(x: Option[X]) = x match { case Some(x) => }

Maybe it's enough to note type of x in the wrapped type. That is, like the current tweak, it's enough if it appears somewhere, and we won't be picky about the shape. Really we're saying, x should have some type relationship to what it shadows, if not conforming then at least embedding.

[som-snytt]

@lrytz that was my last swing for now. You may not like that I chose not to clutter up the help text. Currently, the lint passes the "no-brainer" test (not noisy and helpful if it warns).

Since everyone asks about warnings on Scala 2 vs Scala 3, not to mention compiler options, what is needed is that doc, which would explain the options operationally.

[lrytz]

Great, thank you for going through the many iterations!