From f0420330a722ab7aad568a4714b65eaae5ec4fbf Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Fri, 15 Dec 2023 15:43:05 -0800 Subject: [PATCH] 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 | 2 +- src/binding.jl | 24 +++++++++++++ src/bound_pattern.jl | 3 ++ src/lowering.jl | 2 +- test/matchtests.jl | 80 ++++++++++++++++++++++++++++++++++++++++++++ test/rematch.jl | 11 +++--- 6 files changed, 116 insertions(+), 6 deletions(-) diff --git a/Project.toml b/Project.toml index d5e4d80..8c043fb 100644 --- a/Project.toml +++ b/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 ", "Kevin Squire "] [deps] diff --git a/src/binding.jl b/src/binding.jl index bc6f9a0..2ada559 100644 --- a/src/binding.jl +++ b/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 diff --git a/src/bound_pattern.jl b/src/bound_pattern.jl index f6f06d0..2e59388 100644 --- a/src/bound_pattern.jl +++ b/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 diff --git a/src/lowering.jl b/src/lowering.jl index 8584c48..c2335fe 100644 --- a/src/lowering.jl +++ b/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) diff --git a/test/matchtests.jl b/test/matchtests.jl index 24d7a38..566bb25 100644 --- a/test/matchtests.jl +++ b/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 diff --git a/test/rematch.jl b/test/rematch.jl index 9d35003..1a2e182 100644 --- a/test/rematch.jl +++ b/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