When a pattern variable is defined on only one side of a disjunction,

it is now an error to use it again later in the pattern,
or in a value expression. The reason is that the semantics
are too confusing. If you get this error, just rename the
pattern variable to avoid the conflict, or use a wildcard
instead.

This is technically a breaking change, but I'm guessing
that it will occur only rarely in practice. Therefore I
am only bumping the minor version number.

Fixes #99

Project.toml

@@ -1,6 +1,6 @@
 name = "Match"
 uuid = "7eb4fadd-790c-5f42-8a69-bfa0b872bfbf"
-version = "2.0.1"
+version = "2.1.0"
 authors = ["Neal Gafter <neal@gafter.com>", "Kevin Squire <kevin.squire@gmail.com>"]
 
 [deps]

src/binding.jl

@@ -147,6 +147,8 @@ function is_possible_type_name(t::Expr)
         all(is_possible_type_name, t.args)
 end
 
+const unusable_variable = gensym("#a variable that was previously used on one side (only) of a disjunction");
+
 function bind_pattern!(
     location::LineNumberNode,
     source::Any,
@@ -184,6 +186,10 @@ function bind_pattern!(
         if haskey(assigned, varsymbol)
             # previously introduced variable.  Get the symbol holding its value
             var_value = assigned[varsymbol]
+            if var_value === unusable_variable
+                error("$(location.file):$(location.line): May not reuse variable name `$varsymbol` " *
+                    "after it has previously been used on only one side of a disjunction.")
+            end
             bound_expression = BoundExpression(
                 location, source, ImmutableDict{Symbol, Symbol}(varsymbol, var_value))
             pattern = BoundIsMatchTestPattern(
@@ -310,6 +316,9 @@ function bind_pattern!(
             v2 = assigned2[key]
             if v1 == v2
                 assigned = ImmutableDict{Symbol, Symbol}(assigned, key, v1)
+            elseif v1 === unusable_variable || v2 === unusable_variable
+                # A previously unusable variable remains unusable
+                assigned = ImmutableDict{Symbol, Symbol}(assigned, key, unusable_variable)
             else
                 # Every phi gets its own distinct variable.  That ensures we do not
                 # share them between patterns.
@@ -327,6 +336,16 @@ function bind_pattern!(
                 assigned = ImmutableDict{Symbol, Symbol}(assigned, key, temp)
             end
         end
+
+        # compute variables that were assigned on only one side of the disjunction and mark
+        # them (by using a designated value in `assigned`) so we can give an error message
+        # when a variable that is defined on only one side of a disjunction is used again
+        # later in the enclosing pattern.
+        one_only = setdiff(union(keys(assigned1), keys(assigned2)), both)
+        for key in one_only
+            assigned = ImmutableDict{Symbol, Symbol}(assigned, key, unusable_variable)
+        end
+
         pattern = BoundOrPattern(location, source, BoundPattern[bp1, bp2])
 
     elseif is_expr(source, :call, 3) && source.args[1] == :|
@@ -518,6 +537,11 @@ function bind_expression(location::LineNumberNode, expr, assigned::ImmutableDict
     for v in used
         tmp = get(assigned, v, nothing)
         @assert tmp !== nothing
+        if tmp === unusable_variable
+            # The user is attempting to use a variable that was defined on only
+            # one side of a disjunction.  That is an error.
+            error("$(location.file):$(location.line): The pattern variable `$v` cannot be used because it was defined on only one side of a disjunction.")
+        end
         push!(assignments.args, Expr(:(=), v, tmp))
         new_assigned = ImmutableDict(new_assigned, v => tmp)
     end

src/bound_pattern.jl

@@ -56,6 +56,9 @@ struct BoundExpression
     function BoundExpression(location::LineNumberNode,
         source::Any,
         assignments::ImmutableDict{Symbol, Symbol} = ImmutableDict{Symbol, Symbol}())
+        for (k, v) in assignments
+            @assert v !== unusable_variable
+        end
         new(location, source, assignments)
     end
 end

src/lowering.jl

@@ -6,7 +6,7 @@ pattern_matches_value(r::Regex, s::AbstractString) = occursin(r, s)
 
 function assignments(assigned::ImmutableDict{Symbol, Symbol})
     # produce a list of assignments to be splatted into the caller
-    return (:($patvar = $resultsym) for (patvar, resultsym) in assigned)
+    return (:($patvar = $resultsym) for (patvar, resultsym) in assigned if resultsym !== unusable_variable)
 end
 
 function code(e::BoundExpression)

test/matchtests.jl

@@ -330,4 +330,84 @@ end
     @test test_interp([1, 2]) == :(1 + 2)
 end
 
+@testset "handling of variables defined on one side (only) of a disjunction 1" begin
+
+    # It is an error to use a variable name after it has previously been used in the
+    # enclosing pattern on one side (only) of a disjunction.  If you run into this error
+    # you must rename the second variable to use a distinct name so that it is clear
+    # that it is not a reference to the previously named variable.
+
+    let line = (@__LINE__) + 3, file=(@__FILE__)
+        try
+            @eval @match (2, 1) begin
+                ((1|y), y) => true
+                _ => false
+            end
+            @test false
+        catch ex
+            @test ex isa LoadError
+            e = ex.error
+            @test e isa ErrorException
+            @test e.msg == "$file:$line: May not reuse variable name `y` after it has previously been used on only one side of a disjunction."
+        end
+    end
+
+end
+
+@testset "handling of variables defined on one side (only) of a disjunction 2" begin
+
+    # It is an error to use a variable name after it has previously been used in the
+    # enclosing pattern on one side (only) of a disjunction.  If you run into this error
+    # you must rename the second variable to use a distinct name so that it is clear
+    # that it is not a reference to the previously named variable.
+
+    let line = (@__LINE__) + 3, file=(@__FILE__)
+        try
+            @eval @match (2, 1) begin
+                ((1|y|y), y) => true
+                _ => false
+            end
+            @test false
+        catch ex
+            @test ex isa LoadError
+            e = ex.error
+            @test e isa ErrorException
+            @test e.msg == "$file:$line: May not reuse variable name `y` after it has previously been used on only one side of a disjunction."
+        end
+    end
+
+end
+
+@testset "handling of variables defined on one side (only) of a disjunction 3" begin
+
+    # It is an error to use a variable name after it has previously been used in the
+    # enclosing pattern on one side (only) of a disjunction.  If you run into this error
+    # you must rename the second variable to use a distinct name so that it is clear
+    # that it is not a reference to the previously named variable.
+
+    let line = (@__LINE__) + 3, file=(@__FILE__)
+        try
+            @eval @match 1 begin
+                1|y => y
+                _ => false
+            end
+            @test false
+        catch ex
+            @test ex isa LoadError
+            e = ex.error
+            @test e isa ErrorException
+            @test e.msg == "$file:$line: The pattern variable `y` cannot be used because it was defined on only one side of a disjunction."
+        end
+    end
+
+end
+
+@testset "handling of variables defined on one side (only) of a disjunction 4" begin
+
+    @test @match 1 begin
+        1|y => true
+    end
+
+end
+
 end

test/rematch.jl

@@ -371,10 +371,13 @@ end
         (1, a && (1,b)) => (a,b)
     end) == ((2,3),3)
 
-    # only vars that exist in all branches can be accessed
-    @test_throws UndefVarError(:y) @match (1,(2,3)) begin
-      (1, (x,:nope) || (2,y)) => y
-    end
+    # Only pattern variables that exist in all branches can be accessed.
+    # This is now a bind-time error. A test for that is in
+    # matchtests.jl.
+    #
+    # @test_throws UndefVarError(:y) @match (1,(2,3)) begin
+    #   (1, (x,:nope) || (2,y)) => y
+    # end
 end
 
 @testset "Splats" begin