From 4bc64dfbc28b510f9a925df1e7006c520bbf3e3c Mon Sep 17 00:00:00 2001 From: Billy Lanchantin Date: Wed, 22 May 2024 17:57:37 -0400 Subject: [PATCH] Prep for 0.5 release (#5) * try to use a more consistent comment style * try to support more complex exprs these are weird uses, but i'm not sure i want to require < to be at the root of the ast just yet * improve documentation * bump ex_doc * bump project version * changelog entry * use fully qualified name for linking * rewrite the whole thing to cover an edge case * work with `===` and `!==` * rename file * weird typo * test strict operators * refactor tests a bit more * two versions of the mixed operator test * error message should include strict operators * revert to 0.5 -- too many changes * update version in README * fix typo * update warning text * move more code to runtime * readme tweaks * ...changes? * various error message and docs improvements * change date to today (optimistic...) * give actions a try! * oh i don't think i needed to name the branch --- .github/workflows/elixir.yaml | 85 +++++ CHANGELOG.md | 14 + README.md | 176 ++++++++--- lib/compare_chain.ex | 444 +++++++++++++-------------- lib/compare_chain/core.ex | 39 +++ lib/compare_chain/default_compare.ex | 44 --- lib/compare_chain/error_message.ex | 53 +++- mix.exs | 4 +- mix.lock | 12 +- test/compare_chain_test.exs | 173 +++++++---- test/compile_time_test.exs | 44 ++- 11 files changed, 696 insertions(+), 392 deletions(-) create mode 100644 .github/workflows/elixir.yaml create mode 100644 lib/compare_chain/core.ex delete mode 100644 lib/compare_chain/default_compare.ex diff --git a/.github/workflows/elixir.yaml b/.github/workflows/elixir.yaml new file mode 100644 index 0000000..df4f012 --- /dev/null +++ b/.github/workflows/elixir.yaml @@ -0,0 +1,85 @@ +# Adapted from: https://fly.io/phoenix-files/github-actions-for-elixir-ci/ +name: Elixir CI + +# Defines when the workflow runs +on: + push: + branches: ["main"] + pull_request: + branches: ["main"] + +# Sets the ENV `MIX_ENV` to `test` for running tests +env: + MIX_ENV: test + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + name: Test on OTP ${{matrix.otp}} / Elixir ${{matrix.elixir}} + strategy: + # Specify the OTP and Elixir versions to use when building + # and running the workflow steps. + matrix: + otp: ["25.0.4"] + elixir: ["1.14.1"] + steps: + # Step: Setup Elixir + Erlang image as the base. + - name: Set up Elixir + uses: erlef/setup-beam@v1 + with: + otp-version: ${{matrix.otp}} + elixir-version: ${{matrix.elixir}} + + # Step: Check out the code. + - name: Checkout code + uses: actions/checkout@v3 + + # Step: Define how to cache deps. Restores existing cache if present. + - name: Cache deps + id: cache-deps + uses: actions/cache@v3 + env: + cache-name: cache-elixir-deps + with: + path: deps + key: ${{ runner.os }}-mix-${{ env.cache-name }}-${{ hashFiles('**/mix.lock') }} + restore-keys: | + ${{ runner.os }}-mix-${{ env.cache-name }}- + + # Step: Define how to cache the `_build` directory. After the first run, + # this speeds up tests runs a lot. This includes not re-compiling our + # project's downloaded deps every run. + - name: Cache compiled build + id: cache-build + uses: actions/cache@v3 + env: + cache-name: cache-compiled-build + with: + path: _build + key: ${{ runner.os }}-mix-${{ env.cache-name }}-${{ hashFiles('**/mix.lock') }} + restore-keys: | + ${{ runner.os }}-mix-${{ env.cache-name }}- + ${{ runner.os }}-mix- + + # Step: Download project dependencies. If unchanged, uses + # the cached version. + - name: Install dependencies + run: mix deps.get + + # Step: Compile the project treating any warnings as errors. + # Customize this step if a different behavior is desired. + - name: Compiles without warnings + run: mix compile --warnings-as-errors + + # Step: Check that the checked in code has already been formatted. + # This step fails if something was found unformatted. + # Customize this step as desired. + - name: Check Formatting + run: mix format --check-formatted + + # Step: Execute the tests. + - name: Run tests + run: mix test diff --git a/CHANGELOG.md b/CHANGELOG.md index ed42e39..48b0765 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # CHANGELOG +## v0.5.0 (2024-05-22) + + * Allow `===` and `!===` in expressions + * Improve documentation + * Fix bug with certain operation chains: + ``` + compare?(1 < 2 != 3 < 4) #=> true + compare?(1 < 2 != 3 > 4) #=> true (wrong!) + ``` + * Improve error message and documentation for invalid expressions + * [BREAKING] all branches of `and`, `or` and `not` must now contain comparisons + * Example: `compare?(1 < 2 and true)` used to be ok but is no longer + allowed because the right argument to `and` doesn't contain a comparison. + ## v0.4.0 (2023-09-10) * Warn when `compare?/1` is used on a struct diff --git a/README.md b/README.md index 3e5f608..4a36b0a 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ Provides convenience macros for comparisons which do: * chained comparisons like `a < b < c` - * semantic comparisons using the structural operators `<`, `>`, `<=`, `>=`, `==`, and `!=` + * semantic comparisons using the structural operators: `<`, `>`, `<=`, `>=`, `==`, `!=`, `===`, and `!==` * combinations using `and`, `or`, and `not` ### Examples @@ -21,6 +21,10 @@ true iex> compare?(~D[2017-03-31] < ~D[2017-04-01], Date) true +# Chained, semantic comparisons +iex> compare?(~D[2017-03-31] < ~D[2017-04-01] < ~D[2017-04-02], Date) +true + # Semantic comparisons with logical operators iex> compare?(~T[16:00:00] <= ~T[16:00:00] and not (~T[17:00:00] <= ~T[17:00:00]), Time) false @@ -37,7 +41,7 @@ Add `compare_chain` to your list of dependencies in `mix.exs`: ```elixir def deps do [ - {:compare_chain, "~> 0.3"} + {:compare_chain, "~> 0.5"} ] end ``` @@ -46,68 +50,158 @@ Documentation can be found at . ## Usage -Once installed, you can add +Once installed, you can add: ```elixir import CompareChain ``` -to your `defmodule` and you will have access to `compare?/1` and `compare?/2`. +to your `defmodule` and you will have access to `CompareChain.compare?/1` and `CompareChain.compare?/2`. ## Background and motivation -Many languages provide syntactic sugar for chained comparisons. -For example in Python, `a < b < c` would be evaluated as `(a < b) and (b < c)`. +`CompareChain` was originally motivated by the following situation: -Elixir does not provide this. -Instead, `a < b < c` is evaluated as `(a < b) < c`. -Since `a < b` is a boolean, that's probably not what you want. +> You have an interval of time bounded by a two `%Date{}` structs: `start_date` and `end_date`. +> You want to know if some third `date` falls in that interval. +> How do you write this? -Further, operators like `<` do _structural_ comparison instead of _semantic_ comparison. -For most situations, you probably want to use `compare/2`. -From the [`Kernel` docs](https://hexdocs.pm/elixir/Kernel.html#module-structural-comparison): +In Elixir, we'd write this as follows: + +```elixir +Date.compare(start_date, date) == :lt and + Date.compare(date, end_date) == :lt +``` -
-
+This is verbose and therefore a little hard to read. +It's also potentially incorrect. +What if `date` is considered "within" the interval even if it equals `start_date` or `end_date`? +To include the bounds in our comparison, we'd instead write the expression like this: -Show/Hide +```elixir +Date.compare(start_date, date) != :gt and + Date.compare(date, end_date) != :gt +``` -The comparison functions in this module perform structural comparison. -This means structures are compared based on their representation and not on their semantic value. -This is specially important for functions that are meant to provide ordering, such as >/2, </2, >=/2, <=/2, min/2, and max/2. -For example: +(We could have written `Date.compare(start_date, date) in [:lt, :eq]`, but `!= :gt` is faster.) -
-
-~D[2017-03-31] > ~D[2017-04-01]
-
-
+In order to spot the difference between these two cases, you have to keep several things in mind: -returns true because structural comparison compares the :day field before :month or :year. -Therefore, when comparing structs, you often use the compare/2 function made available by the structs modules themselves: + * The order of the arguments passed to `Date.compare/2` + * The specific comparison operators for each clause: `==` vs. `!=` + * The specific comparison atoms for each clause: `:lt` vs. `:gt` -
-
-iex> Date.compare(~D[2017-03-31], ~D[2017-04-01])
-:lt
-
-
+Since this is hard to read, it's easy to introduce bugs. +Contrast this with how you'd write the equivalent code in Python: -
-
+``` +start_date < date < end_date # excluding bounds +start_date <= date <= end_date # including bounds +``` + +This is much easier to read. +So why can't we write this in Elixir? +Two reasons: + + * Structural comparison operators + * Chained vs. nested comparisons + +### Structural comparison operators + +Operators like `<` do _structural_ comparison instead of _semantic_ comparison. +From the [`Kernel` docs](https://hexdocs.pm/elixir/Kernel.html#module-structural-comparison): -The `compare/2` approach works well in many situations, but even moderately complicated logic can be cumbersome. -If we wanted the native equivalent of: +> ... **comparisons in Elixir are structural**, as it has the goal + of comparing data types as efficiently as possible to create flexible + and performant data structures. This distinction is specially important + for functions that provide ordering, such as `>/2`, `=/2`, + `<=/2`, `min/2`, and `max/2`. For example: +> +> ```elixir +> ~D[2017-03-31] > ~D[2017-04-01] +> ``` +> +> will return `true` because structural comparison compares the `:day` + field before `:month` or `:year`. In order to perform semantic comparisons, + the relevant data-types provide a `compare/2` function, such as + `Date.compare/2`: +> +> ```elixir +> iex> Date.compare(~D[2017-03-31], ~D[2017-04-01]) +> :lt +> ``` + +In other words, although `~D[2017-03-31] > ~D[2017-04-01]` is perfectly valid code, it does _not_ tell you if `~D[2017-03-31]` is a later date than `~D[2017-04-01]` like you might expect. +Instead, you need to use `Date.compare/2`. + +### Chained vs. nested comparisons + +Additionally, even if `~D[2017-03-31] > ~D[2017-04-01]` did do semantic comparison, you still couldn't write the interval check like you do in Python. +This is because in Python, an expression like `1 < 2 < 3` is syntactic sugar for `(1 < 2) and (2 < 3)`, aka a series of "chained" expressions. + +Elixir does not provide an equivalent syntactic sugar. +Instead, `1 < 2 < 3` is evaluated as `(1 < 2) < 3`, aka a series of "nested" expressions. +Since `(1 < 2) < 3` simplifies to `true < 3`, that's probably not what you want! + +Elixir will even warn you when you attempt an expression like that: + +> warning: Elixir does not support nested comparisons. Something like +> +> x < y < z +> +> is equivalent to +> +> (x < y) < z +> +> which ultimately compares z with the boolean result of (x < y). Instead, consider joining together each comparison segment with an "and", for example, +> +> x < y and y < z + +### CompareChain + +`CompareChain` attempts to address both of these issues with the macro `CompareChain.compare?/2`. +Its job is to take code similar to how you'd like to write it and rewriting it to be semantically correct. + +For our motivating example, we'd write this: ```elixir -iex> compare?(~D[2017-03-31] <= ~D[2017-04-01] < ~D[2017-04-02], Date) +import CompareChain +compare?(start_date < date < end_date, Date) # excluding bounds +compare?(start_date <= date <= end_date, Date) # including bounds +``` + +And at compile time, `CompareChain.compare?/2` rewrites those to be: + +```elixir +# excluding bounds +Date.compare(start_date, date) == :lt and + Date.compare(date, end_date) == :lt + +# including bounds +Date.compare(start_date, date) != :gt and + Date.compare(date, end_date) != :gt +``` + +This way your code is more readable while still remaining correct. + +`CompareChain.compare?/1` is also available in case you only need chained comparison using the structural operators: + +```elixir +compare?(1 < 2 < 3) ``` -we'd have to write: +Though I find this case comes up less often. + +### One last selling point + +As a happy accident, `CompareChain.compare?/2` always uses fewer characters than its `compare/2` counterpart: ```elixir -iex> Date.compare(~D[2017-03-31], ~D[2017-04-01]) != :gt and Date.compare(~D[2017-04-01]), ~D[2017-04-02]) == :lt +compare?(a <= b, Date) +# vs. +Date.compare(a, b) != :gt ``` -The goal of both `compare?/1` and `compare?/2` is to provide the syntactic sugar for chained comparisons. -With `compare?/2`, there is the added benefit of being able to use the structural comparison operators for semantic comparison. +(Assuming you've already included `import CompareChain`, of course!) + +Because it's shorter _and_ more readable, these days I always use `CompareChain` for any semantic comparison, chained or not. \ No newline at end of file diff --git a/lib/compare_chain.ex b/lib/compare_chain.ex index fe49333..a916d93 100644 --- a/lib/compare_chain.ex +++ b/lib/compare_chain.ex @@ -1,288 +1,286 @@ defmodule CompareChain do @moduledoc """ Convenience macros for doing comparisons + + ## Valid expressions + + Valid expressions for `compare?/1` and `compare?/2` follow these three rules: + + ### 1. A comparison operator like `<` must be present. + + At least one of these must be included: `<`, `>`, `<=`, `>=`, `==`, `!=`, + `===`, or `!===`. So this is valid: + + compare?(1 < 2 < 3) + + but this is not: + + compare?(true) + + because it does not contain any comparisons. + + ### 2. All arguments to boolean operators must also be valid expressions. + + The boolean operators `and`, `or`, and `not` are allowed in expressions so + long as all of their arguments (eventually) contain a comparison. So this is + valid: + + compare?(1 < 2 < 3 and 4 < 5) + + as is this: + + compare?(not (not 1 < 2 < 3)) + + but this is not: + + compare?(1 < 2 < 3 and true) + + because the right side of `and` fails to contain a comparison. This + expression can be refactored to be valid by moving the non-comparison branch + outside `compare?/1` like so: + + compare?(1 < 2 < 3) and true + + ### 3. The root operator of an expression must be a comparison or a boolean. + + So this is not valid: + + compare?(my_function(a < b), Date) + + because its root operator is `my_function/1`. This expression can be + refactored to be valid by moving `compare?/2` inside `my_function/1` like so: + + my_function(compare?(a < b, Date)) + + We restrict expressions in this fashion so we can guarantee that `compare?/1` + and `compare?/2` will always return a boolean. + + Also note that arguments to _comparisons_ may be arbitrarily complicated: + + compare?(a < Date.utc_today(), Date) """ - require Integer + alias CompareChain.Core + alias CompareChain.ErrorMessage @doc """ - Macro that performs chained comparison using operators like `<` and - combinations using `and` `or`, and `not`. + Macro that performs chained comparison with operators like `<`. + + You may also include the boolean operators `and`, `or`, and `not` in the + expression so long as all their arguments all (eventually) contain + comparisons. See the moduledoc for more details. + + For a version that also does semantic comparison, see: `compare?/2`. ## Examples Chained comparison: - ``` - iex> import CompareChain - iex> compare?(1 < 2 < 3) - true - ``` + iex> import CompareChain + iex> compare?(1 < 2 < 3) + true Comparisons joined by logical operators: - ``` - iex> import CompareChain - iex> compare?(1 >= 2 >= 3 or 4 >= 5 >= 6) - false - ``` - - ## Notes - - You must include at least one comparison like `<` in your expression. - Failing to do so will result in a compile time error. - - Including a struct in the expression will result in a warning. - You probably want to use `compare?/2` instead. + iex> import CompareChain + iex> compare?(1 >= 2 >= 3 or 4 >= 5 >= 6) + false + + ## Warnings and errors + + > ### Comparing structs will warn {: .warning} + > + > Expressions which compare matching structs like: + > + > iex> compare?(~D[2017-03-31] < ~D[2017-04-01]) + > false + > + > Will result in a warning: + > + > ```plain + > ... [warning] Performing structural comparison on matching structs. + > + > Did you mean to use `compare?/2`? + > + > compare?(~D[2017-03-31] < ~D[2017-04-01], Date) + > ``` + > + > You probably want to use `compare?/2`, which does semantic comparison, + > instead. + + > ### Invalid expressions will raise {: .error} + > + > See the section on valid expressions in the moduledoc for details. """ defmacro compare?(expr) do ast = quote(do: unquote(expr)) - do_compare?(ast, CompareChain.DefaultCompare) + do_compare?(ast, Kernel) end @doc """ - Similar to `compare?/1` except you can provide a module that defines a - `compare/2` for semantic comparisons. + Macro that performs chained, semantic comparison with operators like `<` by + rewriting the expression using the `compare/2` function defined by the + provided module. This is like how you can provide a module as the second argument to - `Enum.sort/2`. + `Enum.sort/2` when you need to sort items semantically. + + You may also include the boolean operators `and`, `or`, and `not` in the + expression so long as all their arguments all (eventually) contain + comparisons. See the moduledoc for more details. + + For a version that does chained comparison using the normal `<` operators, + see: `compare?/1`. ## Examples - Basic comparison (note how `a < b == false` natively because of structural + Semantic comparison (note how `a < b == false` because of native structural comparison): - ``` - iex> import CompareChain - iex> a = ~D[2017-03-31] - iex> b = ~D[2017-04-01] - iex> a < b - false - iex> compare?(a < b, Date) - true - ``` + iex> import CompareChain + iex> a = ~D[2017-03-31] + iex> b = ~D[2017-04-01] + iex> a < b + false + iex> compare?(a < b, Date) + true - Chained comparison: + Chained, semantic comparison: - ``` - iex> import CompareChain - iex> a = ~D[2017-03-31] - iex> b = ~D[2017-04-01] - iex> c = ~D[2017-04-02] - iex> compare?(a < b < c, Date) - true - ``` + iex> import CompareChain + iex> a = ~D[2017-03-31] + iex> b = ~D[2017-04-01] + iex> c = ~D[2017-04-02] + iex> compare?(a < b < c, Date) + true Comparisons joined by logical operators: - ``` - iex> import CompareChain - iex> a = ~T[15:00:00] - iex> b = ~T[16:00:00] - iex> c = ~T[17:00:00] - iex> compare?(a < b and b > c, Time) - false - ``` + iex> import CompareChain + iex> a = ~T[15:00:00] + iex> b = ~T[16:00:00] + iex> c = ~T[17:00:00] + iex> compare?(a < b and b > c, Time) + false More complex expressions: - ``` - iex> import CompareChain - iex> compare?(%{a: ~T[16:00:00]}.a <= ~T[17:00:00], Time) - true - ``` + iex> import CompareChain + iex> compare?(%{a: ~T[16:00:00]}.a <= ~T[17:00:00], Time) + true Custom module: - ``` - iex> import CompareChain - iex> defmodule AlwaysGreaterThan do - iex> def compare(_left, _right), do: :gt - iex> end - iex> compare?(1 > 2 > 3, AlwaysGreaterThan) - true - ``` - - ## Notes - - You must include at least one comparison like `<` in your expression. - Failing to do so will result in a compile time error. + iex> import CompareChain + iex> defmodule AlwaysGreaterThan do + iex> def compare(_left, _right), do: :gt + iex> end + iex> compare?(1 > 2 > 3, AlwaysGreaterThan) + true + + ## Warnings and errors + + > ### Using the "strict" operators will warn {: .warning} + > + > Expressions which include either `===` or `!==` like: + > + > iex> compare?(~D[2017-03-31] !== ~D[2017-04-01], Date) + > true + > + > Will result in a warning: + > + > ```plain + > ... [warning] Performing semantic comparison using either: `===` or `!===`. + > This is reinterpreted as `==` or `!=`, respectively. + > ``` + > + > These operators have no additional meaning over `==` and `!=` when doing + > semantic comparison. + + > ### Invalid expressions will raise {: .error} + > + > See the section on valid expressions in the moduledoc for details. """ defmacro compare?(expr, module) do ast = quote(do: unquote(expr)) do_compare?(ast, module) end - # Calls `chain` on the arguments of `and` and `or`. - # E.g. for `a < b < c and d > e`, - # - # ``` - # and - # / \ - # (a < b < c) (c > d) - # ``` - # - # becomes - # - # ``` - # and - # / \ - # chain(a < b < c) chain(c > d) - # ``` - defp do_compare?(ast, module) do - {ast, chain_or_raise_called?} = - Macro.postwalk(ast, false, fn - {op, meta, [left, right]}, called? when op in [:and, :or] -> - {left, called_left?} = maybe_call_chain_or_raise(left, module) - {right, called_right?} = maybe_call_chain_or_raise(right, module) - - called? = called? or called_left? or called_right? + defguardp is_symmetric_op(op) when op == :== or op == :!= or op == :=== or op == :!== + defguardp is_asymmetric_op(op) when op == :<= or op == :>= or op == :< or op == :> + defguardp is_combination_op(op) when op == :and or op == :or or op == :not + defguardp is_comparison_op(op) when is_symmetric_op(op) or is_asymmetric_op(op) + defguardp is_comparison(node) when is_tuple(node) and is_comparison_op(elem(node, 0)) + defguardp is_combination(node) when is_tuple(node) and is_combination_op(elem(node, 0)) - {{op, meta, [left, right]}, called?} - - node, called? -> - {node, called?} - end) - - # If no `and`s or `or`s were present in `ast`, we haven't called - # `chain_or_raise` yet and so we need to do so. - if not chain_or_raise_called? do - chain_or_raise(ast, module) - else - ast - end + defp do_compare?(ast, module) do + ast + |> preprocess() + |> chain_nested_comparisons() + |> convert_comparisons(module) end - defp maybe_call_chain_or_raise(node, module) do - case node do - {op, _, _} when op in [:<, :>, :<=, :>=, :==, :!=, :not] -> - {chain_or_raise(node, module), true} + defp preprocess(ast) do + # Unwrap blocks so they don't mess with how we detect nested comparisons. + ast_without_blocks = + Macro.prewalk(ast, fn + {:__block__, _, [node]} -> node + node -> node + end) - _ -> - {node, false} + if not valid?(ast_without_blocks) do + raise ArgumentError, ErrorMessage.invalid_expression(ast) end - end - - defp chain_or_raise(node, module) do - node = chain(node, module) - if node == :no_comparison_operators_found do - raise ArgumentError, CompareChain.ErrorMessage.chain_error_message() - else - node - end + ast_without_blocks end - # Transforms a chain of comparisons into a series of `and`'d pairs. - # E.g. for `a < b < c`, - # - # ``` - # < - # / \ - # < c - # / \ - # a b - # ``` - # - # becomes - # - # ``` - # and - # / \ - # ~ ~ - # / \ / \ - # a b b c - # ``` - # - # where `~` is roughly `compare(left, right) == :lt`. - defp chain(ast, module) do - {not_count, ast} = unwrap_nots(ast) - - expr_or_atom = - ast - |> chain_nested_ops() - |> Enum.map(fn op -> op_to_module_expr(op, module) end) - |> Enum.reduce(:no_comparison_operators_found, fn expr, acc -> - if acc == :no_comparison_operators_found do - expr - else - quote(do: unquote(acc) and unquote(expr)) - end - end) + defp valid?(ast), do: valid?(ast, false) + defp valid?(node, false) when is_combination(node), do: Enum.all?(elem(node, 2), &valid?(&1)) + defp valid?(node, false) when is_comparison(node), do: true + defp valid?(_, _), do: false - cond do - expr_or_atom == :no_comparison_operators_found -> - :no_comparison_operators_found + defp chain_nested_comparisons(ast) do + ast + |> Macro.prewalk(fn + {_, _, [inner, _]} = outer when is_comparison(outer) and is_comparison(inner) -> + chain_nested_comparison(outer) - Integer.is_odd(not_count) -> - quote(do: not unquote(expr_or_atom)) + node -> + node + end) + end - true -> - expr_or_atom - end + # Converts an ast like `==(c, <=(a, b))` to an ast like `<=(==(c, a), b)`. + # We do this to cover an edge case where symmetric comparison operators have + # a higher precedence than the asymmetric operators. They therefore appear + # "out of order" in the AST. This reordering should result in an equivalent + # expression due to symmetry. + defp chain_nested_comparison({outer, meta_outer, [c, {inner, meta_inner, [a, b]}]}) + when is_symmetric_op(outer) and is_asymmetric_op(inner) do + # Reminder! The ((c, a), b) order looks wrong but is correct. + reordered_ast = {inner, meta_inner, [{outer, meta_outer, [c, a]}, b]} + chain_nested_comparison(reordered_ast) end - # Unwraps any nested series of `not`s and counts the number of `not`s. - # E.g. `not (not ( not (1 < 2)))` returns `{3, 1 < 2}` - defp unwrap_nots(ast) do - [nil] - |> Stream.cycle() - |> Enum.reduce_while({0, ast}, fn - _, {count, {:not, _, [node]}} -> - {:cont, {count + 1, node}} - - # Do I need to also account for `:__block__` elsewhere? - _, {count, {:__block__, _, [node]}} -> - {:cont, {count, node}} - - _, {count, node} -> - {:halt, {count, node}} - end) + # Converts an ast for `a < b < c` to an ast for `a < b and b < c`. + defp chain_nested_comparison({outer, meta, [{inner, _, [a, b]}, c]}) do + left = {inner, meta, [a, b]} + right = {outer, meta, [b, c]} + {:and, meta, [left, right]} end - # Converts nested expressions like - # <(<(<(a, b), c), d) - # to a list of paired expresions like - # [<(a, b), <(b, c), <(c, d)] - defp chain_nested_ops(ast) do - ast - # Build up a stack of comparison operators and their right arguments. - # This works because the right is guaranteed to be a comparison leaf, not - # another comparison. - |> Macro.prewalker() - |> Enum.reduce_while([], fn - {op, _, [_left, right]}, acc when op in [:<, :>, :<=, :>=, :==, :!=] -> - {:cont, [{op, right} | acc]} - - node, acc -> - {:halt, [{nil, node} | acc]} + defp convert_comparisons(ast, module) do + Macro.prewalk(ast, fn + node when is_comparison(node) -> convert_comparison(node, module) + node -> node end) - |> Enum.chunk_every(2, 1, :discard) - |> Enum.map(fn [{_, left}, {op, right}] -> {op, left, right} end) end - # Converts an ast like - # `{<, left, right}` - # to an expression like - # `module.compare(left, right) == :lt` - defp op_to_module_expr({op, left, right}, module) do - {kernel_fun, evals_to} = - case op do - :< -> {:==, :lt} - :> -> {:==, :gt} - :<= -> {:!=, :gt} - :>= -> {:!=, :lt} - :== -> {:==, :eq} - :!= -> {:!=, :eq} - end - - inner_comparison = - quote do - unquote(module).compare(unquote(left), unquote(right)) - end - - quote do - Kernel.unquote(kernel_fun)(unquote(inner_comparison), unquote(evals_to)) - end + defp convert_comparison({op, meta, [left, right]}, module) do + # We use `Core.compare?/4` instead of direct AST manipulation so we can + # warn at runtime. + compare_fun = {:., meta, [Core, :compare?]} + {compare_fun, meta, [module, op, left, right]} end end diff --git a/lib/compare_chain/core.ex b/lib/compare_chain/core.ex new file mode 100644 index 0000000..a00cbdd --- /dev/null +++ b/lib/compare_chain/core.ex @@ -0,0 +1,39 @@ +defmodule CompareChain.Core do + @moduledoc false + + alias CompareChain.ErrorMessage + + require Logger + + @doc false + def compare?(Kernel, operator, left, right) do + if is_struct(left) or is_struct(right) do + message = ErrorMessage.struct_warning(operator, left, right) + Logger.warning(message) + end + + apply(Kernel, operator, [left, right]) + end + + def compare?(module, operator, left, right) when is_atom(module) do + if operator == :=== or operator == :!== do + message = ErrorMessage.strict_operator_warning() + Logger.warning(message) + end + + {kernel_fun, evals_to} = + case operator do + :< -> {:==, :lt} + :> -> {:==, :gt} + :<= -> {:!=, :gt} + :>= -> {:!=, :lt} + :== -> {:==, :eq} + :!= -> {:!=, :eq} + :=== -> {:==, :eq} + :!== -> {:!=, :eq} + end + + comparison = apply(module, :compare, [left, right]) + apply(Kernel, kernel_fun, [comparison, evals_to]) + end +end diff --git a/lib/compare_chain/default_compare.ex b/lib/compare_chain/default_compare.ex deleted file mode 100644 index 9a0a531..0000000 --- a/lib/compare_chain/default_compare.ex +++ /dev/null @@ -1,44 +0,0 @@ -defmodule CompareChain.DefaultCompare do - @moduledoc false - require Logger - - def compare(left, right) do - if is_struct(left) or is_struct(right) do - message = struct_warning_message(left, right) - - Logger.warning(message) - end - - cond do - left < right -> :lt - left > right -> :gt - left == right -> :eq - end - end - - defp struct_warning_message(%match{} = left, %match{} = right) do - """ - Performing structural comparison on matching structs. - - Did you mean to use `compare?/2`? - - compare?(#{inspect(left)} ??? #{inspect(right)}, #{struct_string(left)}) - """ - end - - defp struct_warning_message(left, right) do - """ - Performing structural comparison on one or more mismatched structs. - - Left#{if(is_struct(left), do: " (%#{struct_string(left)}{} struct)", else: "")}: - - #{inspect(left)} - - Right#{if(is_struct(right), do: " (%#{struct_string(right)}{} struct)", else: "")}: - - #{inspect(right)} - """ - end - - defp struct_string(%s{}), do: s |> Atom.to_string() |> String.replace_leading("Elixir.", "") -end diff --git a/lib/compare_chain/error_message.ex b/lib/compare_chain/error_message.ex index d7356eb..e7478b9 100644 --- a/lib/compare_chain/error_message.ex +++ b/lib/compare_chain/error_message.ex @@ -1,12 +1,57 @@ defmodule CompareChain.ErrorMessage do + # This module is public for testing. @moduledoc false - # Public for testing @doc false - def chain_error_message() do + def invalid_expression(ast) do """ - No comparison operators found. - Expression must include at least one of `<`, `>`, `<=`, `>=`, `==`, or `!=`. + The following expression is an invalid argument to `compare?/{1,2}`: + + #{Macro.to_string(ast)} + + Valid expressions follow these three rules: + + 1. A comparison operator like `<` must be present. + 2. All arguments to boolean operators must also be valid expressions. + 3. The root operator of an expression must be a comparison or a boolean. + + So at least one of these rules is not satisfied by the expression. See the + moduledoc for `CompareChain` for more details including refactoring hints. """ end + + def strict_operator_warning do + """ + Performing semantic comparison using either: `===` or `!===`. + This is reinterpreted as `==` or `!=`, respectively. + """ + end + + @doc false + def struct_warning(operator, %match{} = left, %match{} = right) do + """ + Performing structural comparison on matching structs. + + Did you mean to use `compare?/2`? + + compare?(#{inspect(left)} #{operator} #{inspect(right)}, #{struct_string(left)}) + """ + end + + @doc false + def struct_warning(_operator, left, right) do + """ + Performing structural comparison on one or more mismatched structs. + + Left#{if(is_struct(left), do: " (%#{struct_string(left)}{} struct)", else: "")}: + + #{inspect(left)} + + Right#{if(is_struct(right), do: " (%#{struct_string(right)}{} struct)", else: "")}: + + #{inspect(right)} + """ + end + + defp struct_string(%s{}), do: s |> Atom.to_string() |> String.replace_leading("Elixir.", "") end diff --git a/mix.exs b/mix.exs index dfeb8fe..04059d0 100644 --- a/mix.exs +++ b/mix.exs @@ -2,7 +2,7 @@ defmodule CompareChain.MixProject do use Mix.Project @source_url "https://github.com/CargoSense/compare_chain" - @version "0.4.0" + @version "0.5.0" def project do [ @@ -26,7 +26,7 @@ defmodule CompareChain.MixProject do # Run `mix help deps` to learn about dependencies. defp deps do [ - {:ex_doc, "~> 0.27", only: :dev, runtime: false} + {:ex_doc, "~> 0.31", only: :dev, runtime: false} ] end diff --git a/mix.lock b/mix.lock index e2a26ce..559b793 100644 --- a/mix.lock +++ b/mix.lock @@ -1,8 +1,8 @@ %{ - "earmark_parser": {:hex, :earmark_parser, "1.4.30", "0b938aa5b9bafd455056440cdaa2a79197ca5e693830b4a982beada840513c5f", [:mix], [], "hexpm", "3b5385c2d36b0473d0b206927b841343d25adb14f95f0110062506b300cd5a1b"}, - "ex_doc": {:hex, :ex_doc, "0.29.1", "b1c652fa5f92ee9cf15c75271168027f92039b3877094290a75abcaac82a9f77", [:mix], [{:earmark_parser, "~> 1.4.19", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1", [hex: :makeup_erlang, repo: "hexpm", optional: false]}], "hexpm", "b7745fa6374a36daf484e2a2012274950e084815b936b1319aeebcf7809574f6"}, - "makeup": {:hex, :makeup, "1.1.0", "6b67c8bc2882a6b6a445859952a602afc1a41c2e08379ca057c0f525366fc3ca", [:mix], [{:nimble_parsec, "~> 1.2.2 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "0a45ed501f4a8897f580eabf99a2e5234ea3e75a4373c8a52824f6e873be57a6"}, - "makeup_elixir": {:hex, :makeup_elixir, "0.16.0", "f8c570a0d33f8039513fbccaf7108c5d750f47d8defd44088371191b76492b0b", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}, {:nimble_parsec, "~> 1.2.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "28b2cbdc13960a46ae9a8858c4bebdec3c9a6d7b4b9e7f4ed1502f8159f338e7"}, - "makeup_erlang": {:hex, :makeup_erlang, "0.1.1", "3fcb7f09eb9d98dc4d208f49cc955a34218fc41ff6b84df7c75b3e6e533cc65f", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "174d0809e98a4ef0b3309256cbf97101c6ec01c4ab0b23e926a9e17df2077cbb"}, - "nimble_parsec": {:hex, :nimble_parsec, "1.2.3", "244836e6e3f1200c7f30cb56733fd808744eca61fd182f731eac4af635cc6d0b", [:mix], [], "hexpm", "c8d789e39b9131acf7b99291e93dae60ab48ef14a7ee9d58c6964f59efb570b0"}, + "earmark_parser": {:hex, :earmark_parser, "1.4.39", "424642f8335b05bb9eb611aa1564c148a8ee35c9c8a8bba6e129d51a3e3c6769", [:mix], [], "hexpm", "06553a88d1f1846da9ef066b87b57c6f605552cfbe40d20bd8d59cc6bde41944"}, + "ex_doc": {:hex, :ex_doc, "0.32.2", "f60bbeb6ccbe75d005763e2a328e6f05e0624232f2393bc693611c2d3ae9fa0e", [:mix], [{:earmark_parser, "~> 1.4.39", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_c, ">= 0.1.0", [hex: :makeup_c, repo: "hexpm", optional: true]}, {:makeup_elixir, "~> 0.14 or ~> 1.0", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1 or ~> 1.0", [hex: :makeup_erlang, repo: "hexpm", optional: false]}, {:makeup_html, ">= 0.1.0", [hex: :makeup_html, repo: "hexpm", optional: true]}], "hexpm", "a4480305cdfe7fdfcbb77d1092c76161626d9a7aa4fb698aee745996e34602df"}, + "makeup": {:hex, :makeup, "1.1.2", "9ba8837913bdf757787e71c1581c21f9d2455f4dd04cfca785c70bbfff1a76a3", [:mix], [{:nimble_parsec, "~> 1.2.2 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "cce1566b81fbcbd21eca8ffe808f33b221f9eee2cbc7a1706fc3da9ff18e6cac"}, + "makeup_elixir": {:hex, :makeup_elixir, "0.16.2", "627e84b8e8bf22e60a2579dad15067c755531fea049ae26ef1020cad58fe9578", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}, {:nimble_parsec, "~> 1.2.3 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "41193978704763f6bbe6cc2758b84909e62984c7752b3784bd3c218bb341706b"}, + "makeup_erlang": {:hex, :makeup_erlang, "1.0.0", "6f0eff9c9c489f26b69b61440bf1b238d95badae49adac77973cbacae87e3c2e", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "ea7a9307de9d1548d2a72d299058d1fd2339e3d398560a0e46c27dab4891e4d2"}, + "nimble_parsec": {:hex, :nimble_parsec, "1.4.0", "51f9b613ea62cfa97b25ccc2c1b4216e81df970acd8e16e8d1bdc58fef21370d", [:mix], [], "hexpm", "9c565862810fb383e9838c1dd2d7d2c437b3d13b267414ba6af33e50d2d1cf28"}, } diff --git a/test/compare_chain_test.exs b/test/compare_chain_test.exs index 46eb9fe..bfae086 100644 --- a/test/compare_chain_test.exs +++ b/test/compare_chain_test.exs @@ -5,86 +5,129 @@ defmodule CompareChainTest do import CompareChain - test "comparison on incompatible structs raises" do - # Finding the expected error message this way seems a bit brittle. - stacktrace = - try do - Date.compare(~D[2022-01-01], ~T[00:00:00]) == :lt - rescue - err -> Exception.format(:error, err, __STACKTRACE__) - end - - "** (MatchError) " <> message = stacktrace |> String.split("\n") |> List.first() - - assert_raise(MatchError, message, fn -> - compare?(~D[2022-01-01] < ~T[00:00:00], Date) - end) + describe "`compare?/1`" do + test "works with `===` and `!==`" do + refute compare?(1 === 1.0) + assert compare?(1 !== 1.0) + end + + # The AST is out of order when these operators are mixed. + test "mixed symmetric and asymmetric operators" do + assert compare?(1 < 2 != 3 < 4) + refute compare?(1 < 2 != 3 > 4) + end end - test "compare?/1 with mismatched structs raises warning" do - warning_message = - ExUnit.CaptureLog.capture_log(fn -> - compare?(~D[2022-01-02] < ~T[00:00:00]) - end) + describe "`compare?/2`" do + test "works with `==` and `!=`" do + assert compare?(~T[00:00:00] == ~T[00:00:00], Time) + refute compare?(~T[00:00:00] == ~T[11:11:11], Time) + refute compare?(~T[00:00:00] != ~T[00:00:00], Time) + assert compare?(~T[00:00:00] != ~T[11:11:11], Time) + end + + test "basic examples" do + a = ~U[2020-01-01 00:00:00Z] + b = ~U[2021-01-01 00:00:00Z] + c = ~U[2022-01-01 00:00:00Z] + d = ~U[2023-01-01 00:00:00Z] + + assert compare?(a < b <= c, DateTime) + assert compare?(a < b < c < d, DateTime) + refute compare?(%{val: b}.val >= d, DateTime) + assert compare?(a > b or c < d, DateTime) + refute compare?(a > b and c < d, DateTime) + end + + # The AST is out of order when these operators are mixed. + test "mixed symmetric and asymmetric operators" do + a = ~U[2020-01-01 00:00:00Z] + b = ~U[2021-01-01 00:00:00Z] + c = ~U[2022-01-01 00:00:00Z] + d = ~U[2023-01-01 00:00:00Z] + e = ~U[2024-01-01 00:00:00Z] + f = ~U[2025-01-01 00:00:00Z] + g = ~U[2026-01-01 00:00:00Z] + h = ~U[2027-01-01 00:00:00Z] + + refute compare?(a < b == c < d == e < f != g < h, DateTime) + end + end - assert warning_message =~ """ - [warning] Performing structural comparison on one or more mismatched structs. + describe "odd usages" do + test "nested nots" do + assert compare?( + ~D[2020-01-01] > ~D[2020-01-02] or + (not (not (not (not (~D[2020-01-01] < ~D[2020-01-02])))) and + not (not (~D[2020-01-01] < ~D[2020-01-02]))) or + not (~D[2020-01-01] < ~D[2020-01-02]), + Date + ) + end + end - Left (%Date{} struct): + describe "errors and warnings" do + test "comparison on incompatible structs raises" do + # Finding the expected error message this way seems a bit brittle. + stacktrace = + try do + Date.compare(~D[2022-01-01], ~T[00:00:00]) == :lt + rescue + err -> Exception.format(:error, err, __STACKTRACE__) + end - ~D[2022-01-02] + "** (MatchError) " <> message = stacktrace |> String.split("\n") |> List.first() - Right (%Time{} struct): + assert_raise(MatchError, message, fn -> + compare?(~D[2022-01-01] < ~T[00:00:00], Date) + end) + end - ~T[00:00:00] - """ - end + test "compare?/1 with mismatched structs raises warning" do + warning_message = + ExUnit.CaptureLog.capture_log(fn -> + compare?(~D[2022-01-02] < ~T[00:00:00]) + end) - test "compare?/1 with matching structs raises warning with a hint" do - warning_message = - ExUnit.CaptureLog.capture_log(fn -> - compare?(~D[2022-01-02] < ~D[2022-02-01]) - end) + assert warning_message =~ """ + [warning] Performing structural comparison on one or more mismatched structs. - assert warning_message =~ """ - [warning] Performing structural comparison on matching structs. + Left (%Date{} struct): - Did you mean to use `compare?/2`? + ~D[2022-01-02] - compare?(~D[2022-01-02] ??? ~D[2022-02-01], Date) - """ - end + Right (%Time{} struct): - test "works with boolean literals" do - assert compare?((true and 1 < 2) or false) - end + ~T[00:00:00] + """ + end - test "works with `==` and `!=`" do - assert compare?(~T[00:00:00] == ~T[00:00:00], Time) - refute compare?(~T[00:00:00] == ~T[11:11:11], Time) - refute compare?(~T[00:00:00] != ~T[00:00:00], Time) - assert compare?(~T[00:00:00] != ~T[11:11:11], Time) - end + test "compare?/1 with matching structs raises warning with a hint" do + warning_message = + ExUnit.CaptureLog.capture_log(fn -> + compare?(~D[2022-01-02] < ~D[2022-02-01]) + end) - test "basic `compare?/2` examples" do - a = ~U[2020-01-01 00:00:00Z] - b = ~U[2021-01-01 00:00:00Z] - c = ~U[2022-01-01 00:00:00Z] - d = ~U[2023-01-01 00:00:00Z] + assert warning_message =~ """ + [warning] Performing structural comparison on matching structs. - assert compare?(a < b <= c, DateTime) - refute compare?(%{val: b}.val >= d, DateTime) - assert compare?(a > b or c < d, DateTime) - refute compare?(a > b and c < d, DateTime) - end + Did you mean to use `compare?/2`? + + compare?(~D[2022-01-02] < ~D[2022-02-01], Date) + """ + end + + test "compare?/2 with strict operator raises warning" do + warning_message = + ExUnit.CaptureLog.capture_log(fn -> + compare?(~D[2022-01-02] === ~D[2022-01-02], Date) + end) - test "nested nots" do - assert compare?( - ~D[2020-01-01] > ~D[2020-01-02] or - (not (not (not (not (~D[2020-01-01] < ~D[2020-01-02])))) and - not (not (~D[2020-01-01] < ~D[2020-01-02]))) or - not (~D[2020-01-01] < ~D[2020-01-02]), - Date - ) + assert warning_message =~ + """ + Performing semantic comparison using either: `===` or `!===`. + This is reinterpreted as `==` or `!=`, respectively. + """ + end end end diff --git a/test/compile_time_test.exs b/test/compile_time_test.exs index 80db197..f1398c0 100644 --- a/test/compile_time_test.exs +++ b/test/compile_time_test.exs @@ -8,14 +8,44 @@ defmodule CompileTimeTest do # https://github.com/phoenixframework/phoenix/blob/master/test/phoenix/verified_routes_test.exs use ExUnit.Case - import CompareChain.ErrorMessage + alias CompareChain.ErrorMessage - test "including no comparison operators raises at compile time" do - assert_raise(ArgumentError, chain_error_message(), fn -> - defmodule NoOperators do - import CompareChain - def fun, do: compare?(5) + test "including no comparison operators raises" do + assert_raise( + ArgumentError, + ErrorMessage.invalid_expression(5), + fn -> + defmodule NoOperators do + import CompareChain + def fun, do: compare?(5) + end end - end) + ) + end + + test "a non-comparison-or-combination at the root of the ast raises" do + assert_raise( + ArgumentError, + ErrorMessage.invalid_expression(quote(do: abs(~D[2020-01-01] < ~D[2020-01-02]))), + fn -> + defmodule NestedCalls do + import CompareChain + def fun, do: compare?(abs(~D[2020-01-01] < ~D[2020-01-02]), Date) + end + end + ) + end + + test "one branch of a combination failing to contain a comparison raises" do + assert_raise( + ArgumentError, + ErrorMessage.invalid_expression(quote(do: 1 < 2 < 3 and true)), + fn -> + defmodule NestedCalls do + import CompareChain + def fun, do: compare?(1 < 2 < 3 and true) + end + end + ) end end