Skip to content

Tighter condition for nowarn a single patvar #11048

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

Merged
merged 1 commit into from
Apr 22, 2025

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Apr 17, 2025

Address the question how liberal the exclusion for unused patvars should be.

This commit proposes: a single patvar named after the selector is always OK.

That is analogous to "narrow a var via a type pattern", (x: Option[_]) match { case x: Some[_] => }.

This "ergonomics" respects "cut and paste" where patterns that don't have a "canonical" patvar name may represent "extract a canonically narrower value". It still requires the patvar name to "align" with the selector name.

person match {
  case PreferredName(person) =>
  case _ => // no preference
}

The patvar is unused, but the match expression is allowed as something cut and pasted, so it's annoying to require "fixing up" just to pass a lint. The previous allowance is for case class element names, case Person(firstName, lastName) =>.

At a case, it's not necessary to walk the pattern twice: it will process Apply before recursing into children, so child Bind will see the attachment.

Fixes scala/scala-dev#902

@scala-jenkins scala-jenkins added this to the 2.13.17 milestone Apr 17, 2025
@som-snytt
Copy link
Contributor Author

The other lesson is to pay better attention when lrytz raises his eyebrows during code review.

@som-snytt som-snytt force-pushed the revert/allow-unused-patvar branch from 4a9b358 to 5eef9f3 Compare April 17, 2025 17:04
@som-snytt som-snytt marked this pull request as ready for review April 17, 2025 17:05
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! makes sense.

@@ -632,7 +632,8 @@ trait Trees extends api.Trees {
case class UnApply(fun: Tree, args: List[Tree])
extends TermTree with UnApplyApi {
override def transform(transformer: Transformer): Tree =
transformer.treeCopy.UnApply(this, transformer.transform(fun), transformer.transformTrees(args)) // bq: see test/.../unapplyContexts2.scala
transformer.treeCopy.UnApply(this, transformer.transform(fun), transformer.transformTrees(args))
// bq: see test/.../unapplyContexts2.scala
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, i think bq is @burakemir :)

@lrytz lrytz merged commit cdb1a1d into scala:2.13.x Apr 22, 2025
3 checks passed
@som-snytt som-snytt deleted the revert/allow-unused-patvar branch April 22, 2025 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.13.17 regression (?): Scalafix failing in community build
3 participants