Skip to content

Fixes #10749, feature/pattern variable shadow warning#25196

Open
Bbn08 wants to merge 1 commit intoscala:mainfrom
Bbn08:feature/pattern-variable-shadow-warning
Open

Fixes #10749, feature/pattern variable shadow warning#25196
Bbn08 wants to merge 1 commit intoscala:mainfrom
Bbn08:feature/pattern-variable-shadow-warning

Conversation

@Bbn08
Copy link
Contributor

@Bbn08 Bbn08 commented Feb 5, 2026

Problem

When a pattern-bound variable has the same name as a variable in an enclosing scope, the pattern variable silently shadows the outer one. This can lead to subtle bugs:

scala
def match1(x: Any, y: Any) = x match {
  case y => 1  // User may expect this to match against `y`, but it creates a new binding
}

Solution

Add a new warning option -Wshadow:pattern-variable-shadow that detects when a pattern variable shadows an existing term in the enclosing scope.

Implementation

  1. ScalaSettings.scala: Added pattern-variable-shadow to the -Wshadow multi-choice setting
  2. Typer.scala: Added shadow detection in typedBind() that traverses outer scopes to check for existing terms with the same name

Warning Output

-- Warning: file.scala:3:10 --
3 |    case y => 1
  |         ^
  |         pattern variable y shadows value y in method match1

Test Coverage

Added tests/warn/i10749.scala with 8 test cases covering:

  • Local val shadowing
  • Parameter shadowing
  • Nested pattern shadowing
  • Tuple patterns (multiple warnings)
  • For-comprehension patterns
  • Wildcard patterns (no warning)
  • Fresh variables (no warning)
  • Stable identifiers with backticks (no warning)

Usage

bash
scalac -Wshadow:pattern-variable-shadow file.scala
# Also enabled by -Wshadow:all or -Wall

@Bbn08 Bbn08 marked this pull request as draft February 5, 2026 13:11
@Bbn08 Bbn08 marked this pull request as ready for review February 5, 2026 14:19
@Bbn08 Bbn08 marked this pull request as draft February 5, 2026 14:20
@som-snytt
Copy link
Contributor

There is a general directive that lints should not infect typer. They should have a mini-phase like current shadowing lint.

@Bbn08
Copy link
Contributor Author

Bbn08 commented Feb 5, 2026

@som-snytt Thank you for the review and the clarification!

I understand, I should move the pattern variable shadow check out of Typer and into a dedicated MiniPhase, similar to how CheckShadowing works currently. I will refactor the PR to implement this as a separate phase to avoid complicating the main type checker.

I'll start working on that refactor now.

@Bbn08 Bbn08 marked this pull request as ready for review February 5, 2026 19:49
@Bbn08 Bbn08 marked this pull request as draft February 5, 2026 19:49
@Bbn08 Bbn08 force-pushed the feature/pattern-variable-shadow-warning branch 2 times, most recently from 36fb276 to 070d4f9 Compare February 8, 2026 15:00
@Bbn08
Copy link
Contributor Author

Bbn08 commented Feb 8, 2026

Hi @som-snytt, thank you again for the review feedback.

I've refactored the implementation to move the pattern variable shadow check out of Typer.scala and into the existing CheckShadowing MiniPhase. This follows the compiler's design principle of keeping linting logic separate from core type checking.

Summary of Changes

1. CheckShadowing.scala

  • Added PatternShadowWarning case class to represent pattern variable shadow warnings
  • Added checkPatternVariableShadow() method with three lookup strategies:

1. lookForImportedShadowedTerm() — checks imported terms
2. lookForLocalShadowedTerm() — checks local variables in outer scopes
3. lookForUnitShadowedTerm() — checks compilation unit scope

  • Integrated into transformBind() to catch pattern bindings during the MiniPhase traversal
  • Respects the -Wshadow:pattern-variable-shadow flag

2. ScalaSettings.scala

Added pattern-variable-shadow as a new choice in the -Wshadow multi-choice setting

3. Typer.scala

Removed the shadow checking logic that was previously in typedBind()

4. Tests (tests/warn/i10749.scala)

  • Added 8 test cases covering:

    • Local val shadowing
    • Parameter shadowing
    • Nested pattern shadowing
    • Tuple patterns (multiple warnings)
    • For-comprehension patterns
    • Wildcard patterns (no warning expected)
    • Fresh variables (no warning expected)
    • Stable identifiers with backticks (no warning expected)

If there's anything else you'd like me to work on or improve, please feel free to let me know as I'm happy to make any additional changes :)

@Bbn08 Bbn08 marked this pull request as ready for review February 8, 2026 17:07
@Bbn08 Bbn08 marked this pull request as draft February 9, 2026 08:26
@Bbn08 Bbn08 marked this pull request as ready for review February 9, 2026 11:29
@Gedochao Gedochao requested review from som-snytt and tgodzik February 9, 2026 13:30
@som-snytt
Copy link
Contributor

I'll review shortly, when the other shadowing PR is merged.

A quick note, the shadowing code uses a custom model for scopes. The "unused import" used to do that, but was changed for reasons of correctness and performance.

The Scala 2 was scala/scala#8806 which is now ancient history, but maybe the feature is the same.

@Bbn08
Copy link
Contributor Author

Bbn08 commented Feb 14, 2026

Noted.

@Bbn08 Bbn08 force-pushed the feature/pattern-variable-shadow-warning branch from 614a843 to 60c24ad Compare February 19, 2026 13:44
@Bbn08
Copy link
Contributor Author

Bbn08 commented Feb 19, 2026

Rebased on top of #25101 and adapted the pattern variable shadow warning to the new unified reporting structure. Here's what changed:

Removed the custom case-class hierarchy (PrivateShadowWarning, TypeParamShadowWarning, PatternShadowWarning, ShadowResult) from the ShadowingData companion object these are no longer needed since #25101 introduced ShadowWarning(pos, msg: Message).

  • Added PatternVariableShadowsType as a new structured Message in reporting/messages.scala (with PatternVariableShadowsTypeID = E228), following the same pattern as PrivateShadowsType and TypeParameterShadowsType.

  • Updated CheckShadowing.scala to use the new ShadowWarning + Message pattern:
    - reportShadowing now takes List[ShadowWarning] and reports w.msg directly (no more pattern matching on warning subtypes)
    - patternShadowWarnings uses MutSet[ShadowWarning] instead of the old MutSet[PatternShadowWarning]
    - Pattern variable warnings are created as ShadowWarning(pos, PatternVariableShadowsType(sym, shadowed))

  • Updated the check file (i10749.check) to reflect the new output format with error message IDs ([E228]) and the longer explanation available when compiling with -explain line.

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.

3 participants

Comments