diff --git a/.formatter.exs b/.formatter.exs index 77c8bc58..4885f08c 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -1,3 +1,13 @@ +# Copyright 2024 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + [ inputs: [ "{mix,.formatter}.exs", @@ -8,6 +18,6 @@ assert_style: 2 ], plugins: [Styler], - styler: [alias_lifting_exclude: []], + styler: [alias_lifting_exclude: [], minimum_supported_elixir_version: nil], line_length: 122 ] diff --git a/CHANGELOG.md b/CHANGELOG.md index 62da55e2..4296aa50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,254 @@ **Note** Styler's only public API is its usage as a formatter plugin. While you're welcome to play with its internals, they can and will change without that change being reflected in Styler's semantic version. + ## main +## 1.11.0 + +### Improvements + +- `mix styler.inline_attrs`: Allow multiple file paths to be specified: `mix styler.inline_attrs [ ...]` + +#### Module Directive References + +Module directives got smarter. Styler will no longer move module attributes below their references in `use` or `@moduledoc`s. + +In other words, Styler will leave the following code untouched: + +```elixir +defmodule MyGreatLibrary do + @library_options [...] + @moduledoc make_pretty_docs(@library_options) + use OptionsMagic, my_opts: @library_options +end +``` + +## 1.10.1 + +### Improvements + +Adds two experimental refactoring features as mix tasks. + +#### `mix styler.remove_unused` + +With Elixir 1.20 on the horizon, many projects are about to discover that they have _a lot_ of unnecessary `require Logger` lines throughout their codebase. + +`mix styler.remove_unused` will automate the removal of those `unused require:` statements, alongside any `unused import:` and `unused alias:` warnings. + +This has long been an internal script useful for running after a bigger refactor that resulted in many superfluous aliases, but with 1.20 coming it seems it might be useful for others as well. + +This will never be an integrated part of `Styler`'s format plugin features, as it would _not_ be correct to remove unused nodes whenever running format. It's typical to have unused warnings while in the midst of an implementation, and deleting that code would be obnoxious. + +#### `mix styler.inline_attrs ` + +Inlines one-off module attributes that define literal values. + +This is something that sometimes is good, and sometimes is bad. In general, defining a module attribute when you could've just written an atom is bad, so inlining is good! + +It would probably be most useful as a refactor ability for a language server, but CLIs are a nice second place. + +An example of a situation where it results in an improvement: + +```elixir +# Unnecessary indirection with single-use literal-value module attributes +defmodule A do + @http_client_key :http_key + @default_client MyHTTPClient + + def http_client, do: Application.get_env(:my_app, @http_client_key, @default_client) +end +# Much better! styler.inline_attrs will perform this refactor +defmodule A do + def http_client, do: Application.get_env(:my_app, :http_key, MyHTTPClient) +end +``` + +It's worthwhile to run this on some suspicious files, then followup with manual intervention when it went too far. This style is not aware of quote boundaries, and so might do some broken things. (Hence "EXPERIMENTAL") + +You've been warned =) + +## 1.10.0 + +### Improvements + +Two new standard-library pipe optimizations + +- `enum |> Enum.map(fun) |> Enum.intersperse(separator)` => `Enum.map_intersperse(enum, separator, fun)` +- `enum |> Enum.sort() |> Enum.reverse()` => `Enum.sort(enum, :desc)` + +And Req (the http client library) pipe optimizations, as detailed below + +#### Req pipe optimizations + +[Req](https://github.com/wojtekmach/req) is a popular HTTP Client. If you aren't using it, you can just ignore this whole section! + +Reqs 1-arity "execute the request" functions (`delete get head patch post put request run`) have a 2-arity version that takes a superset of the arguments `Req.new/1` does as its first argument, and the typical `options` keyword list as its second argument. And so, many places developers are calling a 1-arity function can be replaced with a 2-arity function. + +More succinctly, these two statements are equivalent: + +- `foo |> Req.new() |> Req.merge(bar) |> Req.post!()` +- `Req.post!(foo, bar)` + +Styler now rewrites the former to the latter, since "less is more" or "code is a liability". + +It also rewrites `|> Keyword.merge(bar) |> Req.foo()` to `|> Req.foo(bar)`. **This changes the program's behaviour**, since `Keyword.merge` would overwrite existing values in all cases, whereas `Req` 2-arity functions intelligently deep-merge values for some keys, like `:headers`. + +## 1.9.1 + +### Fix + +- fixes rewrites of single-clause case statement with assignment parent (Closes #247, h/t @vasspilka) + +## 1.9.0 + +This was a weird one, but I found myself often writing `to_timeout` with plural units and then having to go back and fix +the code to be singular units instead. Polling a few colleagues, it seemed I wasn't alone in that mistake. So for the first time, +Styler will correct code that would otherwise produce a runtime error, saving you from flow-breaking backtracking. + +### Improvements + +`to_timeout` improvements: + +- translate plural units to singular `to_timeout(hours: 2)` -> `to_timeout(hour: 2)` (plurals are valid ast, but invalid arguments to this function) +- transform when there are multiple keys: `to_timeout(hours: 24 * 1, seconds: 60 * 4)` -> `to_timeout(day: 1, minute: 4)`. **this can introduce runtime bugs** due to duplicate keys, as in the following scenario: `to_timeout(minute: 60, hours: 3)` -> `to_timeout(hour: 1, hour: 3)` + +## 1.8.0 + +### Improvements + +Rewrite single-clause case statements to be assignments (h/t 🤖) + +```elixir +# before +case foo |> Bar.baz() |> Bop.boop() do + {:ok, widget} -> + x = y + wodget(widget) +end + +# after +{:ok, widget} = foo |> Bar.baz() |> Bop.boop() +x = y +wodget(widget) +``` + +## 1.7.0 + +Surprising how fast numbers go up when you're following semver. + +Two new features, one being a pipe optimization and the other a style-consistency-enforcer in `cond` statements. + +### Improvements + +- `|> Enum.filter(fun) |> List.first([default])` => `|> Enum.find([default], fun)` (#242, h/t @janpieper) + +#### `cond` + +If the last clause's left-hand-side is a truthy atom, map literal, or tuple, rewrite it to be `true` + +```elixir +# before +cond do + a -> b + c -> d + :else -> e +end + +# styled +cond do + a -> b + c -> d + true -> e +end +``` + +This also helps Styler identify 2-clause conds that can be rewritten to `if/else` more readily, like the following: + +```elixir +# before +cond do + a -> b + :else -> c +end + +# styled +if a do + b +else + c +end +``` + +## 1.6.0 + +That's right, a feature release again so soon! + +### Improvements + +This version of Styler adds many readability improvements around ExUnit `assert` and `refute`, specifically when working with 1. negations or 2. some `Enum` stdlib functions. + +Some of these rewrites are not semantically equivalent due to `refute` passing for both `nil` and `false`. + +#### ExUnit assert/refute rewrites + +Styler now inverts negated (`!, not`) assert/refute (eg `assert !x` => `refute x`) statements, and further inverts `refute` with boolean comparison operators (`refute x < y` => `assert x >= y`) because non-trivial refutes are harder to reason about \[ _citation needed_ ]. Asserting something is not nil is the same as just asserting that something, so that's gone too now. + +These changes are best summarized by the following table: + +| before | styled | +|---------------------|-------------------| +| `assert !x` | `refute x` | +| `assert not x` | `refute x` | +| `assert !!x` | `assert x` | +| `assert x != nil` | `assert x` | +| `assert x == nil` | _no change_ | +| `assert is_nil(x)` | _no change_ | +| `assert !is_nil(x)` | `assert x` | +| `assert x not in y` | `refute x in y` | +| refute negated | | +| `refute x` | _no change_ | +| `refute !x` | `assert x` | +| `refute not x` | `assert x` | +| `refute x != y` | `assert x == y` | +| `refute x !== y` | `assert x === y` | +| `refute x != nil` | `assert x == nil` | +| `refute x not in y` | `assert x in y` | +| refute comparison | | +| `refute x < y` | `assert x >= y` | +| `refute x <= y` | `assert x > y` | +| `refute x > y` | `assert x <= y` | +| `refute x >= y` | `assert x < y` | + +- `assert Enum.member?(y, x)` -> `assert x in y` +- `assert Enum.find(x, y)` -> `assert Enum.any?(x, y)` (nb. not semantically equivalent in theory, but equivalent in practice) +- `assert Enum.any?(y, & &1 == x)` -> `assert x in y` +- `assert Enum.any?(y, fn var -> var == x end)` -> `assert x in y` + +### Fixes + +- alias lifting: fix bug lifting in snippets with a single ast node at the root level (like a credo config file) (#240, h/t @defndaines) + +## 1.5.1 + +### Fixes + +- alias lifting: handle comments in snippets with no existing directives (#239, h/t @kerryb) + +## 1.5.0 + +### Improvements + +- apply aliases to code. if a module is aliased, and then later referenced with its full name, Styler will now shorten it to its alias. (#235, h/t me) +- added `:minimum_supported_elixir_version` configuration to better support libraries using Styler (#231, h/t @maennchen) +- `# styler:sort` will now sort keys for struct/map typespecs (#213, h/t @rojnwa) + +### Fixes + +- apply alias lifting to snippets with no modules or module directives in them. (#189, @h/t @halfdan) +- fix de-sugaring of syntax-sugared keyword lists whose values weren't atoms in map values (#236, h/t @RisPNG) +- fix mix config sorting mangling floating comment blocks in some cases (#230 again, h/t @ryoung786) + ## 1.4.2 ### Fixes diff --git a/README.md b/README.md index 78a1ed84..a3a9237d 100644 --- a/README.md +++ b/README.md @@ -4,31 +4,38 @@ # Styler -Styler is an Elixir formatter plugin that's combination of `mix format` and `mix credo`, except instead of telling -you what's wrong, it just rewrites the code for you to fit its style rules. - -You can learn more about the history, purpose and implementation of Styler from our talk: [Styler: Elixir Style-Guide Enforcer @ GigCity Elixir 2023](https://www.youtube.com/watch?v=6pF8Hl5EuD4) +Styler is an Elixir formatter plugin that goes beyond formatting by rewriting your code to optimize for consistency, readability, and performance. ## Features -Styler fixes a plethora of elixir style and optimization issues automatically as part of mix format. +[Styler's full feature documentation can be found on Hexdocs.](https://hexdocs.pm/styler/styles.html) -[See Styler's documentation on Hex](https://hexdocs.pm/styler/styles.html) for the comprehensive list of its features. +Styler fixes a plethora of Elixir style and optimization issues automatically as part of mix format. The fastest way to see what all it can do you for you is to just try it out in your codebase... but here's a list of a few features to help you decide if you're interested in Styler: -- sorts and organizes `import`/`alias`/`require` and other [module directives](docs/module_directives.md) -- keeps lists, sigils, and even arbitrary code sorted with the `# styler:sort` [comment directive](docs/comment_directives.md) -- automatically creates aliases for repeatedly referenced modules names ([_"alias lifting"_](docs/module_directives.md#alias-lifting)) -- optimizes pipe chains for [readability and performance](docs/pipes.md) -- rewrites strings as sigils when it results in fewer escapes -- auto-fixes [many credo rules](docs/credo.md), meaning you can spend less time fighting with CI +- sorts and organizes `import`,`alias`,`require` and other module directives +- automatically creates aliases for repeatedly referenced modules names (_"alias lifting"_) and makes sure aliases you've defined are being used +- keeps lists, sigils, and even arbitrary code sorted with the `# styler:sort` comment directive +- optimizes pipe chains for readability and performance +- rewrites deprecated Elixir standard library code, speeding adoption of new releases +- auto-fixes many credo rules, meaning you can spend less time fighting with CI + +### Refactoring Mix Tasks + +Styler also includes two experimental refactoring tasks: +- `mix styler.remove_unused`: deletes unused `import|alias|require` nodes that generate compiler warnings +- `mix styler.inline_attrs`: inlines module attributes that have a literal value and are only referenced once, removing unnecessary indirection ## Who is Styler for? -Styler was designed for a **large team (40+ engineers) working in a single codebase. It helps remove fiddly code review comments and removes failed linter CI slowdowns, helping teams get things done faster. Teams in similar situations might appreciate Styler. +> I'm just excited to be on a team that uses Styler and moves on +> +>\- [Amos King](https://github.com/adkron) + +Styler was designed for a large team working in a single codebase (140+ contributors). It helps remove fiddly code review comments and linter CI slowdowns, helping our team get things done faster. Teams in similar situations might appreciate Styler. -Its automations are also extremely valuable for taming legacy elixir codebases or just refactoring in general. Some of its rewrites have inspired code actions in elixir language servers. +Styler has also been extremely valuable for taming legacy Elixir codebases and general refactoring. Some of its rewrites have inspired code actions in Elixir language servers. Conversely, Styler probably _isn't_ a good match for: @@ -42,7 +49,7 @@ Add `:styler` as a dependency to your project's `mix.exs`: ```elixir def deps do [ - {:styler, "~> 1.4", only: [:dev, :test], runtime: false}, + {:styler, "~> 1.11", only: [:dev, :test], runtime: false}, ] end ``` @@ -57,7 +64,7 @@ Then add `Styler` as a plugin to your `.formatter.exs` file And that's it! Now when you run `mix format` you'll also get the benefits of Styler's Stylish Stylings. -**Speed**: Expect the first run to take some time as `Styler` rewrites violations of styles and bottlenecks on disk I/O. Subsequent formats formats won't take noticeably more time. +**Speed**: Expect the first run to take some time as `Styler` rewrites violations of styles and bottlenecks on disk I/O. Subsequent formats won't take noticeably more time. ### Configuration @@ -67,12 +74,14 @@ Styler can be configured in your `.formatter.exs` file [ plugins: [Styler], styler: [ - alias_lifting_exclude: [...] + alias_lifting_exclude: [...], + minimum_supported_elixir_version: "..." ] ] ``` -Styler's only current configuration option is `:alias_lifting_exclude`, which accepts a list of atoms to _not_ lift. See the [Module Directive documentation](docs/module_directives.md#alias-lifting) for more. +* `alias_lifting_exclude`: a list of module names to _not_ lift. See the [Module Directive documentation](docs/module_directives.md#alias-lifting) for more. +* `minimum_supported_elixir_version`: intended for library authors; overrides the Elixir version Styler relies on with respect to some deprecation rewrites. See [Deprecations documentation](docs/deprecations.md#configuration) for more. #### No Credo-Style Enable/Disable @@ -82,11 +91,13 @@ However, Smartrent has a fork of Styler named [Quokka](https://github.com/smartr Ultimately Styler is @adobe's internal tool that we're happy to share with the world. We're delighted if you like it as is, and just as excited if it's a starting point for you to make something even better for yourself. -## WARNING: Styler can change the behaviour of your program! +## WARNING: Styler can change the behaviour of your program -In some cases, this can introduce bugs. It goes without saying, but look over your changes before committing to main :) +While Styler endeavors to never purposefully create bugs, some of its rewrites can introduce them in obscure cases. -A simple example of a way Styler changes the behaviour of code is the following rewrite: +It goes without saying, but look over any changes Styler writes before committing to main. + +A simple example of a way Styler rewrite can introduce a bug is the following case statement: ```elixir # Before: this case statement... @@ -105,19 +116,9 @@ end These programs are not semantically equivalent. The former would raise if `foo` returned any value other than `true` or `false`, while the latter blissfully completes. -However, Styler is about _style_, and the `if` statement is (in our opinion) of much better style. If the exception behaviour was intentional on the code author's part, they should have written the program like this: - -```elixir -case foo do - true -> :ok - false -> :error - other -> raise "expected `true` or `false`, got: #{inspect other}" -end -``` - -Also good style! But Styler assumes that most of the time people just meant the `if` equivalent of the code, and so makes that change. If issues like this bother you, Styler probably isn't the tool you're looking for. +If issues like this bother you, Styler probably isn't the tool you're looking for. -Other ways Styler can change your program: +Other ways Styler _could_ introduce runtime bugs: - [`with` statement rewrites](https://github.com/adobe/elixir-styler/issues/186) - [config file sorting](https://hexdocs.pm/styler/mix_configs.html#this-can-break-your-program) diff --git a/docs/comment_directives.md b/docs/comment_directives.md index 41dc0a63..830c1acc 100644 --- a/docs/comment_directives.md +++ b/docs/comment_directives.md @@ -1,10 +1,8 @@ -## Comment Directives - Comment Directives are a Styler feature that let you instruct Styler to do maintain additional formatting via comments. The plural in the name is optimistic as there's currently only one, but who knows -### `# styler:sort` +## `# styler:sort` Styler can keep static values sorted for your team as part of its formatting pass. To instruct it to do so, replace any `# Please keep this list sorted!` notes you wrote to your teammates with `# styler:sort` @@ -24,7 +22,7 @@ Styler will apply those sorts when they're on the righthand side fo the followin - assignments (eg `x = ~w(a list again)`) - `defstruct` -#### Examples +### Examples **Before** diff --git a/docs/control_flow_macros.md b/docs/control_flow_macros.md index 70a85460..b9c0593e 100644 --- a/docs/control_flow_macros.md +++ b/docs/control_flow_macros.md @@ -46,7 +46,9 @@ case some_http_call() do end ``` -## `if` and `unless` +--------------------------------- + +## `if`/`unless` Styler removes `else: nil` clauses: @@ -130,7 +132,31 @@ end ## `cond` -Styler has only one `cond` statement rewrite: replace 2-clause statements with `if` statements. +Styler enforces the use of `true` as the final clause of a cond statement when it's equivalent. + +```elixir +# before +cond do + a -> b + c -> d + :else -> e +end +# styled +cond do + a -> b + c -> d + true -> e +end + +# This is left unchanged +cond do + a -> b + c -> d + foo -> e +end +``` + +Styler also replaces 2-clause cond statements with `if` statements when possible ```elixir # Given @@ -144,6 +170,12 @@ if a do else c end + +# This is left unchanged, as its behaviour of raising if `foo` is falsey is assumed to be intentional +cond do + a -> b + foo -> c +end ``` ## `with` diff --git a/docs/deprecations.md b/docs/deprecations.md index 45bdbe24..2741a863 100644 --- a/docs/deprecations.md +++ b/docs/deprecations.md @@ -1,29 +1,51 @@ -## Elixir Deprecation Rewrites - Elixir's built-in formatter now does its own rewrites via the `--migrate` flag, but doesn't quite cover every possible automated rewrite on the hard deprecations list. Styler tries to cover the rest! Styler will rewrite deprecations so long as their alternative is available on the given elixir version. In other words, Styler doesn't care what version of Elixir you're using when it applies the ex-1.18 rewrites - all it cares about is that the alternative is valid in your version of elixir. -### elixir `main` +## Configuration + +While most deprecation rewrites rely on the system's Elixir version, that version can be overridden for some rewrites with the `minimum_supported_elixir_version` configuration. For example, to keep Styler from using rewrites that would be incompatible with Elixir 1.15: + +```elixir +# .formatter.exs +[ + plugins: [Styler], + styler: [ + minimum_supported_elixir_version: "1.15.0" + ] +] +``` -https://github.com/elixir-lang/elixir/blob/main/CHANGELOG.md#4-hard-deprecations +Libraries using Styler may be running on a more modern version of Elixir while intending to support older versions. Styler can therefore break a library's minimum supported Elixir version contract when rewriting deprecated code to use more recently added standard library APIs. -These deprecations will be released with Elixir 1.18 +For example, the `to_timeout` rewrite is only valid when running on Elixir 1.17 and greater. If a library supports older versions of Elixir it cannot use that function, and Styler automatically adding that function breaks them. This can be remedied by setting an earlier `minimum_supported_elixir_version`. -#### `List.zip/1` +If you want to keep this configuration in sync with your project's mix.exs, consider something like the following: ```elixir -# Before -List.zip(list) -# Styled -Enum.zip(list) +# .formatter.exs +# Parse SemVer minor elixir version from project configuration +# eg `"~> 1.15"` version requirement will yield `"1.15"` +elixir_minor_version = Regex.run(~r/([\d\.]+)/, Mix.Project.config()[:elixir]) + +[ + plugins: [Styler], + styler: [ + # appending `.0` to the minor version gives us a valid SemVer version string. + minimum_supported_elixir_version: "#{elixir_minor_version}.0" + ] +] ``` -#### `unless` +## 1.20 -This is covered by the Elixir Formatter with the `--migrate` flag, but Styler brings the same transformation to codebases on earlier versions of Elixir. +[1.20 Deprecations](https://github.com/elixir-lang/elixir/blob/main/CHANGELOG.md#4-hard-deprecations) -Rewrite `unless x` to `if !x` +No deprecation rewrites have been added to Styler for 1.20 + +## 1.19 + +[1.19 Deprecations](https://github.com/elixir-lang/elixir/blob/v1.19/CHANGELOG.md#4-hard-deprecations) ### Change Struct Updates to Map Updates @@ -38,17 +60,30 @@ Rewrite `unless x` to `if !x` **WARNING** Double check your diffs to make sure your variable is pattern matching against the same struct if you want to harness 1.19's type checking features. -### 1.18 +## 1.18 -None? +### `List.zip/1` + +```elixir +# Before +List.zip(list) +# Styled +Enum.zip(list) +``` + +### `unless` + +This is covered by the Elixir Formatter with the `--migrate` flag, but Styler brings the same transformation to codebases on earlier versions of Elixir, and insures future uses are automatically rewritten without relying on the flag. + +Rewrite `unless x` to `if !x` -### 1.17 +## 1.17 [1.17 Deprecations](https://hexdocs.pm/elixir/1.17.0/changelog.html#4-hard-deprecations) -- Replace `:timer.units(x)` with the new `to_timeout(unit: x)` for `hours|minutes|seconds` +- Replace `:timer.units(x)` with the new `to_timeout(unit: x)` for `hours|minutes|seconds` (relies on `minimum_supported_elixir_version`) -#### Range Matching Without Step +### Range Matching Without Step ```elixir # Before @@ -62,11 +97,11 @@ def foo(x..y), do: :ok def foo(x..y//_), do: :ok ``` -### 1.16 +## 1.16 [1.16 Deprecations](https://hexdocs.pm/elixir/1.16.0/changelog.html#4-hard-deprecations) -#### `File.stream!/3` `:line` and `:bytes` deprecation +### `File.stream!/3` `:line` and `:bytes` deprecation ```elixir # Before @@ -91,7 +126,7 @@ Date.range(~D[2000-01-01], ~D[1999-01-01]) Date.range(~D[2000-01-01], ~D[1999-01-01], -1) ``` -### 1.15 +## 1.15 [1.15 Deprecations](https://hexdocs.pm/elixir/1.15.0/changelog.html#4-hard-deprecations) diff --git a/docs/general_styles.md b/docs/general_styles.md new file mode 100644 index 00000000..789ba5d6 --- /dev/null +++ b/docs/general_styles.md @@ -0,0 +1,279 @@ +# General Styles + +_(a.k.a. 1-1 or "Single Node" styles)_ + +Function Performance & Readability Optimizations + +Optimizing for either performance or readability, probably both! +These apply to the piped versions as well + +## Strings to Sigils + +Rewrites strings with 4 or more escaped quotes to string sigils with an alternative delimiter. +The delimiter will be one of `" ( { | [ ' < /`, chosen by which would require the fewest escapes, and otherwise preferred in the order listed. + +```elixir +# Before +"{\"errors\":[\"Not Authorized\"]}" +# Styled +~s({"errors":["Not Authorized"]}) +``` + +## Large Base 10 Numbers + +Style base 10 numbers with 5 or more digits to have a `_` every three digits. +Formatter already does this except it doesn't rewrite "typos" like `100_000_0`. + +If you're concerned that this breaks your team's formatting for things like "cents" (like "$100" being written as `100_00`), +consider using a library made for denoting currencies rather than raw elixir integers. + +| Before | After | +|--------|-------| +| `10000 ` | `10_000`| +| `1_0_0_0_0` | `10_000` (elixir's formatter leaves the former as-is)| +| `-543213 ` | `-543_213`| +| `123456789 ` | `123_456_789`| +| `55333.22 ` | `55_333.22`| +| `-123456728.0001 ` | `-123_456_728.0001`| + +## `Enum.into` -> `X.new` + +This rewrite is applied when the collectable is a new map, keyword list, or mapset via `Enum.into/2,3`. + +This is an improvement for the reader, who gets a more natural language expression: "make a new map from enum" vs "enumerate enum and collect its elements into a new map" + +Note that all of the examples below also apply to pipes (`enum |> Enum.into(...)`) + +| Before | After | +|--------|-------| +| `Enum.into(enum, %{})` | `Map.new(enum)`| +| `Enum.into(enum, Map.new())` | `Map.new(enum)`| +| `Enum.into(enum, Keyword.new())` | `Keyword.new(enum)`| +| `Enum.into(enum, MapSet.new())` | `MapSet.new(enum)`| +| `Enum.into(enum, %{}, fn x -> {x, x} end)` | `Map.new(enum, fn x -> {x, x} end)`| +| `Enum.into(enum, [])` | `Enum.to_list(enum)` | +| `Enum.into(enum, [], mapper)` | `Enum.map(enum, mapper)`| + +## Map/Keyword.merge w/ single key literal -> X.put + +`Keyword.merge` and `Map.merge` called with a literal map or keyword argument with a single key are rewritten to the equivalent `put`, a cognitively simpler function. + +```elixir +# Before +Keyword.merge(kw, [key: :value]) +# Styled +Keyword.put(kw, :key, :value) + +# Before +Map.merge(map, %{key: :value}) +# Styled +Map.put(map, :key, :value) + +# Before +Map.merge(map, %{key => value}) +# Styled +Map.put(map, key, value) + +# Before +map |> Map.merge(%{key: value}) |> foo() +# Styled +map |> Map.put(:key, value) |> foo() +``` + +## Map/Keyword.drop w/ single key -> X.delete + +In the same vein as the `merge` style above, `[Map|Keyword].drop/2` with a single key to drop are rewritten to use `delete/2` +```elixir +# Before +Map.drop(map, [key]) +# Styled +Map.delete(map, key) + +# Before +Keyword.drop(kw, [key]) +# Styled +Keyword.delete(kw, key) +``` + +## `Enum.reverse/1` and concatenation -> `Enum.reverse/2` + +`Enum.reverse/2` optimizes a two-step reverse and concatenation into a single step. + +```elixir +# Before +Enum.reverse(foo) ++ bar +# Styled +Enum.reverse(foo, bar) + +# Before +baz |> Enum.reverse() |> Enum.concat(bop) +# Styled +Enum.reverse(baz, bop) +``` + +## `Timex.now/0` -> `DateTime.utc_now/0` + +Timex certainly has its uses, but knowing what stdlib date/time struct is returned by `now/0` is a bit difficult! + +We prefer calling the actual function rather than its rename in Timex, helping the reader by being more explicit. + +This also hews to our internal styleguide's "Don't make one-line helper functions" guidance. + +## `DateModule.compare/2` -> `DateModule.[before?|after?]` + +Again, the goal is readability and maintainability. `before?/2` and `after?/2` were implemented long after `compare/2`, +so it's not unusual that a codebase needs a lot of refactoring to be brought up to date with these new functions. +That's where Styler comes in! + +The examples below use `DateTime.compare/2`, but the same is also done for `NaiveDateTime|Time|Date.compare/2` + +```elixir +# Before +DateTime.compare(start, end_date) == :gt +# Styled +DateTime.after?(start, end_date) + +# Before +DateTime.compare(start, end_date) == :lt +# Styled +DateTime.before?(start, end_date) +``` + +## Remove parenthesis from 0-arity function & macro definitions + +The author of the library disagrees with this style convention :) BUT, the wonderful thing about Styler is it lets you write code how _you_ want to, while normalizing it for reading for your entire team. The most important thing is not having to think about the style, and instead focus on what you're trying to achieve. + +```elixir +# Before +def foo() do +defp foo() do +defmacro foo() do +defmacrop foo() do + +# Styled +def foo do +defp foo do +defmacro foo do +defmacrop foo do +``` + +## Variable matching on the right + +```elixir +# Before +case foo do + bar = %{baz: baz? = true} -> :baz? + opts = [[a = %{}] | _] -> a +end +# Styled: +case foo do + %{baz: true = baz?} = bar -> :baz? + [[%{} = a] | _] = opts -> a +end + +# Before +with {:ok, result = %{}} <- foo, do: result +# Styled +with {:ok, %{} = result} <- foo, do: result + +# Before +def foo(bar = %{baz: baz? = true}, opts = [[a = %{}] | _]), do: :ok +# Styled +def foo(%{baz: true = baz?} = bar, [[%{} = a] | _] = opts), do: :ok +``` + +## Drops superfluous `= _` in pattern matching + +```elixir +# Before +def foo(_ = bar), do: bar +# Styled +def foo(bar), do: bar + +# Before +case foo do + _ = bar -> :ok +end +# Styled +case foo do + bar -> :ok +end +``` + +## Shrink Function Definitions to One Line When Possible + +```elixir +# Before + +def save( + # Socket comment + %Socket{assigns: %{user: user, live_action: :new}} = initial_socket, + # Params comment + params + ), + do: :ok + +# Styled + +# Socket comment +# Params comment +def save(%Socket{assigns: %{user: user, live_action: :new}} = initial_socket, params), do: :ok +``` + +## Assert/Refute Rewrites + +Styler rewrites negated asserts, assertions that things are not nil (that's the same as just asserting the thing!) and refute with binary operators (refute's hard to reason about with anything other than `==` and predicates). + +| before | styled | +|---------------------|-------------------| +| `assert !x` | `refute x` | +| `assert not x` | `refute x` | +| `assert !!x` | `assert x` | +| `assert x != nil` | `assert x` | +| `assert x == nil` | _no change_ | +| `assert is_nil(x)` | _no change_ | +| `assert !is_nil(x)` | `assert x` | +| `assert x not in y` | `refute x in y` | +| **refute negated** | | +| `refute x` | _no change_ | +| `refute !x` | `assert x` | +| `refute not x` | `assert x` | +| `refute x != y` | `assert x == y` | +| `refute x !== y` | `assert x === y` | +| `refute x != nil` | `assert x == nil` | +| `refute x not in y` | `assert x in y` | +| **refute comparison** | | +| `refute x < y` | `assert x >= y` | +| `refute x <= y` | `assert x > y` | +| `refute x > y` | `assert x <= y` | +| `refute x >= y` | `assert x < y` | + +Additionally, styler rewrites some `Enum` functions inside `assert` + +| before | styled | +|-----------------------------------------------|-------------------------------------| +| `assert Enum.find(x, y)` | `assert Enum.any?(x, y)` | +| `assert Enum.member?(y, x)` | `assert x in y` | +| `assert Enum.any?(y, & &1 == x)` | `assert x in y` | +| `assert Enum.any?(y, fn var -> var == x end)` | `assert x in y` | + +## `to_timeout/1` + +- rewrites plural units to singular (plurals are invalid values and runtime errors, but they're oh-so-natural to write) +- steps units up to the next level + +```elixir +# before +to_timeout(second: 60 * m) +# styled +to_timeout(minute: m) +# before +to_timeout(second: 60) +# styled +to_timeout(minute: 1) + +# before +to_timeout(hours: 24 * 1, seconds: 60 * 4) +# styled +to_timeout(day: 1, minute: 4) +``` diff --git a/docs/module_directives.md b/docs/module_directives.md index c385d0b3..aabecf77 100644 --- a/docs/module_directives.md +++ b/docs/module_directives.md @@ -1,38 +1,3 @@ -## Adds Moduledoc - -Adds `@moduledoc false` to modules without a moduledoc unless the module's name ends with one of the following: - -* `Test` -* `Mixfile` -* `MixProject` -* `Controller` -* `Endpoint` -* `Repo` -* `Router` -* `Socket` -* `View` -* `HTML` -* `JSON` - -## Directive Expansion - -Expands `Module.{SubmoduleA, SubmoduleB}` to their explicit forms for ease of searching. - -```elixir -# Before -import Foo.{Bar, Baz, Bop} -alias Foo.{Bar, Baz.A, Bop} - -# After -import Foo.Bar -import Foo.Baz -import Foo.Bop - -alias Foo.Bar -alias Foo.Baz.A -alias Foo.Bop -``` - ## Directive Organization Modules directives are sorted into the following order: @@ -147,6 +112,29 @@ import Foo.Bar alias Foo.Bar ``` +## Directive Expansion + +Expands `Module.{SubmoduleA, SubmoduleB}` to their explicit forms for ease of searching. + +```elixir +# Before +import Foo.{Bar, Baz, Bop} +alias Foo.{Bar, Baz.A, Bop} + +# After +import Foo.Bar +import Foo.Baz +import Foo.Bop + +alias Foo.Bar +alias Foo.Baz.A +alias Foo.Bop +``` + +## Remove Unnecessary / Basic Aliases + +Styler removes root node or single aliases like `alias Foo`, as they accomplish nothing. + ## Alias Lifting When a module with three parts is referenced two or more times, styler creates a new alias for that module and uses it. @@ -194,3 +182,54 @@ You can specify additional modules to exclude from lifting via the `:alias_lifti styler: [alias_lifting_exclude: [:C]], ] ``` + +## Alias Application + +Styler applies aliases in those cases where a developer wrote out a full module name without realizing that the module is already aliased. + +```elixir +# Given +alias A.B +alias A.B.C +alias A.B.C.D, as: X + +A.B.foo() +A.B.C.foo() +A.B.C.D.woo() +C.D.woo() + +# Styled +alias A.B +alias A.B.C +alias A.B.C.D, as: X + +B.foo() +C.foo() +X.woo() +X.woo() +``` + +## Adds `@moduledoc false` + +Adds `@moduledoc false` to modules without a moduledoc. + +Styler does this for two reasons: + +1. Its appearance during code review is a reminder to the author and reviewer that they may want to document the module. This can otherwise be easily forgotten. +2. To normalize the use of `@moduledoc false`, because it's preferable to docstrings which convey no actual information. + + For example: `@moduledoc "The module for functions for interacting with Widgets"` in the `Widget` module is crueler code to have written than just `@moduledoc false`, because including a string gave the reader hope that the author was going to help them comprehend the module. That false hope causes more harm than just saying "Sorry, you're on your own." (aka `@moduledoc false`) + +In conformance with the precedent set by Credo, this `@moduledoc` is not added if the module's name ends with any of the following: + +* `Test` +* `Mixfile` +* `MixProject` +* `Controller` +* `Endpoint` +* `Repo` +* `Router` +* `Socket` +* `View` +* `HTML` +* `JSON` diff --git a/docs/pipes.md b/docs/pipes.md index 8d800984..0174feba 100644 --- a/docs/pipes.md +++ b/docs/pipes.md @@ -1,6 +1,4 @@ -## Pipe Chains - -### Pipe Start +## Pipe Start Styler will ensure that the start of a pipechain is a 0-arity function, a raw value, or a variable. @@ -38,7 +36,7 @@ if_result |> IO.inspect() ``` -### Add parenthesis to function calls in pipes +## Add parenthesis to function calls in pipes ```elixir a |> b |> c |> d @@ -46,27 +44,27 @@ a |> b |> c |> d a |> b() |> c() |> d() ``` -### Remove Unnecessary `then/2` +## `then/2` improvements -When the piped argument is being passed as the first argument to the inner function, there's no need for `then/2`. +### Add `then/2` when defining and calling anonymous functions in pipes ```elixir -a |> then(&f(&1, ...)) |> b() +a |> (fn x -> x end).() |> c() # Styled: -a |> f(...) |> b() +a |> then(fn x -> x end) |> c() ``` -- add parens to function calls `|> fun |>` => `|> fun() |>` +### Remove Redundant `then/2` -### Add `then/2` when defining and calling anonymous functions in pipes +When the piped argument is being passed as the first argument to the inner function, there's no need for `then/2`. ```elixir -a |> (fn x -> x end).() |> c() +a |> then(&f(&1, ...)) |> b() # Styled: -a |> then(fn x -> x end) |> c() +a |> f(...) |> b() ``` -### Piped function optimizations +## Optimizations Two function calls into one! Fewer steps is always nice. @@ -105,8 +103,45 @@ a |> b() |> Stream.map(fun) |> Stream.run() # Styled: a |> b() |> Enum.each(fun) a |> b() |> Enum.each(fun) + +# Given: +a |> Enum.filter(fun) |> List.first() |> ... +a |> Enum.filter(fun) |> List.first(default) |> ... +# Styled: +a |> Enum.find(fun) |> ... +a |> Enum.find(default, fun) |> ... + +# Given: +a |> Enum.sort() |> Enum.reverse() |> ... +a |> Enum.sort(:desc) |> ... + +# Given: +a |> Enum.map(fun) |> Enum.intersperse(separator) |> ... +a |> Enum.map_intersperse(separator, fun) |> ... ``` +### Req Optimizations + +[Req](https://github.com/wojtekmach/req) is a popular HTTP Client. If you aren't using it, you can just ignore this whole section! + +Styler ensures a minimal number of functions are being called when using any Req 1-arity execution functions (`delete get head patch post put request run` and their bangified versions). + +```elixir +# before +keyword |> Req.new() |> Req.merge(opts) |> Req.post!() +# Styled: +Req.post!(keyword, opts) + +# before +foo |> Keyword.merge(opts) |> Req.head() +# Styled: +Req.head(foo, opts) +``` + +**This changes the program's behaviour**, since `Keyword.merge` would overwrite existing values in all cases, whereas `Req` 2-arity functions intelligently deep-merge values for some keys, like `:headers`. + +## Adding & Removing Pipes + ### Unpiping Single Pipes Styler rewrites pipechains with a single pipe to be function calls. Notably, this rule combined with the optimizations rewrites above means some chains with more than one pipe will also become function calls. @@ -130,3 +165,21 @@ d(a |> b |> c) # Styled a |> b() |> c() |> d() ``` + +Styler does not pipe-ify nested function calls if there are no pipes: + +```elixir +# Styler does not change this +d(c(b(a)) +``` + +If you want Styler to do the work of transforming nested function calls into a pipe for you, change the innermost function call to a pipe, then format. Styler will take care of the rest. + +```elixir +# To have Styler change this to pipes +d(c(b(a)) +# You will have to add a single pipe to the innermost function call, like so: +d(c(a |> b)) +# At which point Styler will pipe-ify the entire chain +a |> b() |> c() |> d() +``` diff --git a/docs/styles.md b/docs/styles.md index a7e89b7e..6f814ed5 100644 --- a/docs/styles.md +++ b/docs/styles.md @@ -1,247 +1,13 @@ -# Simple (Single Node) Styles +# Style Table of Contents -Function Performance & Readability Optimizations +Styler performs myriad rewrites, logically broken apart into the following groups: -Optimizing for either performance or readability, probably both! -These apply to the piped versions as well +- [Comment Directives](./comment_directives.md): leave comments to Styler instructing it to perform a task +- [Control Flow Macros](./control_flow_macros.md): styles modifying `case`, `if`, `unless`, `cond` and `with` statements +- [Elixir Deprecations](./deprecations.md): Styles which automate the replacement or updating of code deprecated by new Elixir releases +- [General Styles](./general_styles.md): general simple 1-1 rewrites that require a minimum amount of awareness of the AST +- [Mix Configs](./mix_configs.md): Styler applies order to chaos by organizing mix `config ...` stanzas +- [Module Directives](./module_directives.md): Styles for `alias`, `use`, `import`, `require`, as well as alias lifting and alias application. +- [Pipes](./pipes.md): Styles for the famous Elixir pipe `|>`, including optimizations for piping standard library functions -## Strings to Sigils - -Rewrites strings with 4 or more escaped quotes to string sigils with an alternative delimiter. -The delimiter will be one of `" ( { | [ ' < /`, chosen by which would require the fewest escapes, and otherwise preferred in the order listed. - -```elixir -# Before -"{\"errors\":[\"Not Authorized\"]}" -# Styled -~s({"errors":["Not Authorized"]}) -``` - -## Large Base 10 Numbers - -Style base 10 numbers with 5 or more digits to have a `_` every three digits. -Formatter already does this except it doesn't rewrite "typos" like `100_000_0`. - -If you're concerned that this breaks your team's formatting for things like "cents" (like "$100" being written as `100_00`), -consider using a library made for denoting currencies rather than raw elixir integers. - -| Before | After | -|--------|-------| -| `10000 ` | `10_000`| -| `1_0_0_0_0` | `10_000` (elixir's formatter leaves the former as-is)| -| `-543213 ` | `-543_213`| -| `123456789 ` | `123_456_789`| -| `55333.22 ` | `55_333.22`| -| `-123456728.0001 ` | `-123_456_728.0001`| - -## `Enum.into` -> `X.new` - -This rewrite is applied when the collectable is a new map, keyword list, or mapset via `Enum.into/2,3`. - -This is an improvement for the reader, who gets a more natural language expression: "make a new map from enum" vs "enumerate enum and collect its elements into a new map" - -Note that all of the examples below also apply to pipes (`enum |> Enum.into(...)`) - -| Before | After | -|--------|-------| -| `Enum.into(enum, %{})` | `Map.new(enum)`| -| `Enum.into(enum, Map.new())` | `Map.new(enum)`| -| `Enum.into(enum, Keyword.new())` | `Keyword.new(enum)`| -| `Enum.into(enum, MapSet.new())` | `MapSet.new(enum)`| -| `Enum.into(enum, %{}, fn x -> {x, x} end)` | `Map.new(enum, fn x -> {x, x} end)`| -| `Enum.into(enum, [])` | `Enum.to_list(enum)` | -| `Enum.into(enum, [], mapper)` | `Enum.map(enum, mapper)`| - -## Map/Keyword.merge w/ single key literal -> X.put - -`Keyword.merge` and `Map.merge` called with a literal map or keyword argument with a single key are rewritten to the equivalent `put`, a cognitively simpler function. - -```elixir -# Before -Keyword.merge(kw, [key: :value]) -# Styled -Keyword.put(kw, :key, :value) - -# Before -Map.merge(map, %{key: :value}) -# Styled -Map.put(map, :key, :value) - -# Before -Map.merge(map, %{key => value}) -# Styled -Map.put(map, key, value) - -# Before -map |> Map.merge(%{key: value}) |> foo() -# Styled -map |> Map.put(:key, value) |> foo() -``` - -## Map/Keyword.drop w/ single key -> X.delete - -In the same vein as the `merge` style above, `[Map|Keyword].drop/2` with a single key to drop are rewritten to use `delete/2` -```elixir -# Before -Map.drop(map, [key]) -# Styled -Map.delete(map, key) - -# Before -Keyword.drop(kw, [key]) -# Styled -Keyword.delete(kw, key) -``` - -## `Enum.reverse/1` and concatenation -> `Enum.reverse/2` - -`Enum.reverse/2` optimizes a two-step reverse and concatenation into a single step. - -```elixir -# Before -Enum.reverse(foo) ++ bar -# Styled -Enum.reverse(foo, bar) - -# Before -baz |> Enum.reverse() |> Enum.concat(bop) -# Styled -Enum.reverse(baz, bop) -``` - -# Before -baz |> Enum.reverse() |> Enum.concat(bop) -# Styled -Enum.reverse(baz, bop) -``` - -## `Timex.now/0` -> `DateTime.utc_now/0` - -Timex certainly has its uses, but knowing what stdlib date/time struct is returned by `now/0` is a bit difficult! - -We prefer calling the actual function rather than its rename in Timex, helping the reader by being more explicit. - -This also hews to our internal styleguide's "Don't make one-line helper functions" guidance. - -## `DateModule.compare/2` -> `DateModule.[before?|after?]` - -Again, the goal is readability and maintainability. `before?/2` and `after?/2` were implemented long after `compare/2`, -so it's not unusual that a codebase needs a lot of refactoring to be brought up to date with these new functions. -That's where Styler comes in! - -The examples below use `DateTime.compare/2`, but the same is also done for `NaiveDateTime|Time|Date.compare/2` - -```elixir -# Before -DateTime.compare(start, end_date) == :gt -# Styled -DateTime.after?(start, end_date) - -# Before -DateTime.compare(start, end_date) == :lt -# Styled -DateTime.before?(start, end_date) -``` - -## Implicit `try` - -Styler will rewrite functions whose entire body is a try/do to instead use the implicit try syntax, per Credo's `Credo.Check.Readability.PreferImplicitTry` - -```elixir -# before -def foo do - try do - throw_ball() - catch - :ball -> :caught - end -end - -# Styled: -def foo do - throw_ball() -catch - :ball -> :caught -end -``` - -## Remove parenthesis from 0-arity function & macro definitions - -The author of the library disagrees with this style convention :) BUT, the wonderful thing about Styler is it lets you write code how _you_ want to, while normalizing it for reading for your entire team. The most important thing is not having to think about the style, and instead focus on what you're trying to achieve. - -```elixir -# Before -def foo() do -defp foo() do -defmacro foo() do -defmacrop foo() do - -# Styled -def foo do -defp foo do -defmacro foo do -defmacrop foo do -``` - -## Variable matching on the right - -```elixir -# Before -case foo do - bar = %{baz: baz? = true} -> :baz? - opts = [[a = %{}] | _] -> a -end -# Styled: -case foo do - %{baz: true = baz?} = bar -> :baz? - [[%{} = a] | _] = opts -> a -end - -# Before -with {:ok, result = %{}} <- foo, do: result -# Styled -with {:ok, %{} = result} <- foo, do: result - -# Before -def foo(bar = %{baz: baz? = true}, opts = [[a = %{}] | _]), do: :ok -# Styled -def foo(%{baz: true = baz?} = bar, [[%{} = a] | _] = opts), do: :ok -``` - -## Drops superfluous `= _` in pattern matching - -```elixir -# Before -def foo(_ = bar), do: bar -# Styled -def foo(bar), do: bar - -# Before -case foo do - _ = bar -> :ok -end -# Styled -case foo do - bar -> :ok -end -``` - -## Shrink Function Definitions to One Line When Possible - -```elixir -# Before - -def save( - # Socket comment - %Socket{assigns: %{user: user, live_action: :new}} = initial_socket, - # Params comment - params - ), - do: :ok - -# Styled - -# Socket comment -# Params comment -def save(%Socket{assigns: %{user: user, live_action: :new}} = initial_socket, params), do: :ok -``` +Finally, if you're using Credo [see our documentation](./credo.md) about rules that can be disabled in Credo because Styler automatically enforces them for you, saving a modicum of CI time. diff --git a/lib/alias_env.ex b/lib/alias_env.ex index 65029bc9..a04d9a4e 100644 --- a/lib/alias_env.ex +++ b/lib/alias_env.ex @@ -1,10 +1,20 @@ +# Copyright 2024 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + defmodule Styler.AliasEnv do @moduledoc """ A datastructure for maintaining something like compiler alias state when traversing AST. Not anywhere as correct as what the compiler gives us, but close enough for open source work. - A alias env is a map from an alias's `as` to its resolution in a context. + An alias env is a map from an alias's `as` to its resolution in a context. Given the ast for @@ -16,40 +26,78 @@ defmodule Styler.AliasEnv do """ def define(env \\ %{}, ast) - def define(env, asts) when is_list(asts), do: Enum.reduce(asts, env, &define(&2, &1)) + def define(env, {:alias, _, [{:__aliases__, _, aliases}]}), do: define(env, aliases, List.last(aliases)) + def define(env, {:alias, _, [{:__aliases__, _, aliases}, [{_, {:__aliases__, _, [as]}}]]}), do: define(env, aliases, as) + # `alias __MODULE__` or other oddities i'm not bothering to get right + def define(env, {:alias, _, _}), do: env - def define(env, {:alias, _, aliases}) do - case aliases do - [{:__aliases__, _, aliases}] -> define(env, aliases, List.last(aliases)) - [{:__aliases__, _, aliases}, [{_as, {:__aliases__, _, [as]}}]] -> define(env, aliases, as) - # `alias __MODULE__` or other oddities i'm not bothering to get right - _ -> env - end - end + defp define(env, modules, as), do: Map.put(env, as, expand(env, modules)) + + @doc """ + Lengthens an alias to its full name, if its first name is defined in the environment" - defp define(env, modules, as), do: Map.put(env, as, do_expand(env, modules)) + Useful for transforming the ast for code like: + alias Bar.Baz.Foo #<- given the env with this alias + Foo.Woo.Cool # <- ast + + to the ast for code like: + + alias Bar.Baz.Foo + Bar.Baz.Foo.Woo.Cool + """ # no need to traverse ast if there are no aliases - def expand(env, ast) when map_size(env) == 0, do: ast + def expand_ast(env, ast) when map_size(env) == 0, do: ast - def expand(env, ast) do + def expand_ast(env, ast) do Macro.prewalk(ast, fn - {:__aliases__, meta, modules} -> {:__aliases__, meta, do_expand(env, modules)} + {:__aliases__, meta, modules} -> {:__aliases__, meta, expand(env, modules)} ast -> ast end) end - # if the list of modules is itself already aliased, dealias it with the compound alias - # given: - # alias Foo.Bar - # Bar.Baz.Bop.baz() - # - # lifting Bar.Baz.Bop should result in: - # alias Foo.Bar - # alias Foo.Bar.Baz.Bop - # Bop.baz() - defp do_expand(env, [first | rest] = modules) do + @doc """ + Expands modules from env (wow that was helpful). + + Using the examples from `expand_ast`, this works roughly like so: + + > expand(%{Foo: [Bar, Baz, Foo]}, [Foo, Woo, Cool]) + => [Bar, Baz, Foo, Woo, Cool] + > expand(%{}, [No, Alias, For, Me]) + => [No, Alias, For, Me] + """ + def expand(env, [first | rest] = modules) do if dealias = env[first], do: dealias ++ rest, else: modules end + + @doc """ + An inverted AliasEnv is useful for translating a module to its alias, if one existed in the env + + In the case that a module is aliased multiple times, the inverted env will only keep the final alias as lexically sorted + """ + def invert(env) do + # It's a bit of a bummer to do the extra group_by out of caution that this 1-off mistake happens, + # but ultimately we're usually working with a small list so performance costs are negligible + env + |> Enum.group_by(fn {_, v} -> v end, fn {k, _} -> k end) + |> Map.new(fn + {modules, [as]} -> + {modules, as} + + # someone has something goofy going on, aliasing the same module with multiple names + # alias A.B.C + # alias A.B.C, as: Bar + # alias A.B.C, as: Foo + # we'll choose the one that comes last lexically, which will be the alpha-sorted last entry that isn't the default as + {modules, multiple_as} -> + default_as = List.last(modules) + # being clever - rather than rejecting the default up front and doing an extra list-traversal, + # just sort things and if the default comes first, grab the second element + case Enum.sort(multiple_as, :desc) do + [^default_as, last_as | _] -> {modules, last_as} + [last_as | _] -> {modules, last_as} + end + end) + end end diff --git a/lib/mix/tasks/inline_attributes.ex b/lib/mix/tasks/inline_attributes.ex new file mode 100644 index 00000000..3d322e7e --- /dev/null +++ b/lib/mix/tasks/inline_attributes.ex @@ -0,0 +1,57 @@ +# Copyright 2025 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + +defmodule Mix.Tasks.Styler.InlineAttrs do + @shortdoc "EXPERIMENTAL: inlines module attributes with literal values that are only referenced once" + @moduledoc """ + WARNING: EXPERIMENTAL | Inlines module attributes that are only referenced once in their module. + + **This is known to create invalid code.** It's far from perfect. + It can still be a helpful first step in refactoring though. + + Formats files with a currently hard-coded length of 122. + + **Usage**: + + mix styler.inline_attrs [... additional file paths] + + mix styler.inline_attrs path/to/my/file.ex path/to/another_file.ex + + ## Example: + + # This ... + defmodule A do + @non_literal_attr Application.compile_env(...) + @literal_value_with_only_one_reference :my_key + + def foo(), do: Application.get_env(:my_app, @literal_value_with_only_one_reference) + end + + # Becomes this + defmodule A do + @non_literal_attr Application.compile_env(...) + + def foo(), do: Application.get_env(:my_app, :my_key) + end + """ + + use Mix.Task + + alias Styler.Zipper + + @impl Mix.Task + def run(files) do + for file <- files do + {ast, comments} = file |> File.read!() |> Styler.string_to_ast(file) + {{ast, _}, _} = ast |> Zipper.zip() |> Zipper.traverse_while(nil, &Styler.Style.InlineAttrs.run/2) + File.write!(file, Styler.ast_to_string(ast, comments)) + end + end +end diff --git a/lib/mix/tasks/remove_unused.ex b/lib/mix/tasks/remove_unused.ex new file mode 100644 index 00000000..60ef7920 --- /dev/null +++ b/lib/mix/tasks/remove_unused.ex @@ -0,0 +1,78 @@ +# Copyright 2025 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + +defmodule Mix.Tasks.Styler.RemoveUnused do + @shortdoc "EXPERIMENTAL: uses unused import/alias/require compiler warnings to remove those lines" + @moduledoc """ + WARNING: EXPERIMENTAL | Removes unused import/alias/require statements by compiling the app and parsing warnings, then deleting the specified lines. + + Usage: + + mix styler.remove_unused + """ + + use Mix.Task + + alias Mix.Shell.IO + + @impl Mix.Task + def run(_) do + # Warnings come over stderr, so gotta redirect + {output, _} = System.cmd("mix", ~w(compile --all-warnings), stderr_to_stdout: true) + + if output =~ "warning: unused" do + IO.info("Removing unused import/alias/require lines...\n") + + output + |> String.split("\n\n") + |> Stream.map(&Regex.run(~r/warning\: unused (alias|require|import).* (.*\.exs?):(\d+)\:/s, &1)) + |> Stream.filter(& &1) + |> Stream.map(fn [_full_message, _require_or_alias, file, line] -> + file = + if File.exists?(file) do + file + else + [umbrella_corrected] = Path.wildcard("apps/*/#{file}") + umbrella_corrected + end + + {file, String.to_integer(line)} + end) + |> Enum.group_by(fn {file, _} -> file end, fn {_, line} -> line end) + |> Enum.sort_by(fn {file, _} -> file end) + |> Enum.each(fn {file, lines} -> + contents = file |> File.read!() |> String.split("\n") + + IO.info("==> #{file}") + + lines + |> Enum.sort() + |> Enum.each(&IO.info("#{&1}: #{Enum.at(contents, &1 - 1)}")) + + contents = + lines + |> Enum.sort(:desc) + |> Enum.reduce(contents, &List.delete_at(&2, &1 - 1)) + |> Enum.join("\n") + + File.write!(file, contents) + IO.info("") + end) + + IO.info("Running `mix format` to remove any excess newlines.") + + Mix.Task.run("format") + + IO.info("Done.") + else + IO.info("No \"unused\" warnings detected, no work to do.") + end + end +end diff --git a/lib/style.ex b/lib/style.ex index 184a902c..c46437e4 100644 --- a/lib/style.ex +++ b/lib/style.ex @@ -194,8 +194,6 @@ defmodule Styler.Style do end @doc "Sets the nodes' meta line and comments' line numbers to fit the ordering of the nodes list." - # TODO this doesn't grab comments which are floating as their own paragrpah, unconnected to a node - # they'll just be left floating where they were, then mangled with the re-ordered comments.. def order_line_meta_and_comments(nodes, comments, first_line) do {nodes, shifted_comments, comments, _line} = Enum.reduce(nodes, {[], [], comments, first_line}, fn node, {n_acc, c_acc, comments, move_to_line} -> @@ -244,19 +242,19 @@ defmodule Styler.Style do comments |> Enum.reverse() |> comments_for_lines(start_line, last_line, [], []) end - defp comments_for_lines([%{line: line} = comment | rev_comments], start, last, match, acc) do + defp comments_for_lines([%{line: line, previous_eol_count: eol} = comment | rev_comments], start, last, matches, misses) do cond do # after our block - no match - line > last -> comments_for_lines(rev_comments, start, last, match, [comment | acc]) + line > last -> comments_for_lines(rev_comments, start, last, matches, [comment | misses]) # after start, before last -- it's a match! - line >= start -> comments_for_lines(rev_comments, start, last, [comment | match], acc) + line >= start -> comments_for_lines(rev_comments, start, last, [comment | matches], misses) # this is a comment immediately before start, which means it's modifying this block... # we count that as a match, and look above it to see if it's a multiline comment - line == start - 1 -> comments_for_lines(rev_comments, start - 1, last, [comment | match], acc) + line == start - 1 -> comments_for_lines(rev_comments, start - eol, last, [comment | matches], misses) # comment before start - we've thus iterated through all comments which could be in our range - true -> {match, Enum.reverse(rev_comments, [comment | acc])} + true -> {matches, Enum.reverse(rev_comments, [comment | misses])} end end - defp comments_for_lines([], _, _, match, acc), do: {match, acc} + defp comments_for_lines([], _, _, matches, misses), do: {matches, misses} end diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index ff951656..66304718 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -43,10 +43,63 @@ defmodule Styler.Style.Blocks do # end # end - # Credo.Check.Refactor.CondStatements - def run({{:cond, _, [[{_, [{:->, _, [[head], a]}, {:->, _, [[{:__block__, _, [truthy]}], b]}]}]]}, _} = zipper, ctx) - when is_atom(truthy) and truthy not in [nil, false], - do: if_ast(zipper, head, a, b, ctx) + # case statements with 1 clause: thanks 🤖! + # + # ideally rewrite to use `=`, but can't do that when there's a `when` clause + def run({{:case, _, [_, [{_, [{:->, _, [[{:when, _, _} | _] | _]}]}]]}, _} = zipper, ctx), do: {:cont, zipper, ctx} + # + def run({{:case, m, [head, [{_, [{:->, _, [[lhs], rhs]}]}]]}, _} = zipper, ctx) do + rhs = + case rhs do + {:__block__, _, children} -> children + node -> [node] + end + + zipper = + case Zipper.up(zipper) do + {{:=, am, [parent_lhs, _case_statement_rhs]}, _} = zipper -> + # this was a `x = case head, do: (lhs -> rhs)`. make it `x = lhs = head; x = rhs` + meta = [line: am[:line]] + # change the if there are multiple clauses in the case, have the final one be the new rhs of parent_lhs + # the meta for the final equality has an incorrect line, but it shouldn't mess with comments so leaving it be + siblings = List.update_at(rhs, -1, &{:=, meta, [parent_lhs, &1]}) + + zipper + |> Zipper.replace({:=, meta, [lhs, head]}) + |> Zipper.insert_siblings(siblings) + + _ -> + zipper + |> Style.find_nearest_block() + |> Zipper.replace({:=, [line: m[:line]], [lhs, head]}) + |> Zipper.insert_siblings(rhs) + end + + {:cont, zipper, ctx} + end + + def run({{:cond, _, [[{do_, clauses}]]}, _} = zipper, ctx) do + # ensure all final `atom -> final_clause` use `true` for consistency. + # `:else` is cute but consistency is all. + rewrite_literal_to_true = fn + {:->, am, [[{:__block__, bm, [truthy]}], body]} when truthy not in [nil, false] -> + {:->, am, [[{:__block__, bm, [true]}], body]} + + # Surely this never happens buuuuut? + # %{} ->; {} -> + {:->, am, [[{literal, bm, _}], body]} when literal in [:{}, :%{}] -> + {:->, am, [[{:__block__, bm, [true]}], body]} + + other -> + other + end + + case List.update_at(clauses, -1, rewrite_literal_to_true) do + # # Credo.Check.Refactor.CondStatements + [{:->, _, [[head], a]}, {:->, _, [[{:__block__, _, [true]}], b]}] -> if_ast(zipper, head, a, b, ctx) + clauses -> {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} + end + end # Credo.Check.Readability.WithSingleClause # rewrite `with success <- single_statement do body else ...elses end` diff --git a/lib/style/comment_directives.ex b/lib/style/comment_directives.ex index c05aa0bc..c5f0e3e8 100644 --- a/lib/style/comment_directives.ex +++ b/lib/style/comment_directives.ex @@ -101,6 +101,11 @@ defmodule Styler.Style.CommentDirectives do {{:@, m, [{a, am, [assignment]}]}, comments} end + defp sort({:"::", m, [name, typespec]}, comments) do + {typespec, comments} = sort(typespec, comments) + {{:"::", m, [name, typespec]}, comments} + end + defp sort({key, value}, comments) do {value, comments} = sort(value, comments) {{key, value}, comments} diff --git a/lib/style/configs.ex b/lib/style/configs.ex index a5379897..d83b4474 100644 --- a/lib/style/configs.ex +++ b/lib/style/configs.ex @@ -48,9 +48,8 @@ defmodule Styler.Style.Configs do end def run({{:config, cfm, [_, _ | _]} = config, zm}, %{mix_config?: true, comments: comments} = ctx) do - {l, p, r} = zm # all of these list are reversed due to the reduce - {configs, assignments, rest} = accumulate(r, [], []) + {configs, assignments, rest} = accumulate(zm.r, [], []) # @TODO # okay so comments between nodes that we moved....... # lets just push them out of the way (???). so @@ -93,9 +92,9 @@ defmodule Styler.Style.Configs do {nodes, comments} end - [config | left_siblings] = Enum.reverse(nodes, l) + [config | left_siblings] = Enum.reverse(nodes, zm.l) - {:skip, {config, {left_siblings, p, rest}}, %{ctx | comments: comments}} + {:skip, {config, %{zm | l: left_siblings, r: rest}}, %{ctx | comments: comments}} end def run(zipper, %{config?: true} = ctx) do diff --git a/lib/style/deprecations.ex b/lib/style/deprecations.ex index 824a88e6..7fdd309e 100644 --- a/lib/style/deprecations.ex +++ b/lib/style/deprecations.ex @@ -59,9 +59,14 @@ defmodule Styler.Style.Deprecations do end if Version.match?(System.version(), ">= 1.17.0-dev") do - for {erl, ex} <- [hours: :hour, minutes: :minute, seconds: :second] do - defp style({{:., _, [{:__block__, _, [:timer]}, unquote(erl)]}, fm, [x]}), - do: {:to_timeout, fm, [[{{:__block__, [format: :keyword, line: fm[:line]], [unquote(ex)]}, x}]]} + @to_timeout_vsn Version.parse!("1.17.0-dev") + defp style({{:., _, [{:__block__, _, [:timer]}, unit]}, fm, [x]} = node) when unit in ~w(hours minutes seconds)a do + if Styler.Config.version_compatible?(@to_timeout_vsn) do + unit = unit |> Atom.to_string() |> String.trim_trailing("s") |> String.to_atom() + {:to_timeout, fm, [[{{:__block__, [format: :keyword, line: fm[:line]], [unit]}, x}]]} + else + node + end end end diff --git a/lib/style/inline_attrs.ex b/lib/style/inline_attrs.ex new file mode 100644 index 00000000..55be8ccd --- /dev/null +++ b/lib/style/inline_attrs.ex @@ -0,0 +1,73 @@ +# Copyright 2025 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + +defmodule Styler.Style.InlineAttrs do + @moduledoc false + + alias Styler.Style + alias Styler.Zipper + + def run(zipper, ctx) do + if defstructz = Zipper.find(zipper, &match?({:defmodule, _, _}, &1)) do + body_zipper = defstructz |> Zipper.down() |> Zipper.right() |> Zipper.down() |> Zipper.down() |> Zipper.right() + + {literal_attrs, others} = + body_zipper + |> Zipper.children() + |> Enum.split_with(fn + {:@, _, [{_name, _, [{:to_timeout, _, values}]}]} -> quoted_literal?(values) + {:@, _, [{name, _, values}]} -> name not in ~w(doc impl moduledoc)a and quoted_literal?(values) + _ -> false + end) + + {_, hits} = + Macro.prewalk(others, %{}, fn + {:@, _, [{name, _, _}]} = ast, acc -> + def = Enum.find(literal_attrs, &match?({:@, _, [{^name, _, _}]}, &1)) + val = def && {def, ast} + {ast, Map.update(acc, name, val, fn _ -> false end)} + + ast, acc -> + {ast, acc} + end) + + zipper = + hits + |> Enum.filter(fn {_, v} -> v end) + |> Enum.reduce(body_zipper, fn {_, {def, ast}}, zipper -> + {_, _, [{_, _, [value]}]} = def + replacement = Style.set_line(value, Style.meta(ast)[:line]) + + zipper + |> Zipper.find(&match?(^def, &1)) + |> Zipper.remove() + |> Zipper.find(&match?(^ast, &1)) + |> Zipper.replace(replacement) + |> Zipper.top() + end) + + {:halt, zipper, ctx} + else + {:halt, zipper, ctx} + end + end + + # Can't rely on `Macro.quoted_literal?` up front because we wrapped our literals :/ + # This function is not complete, but it's good enough for the needs here. + defp quoted_literal?(value) when is_list(value) or is_map(value) do + Enum.all?(value, fn + {k, v} -> quoted_literal?(k) and quoted_literal?(v) + value -> quoted_literal?(value) + end) + end + + defp quoted_literal?({:__block__, _, [value]}), do: quoted_literal?(value) + defp quoted_literal?(value), do: Macro.quoted_literal?(value) +end diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index 59dd153a..fbbcf67b 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -48,9 +48,60 @@ defmodule Styler.Style.ModuleDirectives do @directives ~w(alias import require use)a @attr_directives ~w(moduledoc shortdoc behaviour)a - @defstruct ~w(schema embedded_schema defstruct)a - def run({{:defmodule, _, children}, _} = zipper, ctx) do + @env %{ + shortdoc: [], + moduledoc: [], + behaviour: [], + use: [], + import: [], + alias: [], + require: [], + nondirectives: [], + alias_env: %{}, + attrs: MapSet.new() + } + + # module directives typically doesn't do anything until it sees a module (typical .ex file) or a directive (like a snippet) + # however, if we're in a snippet with no directives we'll never do any work! + # so, we fast-forward the traversal looking for an interesting node. + # if we find one, we'll mark that this file is interesting and proceed as normal from there. + # if we _dont_ find one, then we're likely in a snippet with no typical directives. + # in that case, all that's left is to apply alias lifting and halt. + def run(zipper, %{__MODULE__ => true} = ctx), do: do_run(zipper, ctx) + + def run({node, nil} = zipper, ctx) do + if interesting_zipper = Zipper.find(zipper, &match?({x, _, _} when x in [:defmodule, :@ | @directives], &1)) do + do_run(interesting_zipper, Map.put(ctx, __MODULE__, true)) + else + # there's no defmodules or aliasy things - see if we can do some alias lifting? + case lift_aliases(%{@env | nondirectives: [node]}) do + %{alias: []} -> + {:halt, zipper, ctx} + + %{alias: aliases, nondirectives: [node]} -> + # This is a line handler unique to this situation. + # Nowhere else do we create nodes that we know will go before all existing code in the document. + # All we need to do is set those aliases to have the appropriate lines, + # then bump the line of everything else in the document by the number of aliases + 1! + aliases = Enum.with_index(aliases, fn node, i -> Style.set_line(node, i + 1) end) + shift = Enum.count(aliases) + 1 + comments = Enum.map(ctx.comments, &%{&1 | line: &1.line + shift}) + node = Style.shift_line(node, shift) + + zipper = + case Zipper.replace(zipper, node) do + {{:__block__, _, _}, _} = block_zipper -> Zipper.insert_children(block_zipper, aliases) + # this snippet was a single element, eg just one big map in a credo config file. + non_block_zipper -> Zipper.prepend_siblings(non_block_zipper, aliases) + end + + {:halt, zipper, %{ctx | comments: comments}} + end + end + end + + defp do_run({{:defmodule, _, children}, _} = zipper, ctx) do [name, [{{:__block__, do_meta, [:do]}, _body}]] = children if do_meta[:format] == :keyword do @@ -86,14 +137,14 @@ defmodule Styler.Style.ModuleDirectives do {:skip, zipper, ctx} else - run(body_zipper, ctx) + do_run(body_zipper, ctx) end end end end # Style directives inside of snippets or function defs. - def run({{directive, _, children}, _} = zipper, ctx) when directive in @directives and is_list(children) do + defp do_run({{directive, _, children}, _} = zipper, ctx) when directive in @directives and is_list(children) do # Need to be careful that we aren't getting false positives on variables or fns like `def import(foo)` or `alias = 1` case Style.ensure_block_parent(zipper) do {:ok, zipper} -> {:skip, zipper |> Zipper.up() |> organize_directives(), ctx} @@ -102,66 +153,43 @@ defmodule Styler.Style.ModuleDirectives do end end - # puts `@derive` before `defstruct` etc, fixing compiler warnings - def run({{:@, _, [{:derive, _, _}]}, _} = zipper, ctx) do - case Style.ensure_block_parent(zipper) do - {:ok, {derive, {l, p, r}}} -> - previous_defstruct = - l - |> Stream.with_index() - |> Enum.find_value(fn - {{struct_def, meta, _}, index} when struct_def in @defstruct -> {meta[:line], index} - _ -> nil - end) - - if previous_defstruct do - {defstruct_line, defstruct_index} = previous_defstruct - derive = Style.set_line(derive, defstruct_line - 1) - left_siblings = List.insert_at(l, defstruct_index + 1, derive) - {:skip, Zipper.remove({derive, {left_siblings, p, r}}), ctx} - else - {:cont, zipper, ctx} - end - - :error -> - {:cont, zipper, ctx} - end - end - - def run(zipper, ctx), do: {:cont, zipper, ctx} + defp do_run(zipper, ctx), do: {:cont, zipper, ctx} - # a dynamic module name, like `defmodule my_variable do ... end` + # Don't auto-add `@moduledoc false`; leave moduledoc decisions to authors. defp moduledoc(_), do: nil - @acc %{ - shortdoc: [], - moduledoc: [], - behaviour: [], - use: [], - import: [], - alias: [], - require: [], - nondirectives: [], - dealiases: %{}, - attrs: MapSet.new(), - attr_lifts: [] - } - - defp lift_module_attrs({_node, _, _} = ast, %{attrs: attrs} = acc) do - if Enum.empty?(attrs) do - {ast, acc} + # pops module attributes this directive depends on out of `nondirectives`, + # returning their definitions in a list alongside the updated accumulator + # + # naively grouping module attributes in nondirectives can break a common pattern where an attribute is referenced by + # `use` or `@moduledoc` clauses, which then get moved above the attributes they reference. + # this function finds and returns those dependencies, allowing the caller to keep them above the dependent directive + defp split_depended_attributes({_, _, _} = directive, %{attrs: attrs, nondirectives: nondirectives} = acc) do + if MapSet.size(attrs) == 0 do + {[], acc} else - Macro.prewalk(ast, acc, fn - {:@, _m, [{attr, _, _} = var]} = ast, acc -> + directive + |> Macro.prewalk({[], acc}, fn + {:@, _, [{attr, _, _}]} = ast, {prepends, acc} -> if attr in attrs do - {var, %{acc | attr_lifts: [attr | acc.attr_lifts]}} + {definitions, nondirectives} = Enum.split_with(nondirectives, &match?({:@, _, [{^attr, _, _}]}, &1)) + acc = %{acc | nondirectives: nondirectives} + + recursed = + Enum.reduce(definitions, {definitions ++ prepends, acc}, fn ast, {prepends, acc} -> + {definitions, acc} = split_depended_attributes(ast, acc) + {prepends ++ definitions, acc} + end) + + {ast, recursed} else - {ast, acc} + {ast, {prepends, acc}} end ast, acc -> {ast, acc} end) + |> elem(1) end end @@ -169,24 +197,25 @@ defmodule Styler.Style.ModuleDirectives do acc = parent |> Zipper.children() - |> Enum.reduce(@acc, fn + |> Enum.reduce(@env, fn {:@, _, [{attr_directive, _, _}]} = ast, acc when attr_directive in @attr_directives -> - # attr_directives are moved above aliases, so we need to dealias them - {ast, acc} = acc.dealiases |> AliasEnv.expand(ast) |> lift_module_attrs(acc) - %{acc | attr_directive => [ast | acc[attr_directive]]} + # attr_directives are moved above aliases, so we need to expand them + ast = AliasEnv.expand_ast(acc.alias_env, ast) + {prepends, acc} = split_depended_attributes(ast, acc) + %{acc | attr_directive => [ast | prepends ++ acc[attr_directive]]} {:@, _, [{attr, _, _}]} = ast, acc -> %{acc | nondirectives: [ast | acc.nondirectives], attrs: MapSet.put(acc.attrs, attr)} {directive, _, _} = ast, acc when directive in @directives -> - {ast, acc} = lift_module_attrs(ast, acc) + {prepends, acc} = split_depended_attributes(ast, acc) ast = expand(ast) - # import and used get hoisted above aliases, so need to dealias - ast = if directive in ~w(import use)a, do: AliasEnv.expand(acc.dealiases, ast), else: ast - dealiases = if directive == :alias, do: AliasEnv.define(acc.dealiases, ast), else: acc.dealiases + # import and use get hoisted above aliases, so need to expand them + ast = if directive in ~w(import use)a, do: AliasEnv.expand_ast(acc.alias_env, ast), else: ast + alias_env = if directive == :alias, do: AliasEnv.define(acc.alias_env, ast), else: acc.alias_env # the reverse accounts for `expand` putting things in reading order, whereas we're accumulating in reverse - %{acc | directive => Enum.reverse(ast, acc[directive]), dealiases: dealiases} + %{acc | directive => Enum.reverse(ast, prepends ++ acc[directive]), alias_env: alias_env} ast, acc -> %{acc | nondirectives: [ast | acc.nondirectives]} @@ -196,43 +225,14 @@ defmodule Styler.Style.ModuleDirectives do {:moduledoc, []} -> {:moduledoc, List.wrap(moduledoc)} {:use, uses} -> {:use, uses |> Enum.reverse() |> Style.reset_newlines()} {directive, to_sort} when directive in ~w(behaviour import alias require)a -> {directive, sort(to_sort)} - {:dealiases, d} -> {:dealiases, d} + {:alias_env, d} -> {:alias_env, d} {k, v} -> {k, Enum.reverse(v)} end) + |> redefine_alias_env() |> lift_aliases() - |> Map.update!(:moduledoc, &Style.reset_newlines(&1)) - - # Not happy with it, but this does the work to move module attribute assignments above the module or quote or whatever - # Given that it'll only be run once and not again, i'm okay with it being inefficient - {acc, parent} = - if Enum.any?(acc.attr_lifts) do - lifts = acc.attr_lifts - - nondirectives = - Enum.map(acc.nondirectives, fn - {:@, m, [{attr, am, _}]} = ast -> if attr in lifts, do: {:@, m, [{attr, am, [{attr, am, nil}]}]}, else: ast - ast -> ast - end) - - assignments = - Enum.flat_map(acc.nondirectives, fn - {:@, m, [{attr, am, [val]}]} -> if attr in lifts, do: [{:=, m, [{attr, am, nil}, val]}], else: [] - _ -> [] - end) - - {past, _} = parent - - parent = - parent - |> Zipper.up() - |> Style.find_nearest_block() - |> Zipper.prepend_siblings(assignments) - |> Zipper.find(&(&1 == past)) - - {%{acc | nondirectives: nondirectives}, parent} - else - {acc, parent} - end + |> apply_aliases() + # always force a blank line between @moduledoc and the next directive + |> Map.update!(:moduledoc, &Style.reset_newlines/1) nondirectives = acc.nondirectives @@ -262,42 +262,45 @@ defmodule Styler.Style.ModuleDirectives do end end - defp lift_aliases(%{alias: aliases, require: requires, nondirectives: nondirectives} = acc) do - # we can't use the dealias map built into state as that's what things look like before sorting - # now that we've sorted, it could be different! - dealiases = AliasEnv.define(aliases) - liftable = find_liftable_aliases(requires ++ nondirectives, dealiases) + # alias_env have to be recomputed after we've sorted our `alias` nodes + defp redefine_alias_env(%{alias: aliases} = acc), do: %{acc | alias_env: AliasEnv.define(aliases)} + + defp lift_aliases(%{alias: aliases, require: requires, nondirectives: nondirectives, alias_env: alias_env} = acc) do + liftable = find_liftable_aliases(requires ++ nondirectives, alias_env) if Enum.any?(liftable) do # This is a silly hack that helps comments stay put. # The `cap_line` algo was designed to handle high-line stuff moving up into low line territory, so we set our # new node to have an arbitrarily high line annnnd comments behave! i think. m = [line: 999_999] + aliases_m = [last: m] ++ m aliases = liftable - |> Enum.map(&AliasEnv.expand(dealiases, {:alias, m, [{:__aliases__, [{:last, m} | m], &1}]})) + |> Enum.map(&{:alias, m, [{:__aliases__, aliases_m, AliasEnv.expand(alias_env, &1)}]}) |> Enum.concat(aliases) |> sort() - # lifting could've given us a new order - requires = requires |> do_lift_aliases(liftable) |> sort() - nondirectives = do_lift_aliases(nondirectives, liftable) - %{acc | alias: aliases, require: requires, nondirectives: nondirectives} + # aliases to be lifted have to be applied immediately; yes, these means we're doing multiple apply_alias traversals, + # but we're only doing the duplicate traversals when we're updating the file, so the extra cost of walking + # is negligible vs doing disk I/O + %{acc | alias: aliases} + |> apply_aliases(Map.new(liftable, fn modules -> {modules, List.last(modules)} end)) + |> redefine_alias_env() else acc end end - defp find_liftable_aliases(ast, dealiases) do - excluded = dealiases |> Map.keys() |> Enum.into(Styler.Config.get(:lifting_excludes)) + defp find_liftable_aliases(ast, alias_env) do + excluded = alias_env |> Map.keys() |> Enum.into(Styler.Config.get(:lifting_excludes)) - firsts = MapSet.new(dealiases, fn {_last, [first | _]} -> first end) + firsts = MapSet.new(alias_env, fn {_last, [first | _]} -> first end) ast |> Zipper.zip() # we're reducing a datastructure that looks like - # %{last => {aliases, seen_before?} | :some_collision_probelm} + # %{last => {aliases, seen_before?} | :some_collision_problem} |> Zipper.reduce_while(%{}, fn # we don't want to rewrite alias name `defx Aliases ... do` of these three keywords {{defx, _, args}, _} = zipper, lifts when defx in ~w(defmodule defimpl defprotocol)a -> @@ -312,7 +315,7 @@ defmodule Styler.Style.ModuleDirectives do lifts end - # move the focus to the body block, zkipping over the alias (and the `for` keyword for `defimpl`) + # move the focus to the body block, skipping over the alias (and the `for` keyword for `defimpl`) {:skip, zipper |> Zipper.down() |> Zipper.rightmost() |> Zipper.down() |> Zipper.down(), lifts} {{:quote, _, _}, _} = zipper, lifts -> @@ -324,8 +327,9 @@ defmodule Styler.Style.ModuleDirectives do lifts = cond do # this alias already exists, they just wrote it out fully and are leaving it up to us to shorten it down! - dealiases[last] == aliases -> - Map.put(lifts, last, {aliases, true}) + # we'll get it when we do the apply-aliases scan + alias_env[last] == aliases -> + lifts last in excluded or Enum.any?(aliases, &(not is_atom(&1))) -> lifts @@ -417,27 +421,71 @@ defmodule Styler.Style.ModuleDirectives do |> MapSet.new(fn {_, {aliases, true}} -> aliases end) end - defp do_lift_aliases(ast, to_alias) do + defp apply_aliases(acc) do + apply_aliases(acc, AliasEnv.invert(acc.alias_env)) + end + + defp apply_aliases(acc, inverted_env) when map_size(inverted_env) == 0, do: acc + + defp apply_aliases(%{require: requires, nondirectives: nondirectives, alias_env: alias_env} = acc, inverted_env) do + # applying aliases to requires can change their ordering again + requires = requires |> apply_aliases(inverted_env, alias_env) |> sort() + nondirectives = apply_aliases(nondirectives, inverted_env, alias_env) + %{acc | require: requires, nondirectives: nondirectives} + end + + # applies the aliases withi `to_as` across the given ast + # alias_env is used to expand partial aliases + defp apply_aliases(ast, to_as, alias_env) do ast |> Zipper.zip() - |> Zipper.traverse(fn + |> Zipper.traverse_while(fn {{defx, _, [{:__aliases__, _, _} | _]}, _} = zipper when defx in ~w(defmodule defimpl defprotocol)a -> - # move the focus to the body block, zkipping over the alias (and the `for` keyword for `defimpl`) - zipper |> Zipper.down() |> Zipper.rightmost() |> Zipper.down() |> Zipper.down() |> Zipper.right() - - {{:alias, _, [{:__aliases__, _, [_, _, _ | _] = aliases}]}, _} = zipper -> - # the alias was aliased deeper down. we've lifted that alias to a root, so delete this alias - if aliases in to_alias, - do: Zipper.remove(zipper), - else: zipper + # move the focus to the body block, skipping over the alias (and the `for` keyword for `defimpl`) + zipper = zipper |> Zipper.down() |> Zipper.rightmost() |> Zipper.down() |> Zipper.down() |> Zipper.right() + {:cont, zipper} + + {{:quote, _, _}, _} = zipper -> + {:skip, zipper} + + # apply_aliases is only called on nondirectives (+requires), so this alias must exist within a child node like a def + # if it's within `to_as` then it's an alias that's already defined further up and is thus a duplicate + # either because we lifted and so created a duplicate, or it was just plain ol' user user error. + # either way, we can nix it + {{:alias, _, [{:__aliases__, _, [_ | _] = modules}]}, _} = zipper -> + zipper = if to_as[modules], do: Zipper.remove(zipper), else: zipper + {:cont, zipper} + + # `alias Foo.Bar.Baz, as: Whatever` - never rewrite the LHS through application, + # otherwise we'd produce nonsense like `alias Whatever, as: Whatever`. + # only dedup when the inner alias is an exact duplicate of one defined further up. + {{:alias, _, [{:__aliases__, _, [_ | _] = modules}, [{_, {:__aliases__, _, [as]}}]]}, _} = zipper -> + zipper = if to_as[modules] == as, do: Zipper.remove(zipper), else: zipper + {:skip, zipper} + + # We check even modules of 1 length to catch silly situations like + # alias A.B.C + # alias A.B.C, as: X + # That'll then rename all C to X and C will become unused + {{:__aliases__, meta, [_ | _] = modules}, _} = zipper -> + zipper = + cond do + # There's an alias for this module - replace it with its `as` + as = to_as[modules] -> Zipper.replace(zipper, {:__aliases__, meta, [as]}) + # There's an alias for this modules expansion - replace it with its `as` + # + # This addresses the following: + # given modules=`C.D`, to_as=`A.B.C => C, A.B.C.D => X`, + # should yield -> `X` because `C.D -> A.B.C.D -> X` + as = to_as[AliasEnv.expand(alias_env, modules)] -> Zipper.replace(zipper, {:__aliases__, meta, [as]}) + true -> zipper + end - {{:__aliases__, meta, [_, _, _ | _] = aliases}, _} = zipper -> - if aliases in to_alias, - do: Zipper.replace(zipper, {:__aliases__, meta, [List.last(aliases)]}), - else: zipper + # minor optimization - don't need to walk the module list! + {:skip, zipper} zipper -> - zipper + {:cont, zipper} end) |> Zipper.node() end @@ -474,7 +522,6 @@ defmodule Styler.Style.ModuleDirectives do |> Style.reset_newlines() end - # TODO investigate removing this in favor of the Style.post_sort_cleanup(node, comments) # "Fixes" the line numbers of nodes who have had their orders changed via sorting or other methods. # This "fix" simply ensures that comments don't get wrecked as part of us moving AST nodes willy-nilly. # diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index 0d9b5e48..88c51c0e 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -241,19 +241,13 @@ defmodule Styler.Style.Pipes do else # looks like it's just a normal function, so lift the first arg up into a new pipe # `foo(a, ...) |> ...` => `a |> foo(...) |> ...` + # + # If the first arg is a syntax-sugared kwl, we need to manually desugar it arg = - case arg do - # If the first arg is a syntax-sugared kwl, we need to manually desugar it to cover all scenarios - [{{:__block__, bm, _}, {:__block__, _, _}} | _] -> - if bm[:format] == :keyword do - {:__block__, [line: line, closing: [line: line]], [arg]} - else - arg - end - - arg -> - arg - end + with [{{:__block__, bm, _}, _} | _] <- arg, + :keyword <- bm[:format], + do: {:__block__, [line: line, closing: [line: line]], [arg]}, + else: (_ -> arg) {{:|>, [line: line], [arg, {fun, meta, args}]}, nil} end @@ -300,6 +294,34 @@ defmodule Styler.Style.Pipes do defp fix_pipe({:|>, m, [lhs, {{:., m2, [{anon_fun, _, _}] = fun}, _, []}]}) when anon_fun in [:&, :fn], do: {:|>, m, [lhs, {:then, m2, fun}]} + # `lhs |> Enum.sort([:asc, :desc]) |> Enum.reverse()` => `lhs |> Enum.sort(:desc | :asc)` + defp fix_pipe( + pipe_chain( + pm, + lhs, + {{:., _, [{_, _, [:Enum]}, :sort]} = sort, meta, sort_args}, + {{:., _, [{_, _, [:Enum]}, :reverse]}, _, []} + ) = node + ) do + case sort_args do + [] -> {:|>, pm, [lhs, {sort, meta, [{:__block__, [line: meta[:line]], [:desc]}]}]} + [{_, m, [:desc]}] -> {:|>, pm, [lhs, {sort, meta, [{:__block__, m, [:asc]}]}]} + [{_, m, [:asc]}] -> {:|>, pm, [lhs, {sort, meta, [{:__block__, m, [:desc]}]}]} + _ -> node + end + end + + # `lhs |> Enum.map(fun) |> Enum.intersperse(sep)` => `lhs |> Enum.map_intersperse(sep, fun) + defp fix_pipe( + pipe_chain( + pm, + lhs, + {{:., dm, [{_, _, [:Enum]} = enum, :map]}, em, [fun]}, + {{:., _, [{_, _, [:Enum]}, :intersperse]}, _, [sep]} + ) + ), + do: {:|>, pm, [lhs, {{:., dm, [enum, :map_intersperse]}, em, [Style.set_line(sep, em[:line]), fun]}]} + # `lhs |> Enum.reverse() |> Enum.concat(enum)` => `lhs |> Enum.reverse(enum)` defp fix_pipe( pipe_chain( @@ -308,9 +330,19 @@ defmodule Styler.Style.Pipes do {{:., _, [{_, _, [:Enum]}, :reverse]} = reverse, meta, []}, {{:., _, [{_, _, [:Enum]}, :concat]}, _, [enum]} ) - ) do - {:|>, pm, [lhs, {reverse, [line: meta[:line]], [enum]}]} - end + ), + do: {:|>, pm, [lhs, {reverse, meta, [enum]}]} + + # `lhs |> Enum.filter(fun) |> List.first([default])` => `lhs |> Enum.find([default], fun)` + defp fix_pipe( + pipe_chain( + pm, + lhs, + {{:., dm, [{_, _, [:Enum]} = enum, :filter]}, meta, [fun]}, + {{:., _, [{_, _, [:List]}, :first]}, _, default} + ) + ), + do: {:|>, pm, [lhs, {{:., dm, [enum, :find]}, meta, Style.set_line(default, meta[:line]) ++ [fun]}]} # `lhs |> Enum.reverse() |> Kernel.++(enum)` => `lhs |> Enum.reverse(enum)` defp fix_pipe( @@ -320,9 +352,8 @@ defmodule Styler.Style.Pipes do {{:., _, [{_, _, [:Enum]}, :reverse]} = reverse, meta, []}, {{:., _, [{_, _, [:Kernel]}, :++]}, _, [enum]} ) - ) do - {:|>, pm, [lhs, {reverse, [line: meta[:line]], [enum]}]} - end + ), + do: {:|>, pm, [lhs, {reverse, meta, [enum]}]} # `lhs |> Enum.filter(filterer) |> Enum.count()` => `lhs |> Enum.count(count)` defp fix_pipe( @@ -333,9 +364,8 @@ defmodule Styler.Style.Pipes do {{:., _, [{_, _, [:Enum]}, :count]} = count, _, []} ) ) - when mod in @enum do - {:|>, pm, [lhs, {count, [line: meta[:line]], [filterer]}]} - end + when mod in @enum, + do: {:|>, pm, [lhs, {count, meta, [filterer]}]} # `lhs |> Stream.map(fun) |> Stream.run()` => `lhs |> Enum.each(fun)` # `lhs |> Stream.each(fun) |> Stream.run()` => `lhs |> Enum.each(fun)` @@ -347,9 +377,8 @@ defmodule Styler.Style.Pipes do {{:., _, [{_, _, [:Stream]}, :run]}, _, []} ) ) - when map_or_each in [:map, :each] do - {:|>, pm, [lhs, {{:., dm, [{a, am, [:Enum]}, :each]}, fm, fa}]} - end + when map_or_each in [:map, :each], + do: {:|>, pm, [lhs, {{:., dm, [{a, am, [:Enum]}, :each]}, fm, fa}]} # `lhs |> Enum.map(mapper) |> Enum.join(joiner)` => `lhs |> Enum.map_join(joiner, mapper)` defp fix_pipe( @@ -360,10 +389,8 @@ defmodule Styler.Style.Pipes do {{:., _, [{_, _, [:Enum]} = enum, :join]}, _, join_args} ) ) - when mod in @enum do - rhs = {{:., dm, [enum, :map_join]}, em, Style.set_line(join_args, dm[:line]) ++ map_args} - {:|>, pm, [lhs, rhs]} - end + when mod in @enum, + do: {:|>, pm, [lhs, {{:., dm, [enum, :map_join]}, em, Style.set_line(join_args, dm[:line]) ++ map_args}]} # `lhs |> Enum.map(mapper) |> Enum.into(empty_map)` => `lhs |> Map.new(mapper)` # or @@ -402,9 +429,30 @@ defmodule Styler.Style.Pipes do {{:., _, [{_, _, [mod]}, :new]} = new, _, []} ) ) - when mod in @collectable and enum in @enum do - {:|>, pm, [lhs, {Style.set_line(new, em[:line]), em, [mapper]}]} - end + when mod in @collectable and enum in @enum, + do: {:|>, pm, [lhs, {Style.set_line(new, em[:line]), em, [mapper]}]} + + @req2 for fun <- ~w(delete get head patch post put request run), bang <- ["", "!"], do: :"#{fun}#{bang}" + + # rewrite `Keyword.merge(opt) |> Req.fun1()` to `Req.fun2(opt)` for 2 arity functions that take `opts` as a second arg + defp fix_pipe( + pipe_chain( + pm, + lhs, + {{:., _, [{_, _, [req_or_kw]}, :merge]}, m, [kw]}, + {{:., _, [{_, _, [:Req]}, fun]} = req, _, []} + ) + ) + when req_or_kw in [:Req, :Keyword] and fun in @req2, + do: fix_pipe({:|>, pm, [lhs, {req, m, [kw]}]}) + + # Req.new |> Req.fun1,2 -> Req.fun1,2 + # all `fun` options take the same args as `Req.new`, so it's redundant to call Req.new before them + defp fix_pipe( + pipe_chain(pm, lhs, {{:., _, [{_, _, [:Req]}, :new]}, m, []}, {{:., _, [{_, _, [:Req]}, fun]} = req, _, args}) + ) + when fun in @req2, + do: {:|>, pm, [lhs, {req, m, args}]} defp fix_pipe(node), do: node diff --git a/lib/style/single_node.ex b/lib/style/single_node.ex index 4176909f..43ce25c5 100644 --- a/lib/style/single_node.ex +++ b/lib/style/single_node.ex @@ -17,7 +17,6 @@ defmodule Styler.Style.SingleNode do * Credo.Check.Consistency.ParameterPatternMatching * Credo.Check.Readability.LargeNumbers * Credo.Check.Readability.ParenthesesOnZeroArityDefs - * Credo.Check.Readability.PreferImplicitTry * Credo.Check.Readability.StringSigils * Credo.Check.Readability.WithSingleClause * Credo.Check.Refactor.CaseTrivialMatches @@ -37,6 +36,50 @@ defmodule Styler.Style.SingleNode do def run({node, meta}, ctx), do: {:cont, {style(node), meta}, ctx} + defp style({:assert, meta, [{:!=, _, [x, {:__block__, _, [nil]}]}]}), do: style({:assert, meta, [x]}) + # refute nilly -> assert + defp style({:refute, meta, [{:is_nil, _, [x]}]}), do: style({:assert, meta, [x]}) + defp style({:refute, meta, [{:==, _, [x, {:__block__, _, [nil]}]}]}), do: style({:assert, meta, [x]}) + # boolean ops and assert hurt my brain. + # the lone exception is `==` (... for now) ((uh, and the exception to the exception is when it's `== nil`, above)) + defp style({:refute, meta, [{:!=, m, xy}]}), do: style({:assert, meta, [{:==, m, xy}]}) + defp style({:refute, meta, [{:!==, m, xy}]}), do: style({:assert, meta, [{:===, m, xy}]}) + defp style({:refute, meta, [{:<, m, xy}]}), do: style({:assert, meta, [{:>=, m, xy}]}) + defp style({:refute, meta, [{:<=, m, xy}]}), do: style({:assert, meta, [{:>, m, xy}]}) + defp style({:refute, meta, [{:>, m, xy}]}), do: style({:assert, meta, [{:<=, m, xy}]}) + defp style({:refute, meta, [{:>=, m, xy}]}), do: style({:assert, meta, [{:<, m, xy}]}) + + # `assert x not in y` reads more naturally than `refute x in y`, so leave it alone (and same for refute). + defp style({a, _, [{:not, _, [{:in, _, _}]}]} = node) when a in [:assert, :refute], do: node + + for {a, inverted} <- [{:assert, :refute}, {:refute, :assert}] do + # invert negations + defp style({unquote(a), meta, [{n, _, [x]}]}) when n in [:!, :not], do: style({unquote(inverted), meta, [x]}) + + # assert Enum.member? -> assert in + defp style({unquote(a), meta, [{{:., _, [{:__aliases__, _, [:Enum]}, :member?]}, _, [enum, elem]}]}), + do: {unquote(a), meta, [{:in, [line: meta[:line]], [elem, enum]}]} + + # assert Enum.find -> assert Enum.any? + defp style({unquote(a), meta, [{{:., a, [{:__aliases__, b, [:Enum]}, :find]}, c, [enum, fun]}]}), + do: style({unquote(a), meta, [{{:., a, [{:__aliases__, b, [:Enum]}, :any?]}, c, [enum, fun]}]}) + + # Enum.any?(x, & &1 == y) => y in x + defp style({unquote(a) = a, m, [{{:., _, [{:__aliases__, _, [:Enum]}, :any?]}, _, [y, fun]}]} = node) do + case fun do + # & &1 == x + {:&, _, [{:==, _, [{:&, _, [1]}, x]}]} -> {a, m, [{:in, [line: m[:line]], [x, y]}]} + # & x == &1 + {:&, _, [{:==, _, [x, {:&, _, [1]}]}]} -> {a, m, [{:in, [line: m[:line]], [x, y]}]} + # fn var -> var == x + {:fn, _, [{:->, _, [[{var, _, nil}], {:==, _, [{var, _, nil}, x]}]}]} -> {a, m, [{:in, [line: m[:line]], [x, y]}]} + # fn var -> x == var + {:fn, _, [{:->, _, [[{var, _, nil}], {:==, _, [x, {var, _, nil}]}]}]} -> {a, m, [{:in, [line: m[:line]], [x, y]}]} + _ -> node + end + end + end + # rewrite double-quote strings with >= 4 escaped double-quotes as sigils defp style({:__block__, [{:delimiter, ~s|"|} | meta], [string]} = node) when is_binary(string) do # running a regex against every double-quote delimited string literal in a codebase doesn't have too much impact @@ -51,7 +94,7 @@ defmodule Styler.Style.SingleNode do |> Stream.concat(@closing_delimiters) |> Enum.frequencies() |> Enum.min_by(fn - {~s|"|, count} -> {count, 1} + {"\"", count} -> {count, 1} {")", count} -> {count, 2} {"}", count} -> {count, 3} {"|", count} -> {count, 4} @@ -183,50 +226,54 @@ defmodule Styler.Style.SingleNode do defp style({:case, cm, [head, [{do_, arrows}]]}), do: {:case, cm, [head, [{do_, rewrite_arrows(arrows)}]]} defp style({:fn, m, arrows}), do: {:fn, m, rewrite_arrows(arrows)} - defp style({:to_timeout, meta, [[{{:__block__, um, [unit]}, {:*, _, [left, right]}}]]} = node) - when unit in ~w(day hour minute second millisecond)a do - [l, r] = - Enum.map([left, right], fn - {_, _, [x]} -> x - _ -> nil - end) + defp style({:to_timeout, m, [[_ | _] = args]}), do: {:to_timeout, m, [Enum.map(args, &style_to_timeout_arg/1)]} - {step, next_unit} = + defp style(node), do: node + + # 1. convert plurals to singulars (`minutes` -> `minute`) + # 2. upgrade values, eg `minute: 5 * 60` -> `hour: 5` and `minute: 60` -> `hour: 1` + defp style_to_timeout_arg({{:__block__, m, [unit]}, value}) do + {unit, step, next_unit} = case unit do - :day -> {7, :week} - :hour -> {24, :day} - :minute -> {60, :hour} - :second -> {60, :minute} - :millisecond -> {1000, :second} + :day -> {:day, 7, :week} + :days -> {:day, 7, :week} + :hour -> {:hour, 24, :day} + :hours -> {:hour, 24, :day} + :millisecond -> {:millisecond, 1000, :second} + :milliseconds -> {:millisecond, 1000, :second} + :minute -> {:minute, 60, :hour} + :minutes -> {:minute, 60, :hour} + :second -> {:second, 60, :minute} + :seconds -> {:second, 60, :minute} + :week -> {:week, :"$no_next_step", nil} + :weeks -> {:week, :"$no_next_step", nil} + unit -> {unit, :"$no_next_step", nil} end - if step in [l, r] do - n = if l == step, do: right, else: left - style({:to_timeout, meta, [[{{:__block__, um, [next_unit]}, n}]]}) - else - node - end - end + {unit, value} = + case value do + # minute: 60 -> hours: 1 + {:__block__, tm, [^step]} -> + {next_unit, {:__block__, [token: "1", line: tm[:line]], [1]}} + + # minute: 60 * rhs -> hours: rhs + {:*, _, [{_, _, [^step]}, rhs]} -> + {{_, _, [next_unit]}, value} = style_to_timeout_arg({{:__block__, m, [next_unit]}, rhs}) + {next_unit, value} + + # minute: lhs * 60 -> hours: lhs + {:*, _, [lhs, {_, _, [^step]}]} -> + {{_, _, [next_unit]}, value} = style_to_timeout_arg({{:__block__, m, [next_unit]}, lhs}) + {next_unit, value} - defp style({:to_timeout, meta, [[{{:__block__, um, [unit]}, {:__block__, tm, [n]}}]]} = node) do - step_up = - case {unit, n} do - {:day, 7} -> :week - {:hour, 24} -> :day - {:minute, 60} -> :hour - {:second, 60} -> :minute - {:millisecond, 1000} -> :second - _ -> nil + value -> + {unit, value} end - if step_up do - {:to_timeout, meta, [[{{:__block__, um, [step_up]}, {:__block__, [token: "1", line: tm[:line]], [1]}}]]} - else - node - end + {{:__block__, m, [unit]}, value} end - defp style(node), do: node + defp style_to_timeout_arg(other), do: other defp replace_into({:., dm, [{_, am, _} = enum, _]}, collectable, rest) do case collectable do diff --git a/lib/styler.ex b/lib/styler.ex index 9ca3ded9..be642072 100644 --- a/lib/styler.ex +++ b/lib/styler.ex @@ -33,7 +33,7 @@ defmodule Styler do @doc false def style({ast, comments}, file, opts) do on_error = opts[:on_error] || :log - Styler.Config.set(opts) + Styler.Config.initialize(opts) zipper = Zipper.zip(ast) {{ast, _}, comments} = @@ -76,8 +76,7 @@ defmodule Styler do ast_to_string(ast, comments, formatter_opts) end - @doc false - # Wrap `Code.string_to_quoted_with_comments` with our desired options + @doc "Just `Code.string_to_quoted_with_comments/2` with the necessary options" def string_to_ast(code, file \\ "nofile") when is_binary(code) do Code.string_to_quoted_with_comments!(code, literal_encoder: &__MODULE__.literal_encoder/2, diff --git a/lib/styler/config.ex b/lib/styler/config.ex index a5e31600..9c216b00 100644 --- a/lib/styler/config.ex +++ b/lib/styler/config.ex @@ -20,14 +20,15 @@ defmodule Styler.Config do Range Record Regex Registry Set Stream String StringIO Supervisor System Task Time Tuple URI Version )a) - def set(config) do + def initialize(config) do :persistent_term.get(@key) :ok rescue - ArgumentError -> set!(config) + ArgumentError -> set(config) end - def set!(config) do + # Public for tests + def set(config) do excludes = config[:alias_lifting_exclude] |> List.wrap() @@ -43,8 +44,16 @@ defmodule Styler.Config do end) |> MapSet.union(@stdlib) + elixir_version = + case config[:minimum_supported_elixir_version] do + vsn when is_binary(vsn) -> Version.parse!(vsn) + nil -> nil + other -> raise ArgumentError, "`:minimum_supported_elixir_version` must be a string, got: #{inspect(other)}" + end + :persistent_term.put(@key, %{ - lifting_excludes: excludes + lifting_excludes: excludes, + minimum_supported_elixir_version: elixir_version }) end @@ -53,4 +62,12 @@ defmodule Styler.Config do |> :persistent_term.get() |> Map.fetch!(key) end + + def version_compatible?(%Version{} = version) do + if minimum_supported_elixir_version = get(:minimum_supported_elixir_version) do + Version.compare(version, minimum_supported_elixir_version) != :gt + else + true + end + end end diff --git a/lib/zipper.ex b/lib/zipper.ex index 11288ad4..5ac5132b 100644 --- a/lib/zipper.ex +++ b/lib/zipper.ex @@ -27,8 +27,14 @@ defmodule Styler.Zipper do import Kernel, except: [node: 1] @type tree :: Macro.t() + + @opaque path :: %{ + l: [tree], + ptree: zipper, + r: [tree] + } + @type zipper :: {tree, path | nil} - @type path :: {left :: [tree], parent :: zipper, right :: [tree]} @type t :: zipper @type command :: :cont | :skip | :halt @@ -86,7 +92,7 @@ defmodule Styler.Zipper do def down(zipper) do case children(zipper) do [] -> nil - [first | rest] -> {first, {[], zipper, rest}} + [first | rest] -> {first, %{ptree: zipper, l: [], r: rest}} end end @@ -97,8 +103,9 @@ defmodule Styler.Zipper do @spec up(zipper) :: zipper | nil def up({_, nil}), do: nil - def up({tree, {l, {parent, parent_meta}, r}}) do - children = Enum.reverse(l, [tree | r]) + def up({tree, meta}) do + children = Enum.reverse(meta.l, [tree | meta.r]) + {parent, parent_meta} = meta.ptree {do_replace_children(parent, children), parent_meta} end @@ -106,16 +113,16 @@ defmodule Styler.Zipper do Returns the zipper of the left sibling of the node at this zipper, or nil. """ @spec left(zipper) :: zipper | nil - def left({tree, {[ltree | l], p, r}}), do: {ltree, {l, p, [tree | r]}} + def left({tree, %{l: [ltree | l], r: r} = meta}), do: {ltree, %{meta | l: l, r: [tree | r]}} def left(_), do: nil @doc """ Returns the leftmost sibling of the node at this zipper, or itself. """ @spec leftmost(zipper) :: zipper - def leftmost({tree, {[_ | _] = l, p, r}}) do - [leftmost | r] = Enum.reverse(l, [tree | r]) - {leftmost, {[], p, r}} + def leftmost({tree, %{l: [_ | _] = l} = meta}) do + [leftmost | r] = Enum.reverse(l, [tree | meta.r]) + {leftmost, %{meta | l: [], r: r}} end def leftmost({_, _} = zipper), do: zipper @@ -124,16 +131,16 @@ defmodule Styler.Zipper do Returns the zipper of the right sibling of the node at this zipper, or nil. """ @spec right(zipper) :: zipper | nil - def right({tree, {l, p, [rtree | r]}}), do: {rtree, {[tree | l], p, r}} + def right({tree, %{r: [rtree | r]} = meta}), do: {rtree, %{meta | r: r, l: [tree | meta.l]}} def right(_), do: nil @doc """ Returns the rightmost sibling of the node at this zipper, or itself. """ @spec rightmost(zipper) :: zipper - def rightmost({tree, {l, p, [_ | _] = r}}) do - [rightmost | l] = Enum.reverse(r, [tree | l]) - {rightmost, {l, p, []}} + def rightmost({tree, %{r: [_ | _] = r} = meta}) do + [rightmost | l] = Enum.reverse(r, [tree | meta.l]) + {rightmost, %{meta | l: l, r: []}} end def rightmost({_, _} = zipper), do: zipper @@ -157,8 +164,8 @@ defmodule Styler.Zipper do """ @spec remove(zipper) :: zipper def remove({_, nil}), do: raise(ArgumentError, message: "Cannot remove the top level node.") - def remove({_, {[left | rest], p, r}}), do: prev_down({left, {rest, p, r}}) - def remove({_, {_, {parent, parent_meta}, children}}), do: {do_replace_children(parent, children), parent_meta} + def remove({_, %{l: [left | rest]} = meta}), do: prev_down({left, %{meta | l: rest}}) + def remove({_, %{ptree: {parent, parent_meta}, r: children}}), do: {do_replace_children(parent, children), parent_meta} @doc """ Inserts the item as the left sibling of the node at this zipper, without @@ -178,7 +185,7 @@ defmodule Styler.Zipper do """ @spec prepend_siblings(zipper, [tree]) :: zipper def prepend_siblings({node, nil}, siblings), do: {:__block__, [], siblings ++ [node]} |> zip() |> down() |> rightmost() - def prepend_siblings({tree, {l, p, r}}, siblings), do: {tree, {Enum.reverse(siblings, l), p, r}} + def prepend_siblings({tree, meta}, siblings), do: {tree, %{meta | l: Enum.reverse(siblings, meta.l)}} @doc """ Inserts the item as the right sibling of the node at this zipper, without @@ -198,18 +205,19 @@ defmodule Styler.Zipper do """ @spec insert_siblings(zipper, [tree]) :: zipper def insert_siblings({node, nil}, siblings), do: {:__block__, [], [node | siblings]} |> zip() |> down() - def insert_siblings({tree, {l, p, r}}, siblings), do: {tree, {l, p, siblings ++ r}} + def insert_siblings({tree, meta}, siblings), do: {tree, %{meta | r: siblings ++ meta.r}} - @doc """ - Inserts the item as the leftmost child of the node at this zipper, - without moving. - """ + @doc "Inserts the item as the leftmost child of the node at this zipper." def insert_child({tree, meta}, child), do: {do_insert_child(tree, child), meta} defp do_insert_child({form, meta, args}, child) when is_list(args), do: {form, meta, [child | args]} defp do_insert_child(list, child) when is_list(list), do: [child | list] defp do_insert_child({left, right}, child), do: {:{}, [], [child, left, right]} + @doc "Inserts many children, prepending the new list to the existing children of this node" + def insert_children({tree, meta}, children) when is_list(children), + do: replace_children({tree, meta}, children ++ children({tree, meta})) + @doc """ Inserts the item as the rightmost child of the node at this zipper, without moving. diff --git a/mix.exs b/mix.exs index 305656f2..24c964a3 100644 --- a/mix.exs +++ b/mix.exs @@ -12,7 +12,7 @@ defmodule Styler.MixProject do use Mix.Project # Don't forget to bump the README when doing non-patch version changes - @version "1.4.2" + @version "1.11.0" @url "https://github.com/adobe/elixir-styler" def project do @@ -49,7 +49,7 @@ defmodule Styler.MixProject do [ maintainers: ["Matt Enlow", "Greg Mefford"], licenses: ["Apache-2.0"], - links: %{"GitHub" => @url} + links: %{"GitHub" => @url, "Changelog" => "#{@url}/blob/main/CHANGELOG.md"} ] end @@ -59,18 +59,19 @@ defmodule Styler.MixProject do source_ref: "v#{@version}", source_url: @url, groups_for_extras: [ - Rewrites: ~r/docs/ + Features: ~r/docs/ ], extra_section: "Docs", extras: [ "CHANGELOG.md": [title: "Changelog"], - "docs/styles.md": [title: "Basic Styles"], - "docs/deprecations.md": [title: "Deprecated Elixirisms"], - "docs/pipes.md": [title: "Pipe Chains"], + "docs/styles.md": [title: "Styles/Features Table of Contents"], + "docs/comment_directives.md": [title: "Comment Directives (# styler:sort)"], "docs/control_flow_macros.md": [title: "Control Flow Macros (if, case, ...)"], + "docs/deprecations.md": [title: "Deprecated Elixirisms"], + "docs/general_styles.md": [title: "General Styles"], "docs/mix_configs.md": [title: "Mix Configs (config/*.exs)"], "docs/module_directives.md": [title: "Module Directives (use, alias, ...)"], - "docs/comment_directives.md": [title: "Comment Directives (# styler:sort)"], + "docs/pipes.md": [title: "Pipe Chains"], "docs/credo.md": [title: "Styler & Credo"], "README.md": [title: "Styler"] ] diff --git a/test/alias_env_test.exs b/test/alias_env_test.exs new file mode 100644 index 00000000..40218499 --- /dev/null +++ b/test/alias_env_test.exs @@ -0,0 +1,75 @@ +# Copyright 2025 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + +defmodule Styler.AliasEnvTest do + use ExUnit.Case, async: true + + import Styler.AliasEnv + + test "define" do + {:__block__, [], ast} = + quote do + alias A.B + alias C.D, as: X + end + + env = define(ast) + + assert %{B: [:A, :B], X: [:C, :D]} == env + assert %{B: [:M, :N, :B], X: [:C, :D]} == define(env, quote(do: alias(M.N.B))) + end + + test "expand" do + assert expand(%{B: [:A, :B], X: [:C, :D]}, [:B]) == [:A, :B] + assert expand(%{B: [:A, :B], X: [:C, :D]}, [:B, :C, :D]) == [:A, :B, :C, :D] + assert expand(%{B: [:A, :B], X: [:C, :D]}, [:Not, :Present]) == [:Not, :Present] + assert expand(%{}, [:Hi]) == [:Hi] + end + + test "expand_ast" do + {_, _, aliases} = + quote do + alias A.B + alias A.B.C + alias A.B.C.D, as: X + end + + ast = + quote do + A + B + C + X + end + + expected = + quote do + A + A.B + A.B.C + A.B.C.D + end + + assert aliases |> define() |> expand_ast(ast) == expected + end + + test "invert" do + {_, _, aliases} = + quote do + alias A.B + alias A.B.C + alias A.B.C, as: X + alias A.B.C, as: Y + end + + env = define(aliases) + assert invert(env) == %{[:A, :B, :C] => :Y, [:A, :B] => :B} + end +end diff --git a/test/config_integration_test.exs b/test/config_integration_test.exs new file mode 100644 index 00000000..17d0931a --- /dev/null +++ b/test/config_integration_test.exs @@ -0,0 +1,28 @@ +defmodule Styler.ConfigIntegrationTest do + use Styler.StyleCase, async: false + + alias Styler.Config + + setup do + on_exit(fn -> Config.set([]) end) + end + + test "`:alias_lifting_exclude` - collisions with configured modules" do + Config.set(alias_lifting_exclude: ~w(C)a) + + assert_style """ + alias Foo.Bar + + A.B.C + A.B.C + """ + end + + @tag skip: Version.match?(System.version(), "< 1.17.0-dev") + test "`:minimum_supported_elixir_version` and :timer config @ 1.17-dev" do + Config.set(minimum_supported_elixir_version: "1.16.0") + assert_style ":timer.minutes(60 * 4)" + Config.set(minimum_supported_elixir_version: "1.17.0-dev") + assert_style ":timer.hours(x)", "to_timeout(hour: x)" + end +end diff --git a/test/config_test.exs b/test/config_test.exs index 314e72b2..522d972c 100644 --- a/test/config_test.exs +++ b/test/config_test.exs @@ -1,26 +1,40 @@ +# Copyright 2024 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + defmodule Styler.ConfigTest do use ExUnit.Case, async: false import Styler.Config - test "no config is good times" do - assert :ok = set!([]) + setup do + on_exit(fn -> set([]) end) + end + + test "initialize" do + assert :ok = initialize([]) end describe "alias_lifting_exclude" do test "takes singletons atom" do - set!(alias_lifting_exclude: Foo) + set(alias_lifting_exclude: Foo) assert %MapSet{} = excludes = get(:lifting_excludes) assert :Foo in excludes refute Foo in excludes - set!(alias_lifting_exclude: :Foo) + set(alias_lifting_exclude: :Foo) assert %MapSet{} = excludes = get(:lifting_excludes) assert :Foo in excludes end test "list of atoms" do - set!(alias_lifting_exclude: [Foo, :Bar]) + set(alias_lifting_exclude: [Foo, :Bar]) assert %MapSet{} = excludes = get(:lifting_excludes) assert :Foo in excludes refute Foo in excludes @@ -29,8 +43,39 @@ defmodule Styler.ConfigTest do test "raises on non-atom inputs" do assert_raise RuntimeError, ~r"Expected an atom", fn -> - set!(alias_lifting_exclude: ["Bar"]) + set(alias_lifting_exclude: ["Bar"]) end end end + + describe "minimum_supported_elixir_version" do + test "can be nil/unset" do + set(minimum_supported_elixir_version: nil) + assert is_nil(get(:minimum_supported_elixir_version)) + set([]) + assert is_nil(get(:minimum_supported_elixir_version)) + end + + test "parses semvers" do + set(minimum_supported_elixir_version: "1.15.0") + assert get(:minimum_supported_elixir_version) == Version.parse!("1.15.0") + end + + test "kabooms for UX" do + for weird <- ["1.15", "wee"] do + assert_raise Version.InvalidVersionError, fn -> set(minimum_supported_elixir_version: weird) end + end + + assert_raise ArgumentError, ~r/must be a string/, fn -> set(minimum_supported_elixir_version: 1.15) end + end + end + + test "version_compatible?" do + set(minimum_supported_elixir_version: nil) + assert version_compatible?(Version.parse!("100.0.0")) + set(minimum_supported_elixir_version: "1.15.0") + assert version_compatible?(Version.parse!("1.14.0")) + assert version_compatible?(Version.parse!("1.15.0")) + refute version_compatible?(Version.parse!("1.16.0")) + end end diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 8f6d0780..6c09555c 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -11,7 +11,149 @@ defmodule Styler.Style.BlocksTest do use Styler.StyleCase, async: true - describe "with statements" do + describe "case statements with a single clause" do + test "noop for when" do + assert_style """ + case foo do + bar when is_binary(bar) -> baz(bar) + end + """ + end + + test "changes to assignment" do + assert_style( + """ + case foo do + bar -> baz(bar) + end + """, + """ + bar = foo + baz(bar) + """ + ) + + assert_style( + """ + case foo |> Bar.baz() |> Bop.boop() do + {:ok, widget} -> + x = y + wodget(widget) + end + """, + """ + {:ok, widget} = foo |> Bar.baz() |> Bop.boop() + x = y + wodget(widget) + """ + ) + end + + test "with comments" do + assert_style( + """ + # bar + bar + + # head + case foo |> Bar.baz() |> Bop.boop() do + # clause + {:ok, widget} -> + # body + x = y + wodget(widget) + end + """, + """ + # bar + bar + + # head + # clause + {:ok, widget} = foo |> Bar.baz() |> Bop.boop() + # body + x = y + wodget(widget) + """ + ) + end + + test "singleton parent" do + assert_style( + """ + if foo do + case complex |> head() |> stuff() do + {:ok, whatever} -> + some_body(whatever) + end + end + """, + """ + if foo do + {:ok, whatever} = complex |> head() |> stuff() + some_body(whatever) + end + """ + ) + end + + test "when already an assignment" do + assert_style( + """ + preroll + + assignment = + case head do + lhs -> body + end + """, + """ + preroll + + lhs = head + assignment = body + """ + ) + + assert_style( + """ + # assignmet + assignment = + # case head + case head do + # lhs + lhs -> + # body + body + # clauses + fn -> + look + im(a) + # big function + big function + end + end + """, + """ + # assignmet + # case head + # lhs + lhs = head + # body + body + # clauses + assignment = fn -> + look + im(a) + # big function + big(function) + end + """ + ) + end + end + + describe "with" do test "replacement due to no (or all removed) arrows" do assert_style( """ @@ -577,41 +719,98 @@ defmodule Styler.Style.BlocksTest do end end - test "Credo.Check.Refactor.CondStatements" do - for truthy <- ~w(true :atom :else) do - assert_style( - """ + describe "cond" do + test "rewrite some two-clause conds as an if statement" do + for truthy <- ~w(true :atom :else) do + assert_style( + """ + cond do + a -> b + #{truthy} -> c + end + """, + """ + if a do + b + else + c + end + """ + ) + end + + for falsey <- ~w(false nil) do + assert_style(""" cond do a -> b - #{truthy} -> c + #{falsey} -> c end - """, - """ - if a do - b - else - c + """) + end + + for ignored <- ["x == y", "foo", "foo()", "foo(b)", "Module.foo(x)"] do + assert_style(""" + cond do + a -> b + #{ignored} -> c end - """ - ) + """) + end end - for falsey <- ~w(false nil) do - assert_style(""" + test "if final clause is an atom or truthy value, change the clause to `true`" do + assert_style """ cond do a -> b - #{falsey} -> c + c -> d + true -> woo end - """) - end + """ - for ignored <- ["x == y", "foo", "foo()", "foo(b)", "Module.foo(x)", ~s("else"), "%{}", "{}"] do - assert_style(""" + assert_style """ cond do a -> b - #{ignored} -> c + c -> d + call() -> woo + end + """ + + for truthy <- [~s("else"), ":else", "%{}", "{}"] do + assert_style( + """ + cond do + a -> b + c -> d + #{truthy} -> woo + end + """, + """ + cond do + a -> b + c -> d + true -> woo + end + """ + ) + end + + for truthy <- [~s("else"), ":else", "%{}", "{}"] do + assert_style( + """ + cond do + a -> b + #{truthy} -> woo + end + """, + """ + if a do + b + else + woo + end + """ + ) end - """) end end diff --git a/test/style/comment_directives_test.exs b/test/style/comment_directives_test.exs index c849e829..35fec3f9 100644 --- a/test/style/comment_directives_test.exs +++ b/test/style/comment_directives_test.exs @@ -220,6 +220,39 @@ defmodule Styler.Style.CommentDirectivesTest do ) end + test "typespecs" do + assert_style( + """ + # styler:sort + @type t :: %__MODULE__{ + id: Ecto.UUID.t(), + street_address: String.t(), + postal_code: String.t(), + locality: String.t(), + region: String.t(), + country_iso: String.t(), + deleted_at: DateTime.t() | nil, + inserted_at: DateTime.t(), + updated_at: DateTime.t() + } + """, + """ + # styler:sort + @type t :: %__MODULE__{ + country_iso: String.t(), + deleted_at: DateTime.t() | nil, + id: Ecto.UUID.t(), + inserted_at: DateTime.t(), + locality: String.t(), + postal_code: String.t(), + region: String.t(), + street_address: String.t(), + updated_at: DateTime.t() + } + """ + ) + end + test "assignments" do assert_style( """ diff --git a/test/style/configs_test.exs b/test/style/configs_test.exs index 71e7cf81..215074c5 100644 --- a/test/style/configs_test.exs +++ b/test/style/configs_test.exs @@ -390,5 +390,45 @@ defmodule Styler.Style.ConfigsTest do """ ) end + + test "phx config" do + assert_style( + """ + import Config + + config :demo, DemoWeb.Endpoint, + http: [ip: {127, 0, 0, 1}, port: 4000] + + # In order to use HTTPS in development, a self-signed + # + # mix phx.gen.cert + # + # If desired, both `http:` and `https:` keys can be + + # Set a higher stacktrace during development. Avoid configuring such + config :phoenix, :stacktrace_depth, 20 + + # Initialize plugs at runtime for faster development compilation + config :phoenix, :plug_init_mode, :runtime + """, + """ + import Config + + config :demo, DemoWeb.Endpoint, http: [ip: {127, 0, 0, 1}, port: 4000] + + # Initialize plugs at runtime for faster development compilation + config :phoenix, :plug_init_mode, :runtime + + # In order to use HTTPS in development, a self-signed + # + # mix phx.gen.cert + # + # If desired, both `http:` and `https:` keys can be + + # Set a higher stacktrace during development. Avoid configuring such + config :phoenix, :stacktrace_depth, 20 + """ + ) + end end end diff --git a/test/style/deprecations_test.exs b/test/style/deprecations_test.exs index 0fbd13b7..ff6ffab9 100644 --- a/test/style/deprecations_test.exs +++ b/test/style/deprecations_test.exs @@ -138,7 +138,6 @@ defmodule Styler.Style.DeprecationsTest do describe "1.17+" do @describetag skip: Version.match?(System.version(), "< 1.17.0-dev") - test "to_timeout/1 vs :timer.units(x)" do assert_style ":timer.hours(x)", "to_timeout(hour: x)" assert_style ":timer.minutes(x)", "to_timeout(minute: x)" @@ -152,5 +151,10 @@ defmodule Styler.Style.DeprecationsTest do test "combined with to_timeout improvements" do assert_style ":timer.minutes(60 * 4)", "to_timeout(hour: 4)" end + + test "regression: doesn't touch other timer functions" do + assert_style ":timer.sleep(1000)" + assert_style ":timer.tc(fn -> :ok end)" + end end end diff --git a/test/style/module_directives/alias_lifting_test.exs b/test/style/module_directives/alias_lifting_test.exs index c3ea9cdb..99df07f4 100644 --- a/test/style/module_directives/alias_lifting_test.exs +++ b/test/style/module_directives/alias_lifting_test.exs @@ -13,6 +13,70 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do use Styler.StyleCase, async: true + test "lifts aliases in snippets" do + assert_style( + """ + A.B.C + # z + A.B.C + """, + """ + alias A.B.C + + C + # z + C + """ + ) + end + + test "snippet with a single node" do + assert_style( + """ + # 0 + { + # 1 + A.B.C, + # 2 + A.B.C, + # 3 + + # 4 + "something so long that we keep this silly tuple split across multiple lines to recreate the issue at hand" + } + """, + """ + alias A.B.C + + # 0 + { + # 1 + C, + # 2 + C, + # 3 + + # 4 + "something so long that we keep this silly tuple split across multiple lines to recreate the issue at hand" + } + """ + ) + + assert_style( + """ + { + A.B.C, + A.B.C, + } + """, + """ + alias A.B.C + + {C, C} + """ + ) + end + test "lifts aliases repeated >=2 times from 3 deep" do assert_style( """ @@ -342,19 +406,6 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do end describe "it doesn't lift" do - test "collisions with configured modules" do - Styler.Config.set!(alias_lifting_exclude: ~w(C)a) - - assert_style """ - alias Foo.Bar - - A.B.C - A.B.C - """ - - Styler.Config.set!([]) - end - test "collisions with std lib" do assert_style """ defmodule DontYouDare do diff --git a/test/style/module_directives_test.exs b/test/style/module_directives_test.exs index f32fcb58..7255abb0 100644 --- a/test/style/module_directives_test.exs +++ b/test/style/module_directives_test.exs @@ -294,7 +294,7 @@ defmodule Styler.Style.ModuleDirectivesTest do alias A.A """, """ - require A.A + require A require A.C alias A.A @@ -407,7 +407,7 @@ defmodule Styler.Style.ModuleDirectivesTest do ) end - test "de-aliases use/behaviour/import/moduledoc" do + test "expands use/behaviour/import/moduledoc aliases" do assert_style( """ defmodule MyModule do @@ -445,7 +445,7 @@ defmodule Styler.Style.ModuleDirectivesTest do end describe "module attribute lifting" do - test "replaces uses in other attributes and `use` correctly" do + test "maintains location when used in other spots" do assert_style( """ defmodule MyGreatLibrary do @@ -455,15 +455,214 @@ defmodule Styler.Style.ModuleDirectivesTest do end """, """ - library_options = [...] + defmodule MyGreatLibrary do + @library_options [...] + @moduledoc make_pretty_docs(@library_options) + + use OptionsMagic, my_opts: @library_options + end + """ + ) + end + test "interdepedent module attrs" do + assert_style( + """ + defmodule MyGreatLibrary do + @foo :bar + import Meow + @library_options @foo + @moduledoc make_pretty_docs(@library_options) + use OptionsMagic, my_opts: @library_options + end + """, + """ defmodule MyGreatLibrary do - @moduledoc make_pretty_docs(library_options) + @foo :bar + @library_options @foo + @moduledoc make_pretty_docs(@library_options) + + use OptionsMagic, my_opts: @library_options + + import Meow + end + """ + ) + end + + test "works with `quote`" do + assert_style( + """ + quote do + @library_options [...] + @moduledoc make_pretty_docs(@library_options) + use OptionsMagic, my_opts: @library_options + end + """, + """ + quote do + @library_options [...] + @moduledoc make_pretty_docs(@library_options) + + use OptionsMagic, my_opts: @library_options + end + """ + ) + end + end + + describe "apply aliases" do + test "replaces known aliases" do + assert_style( + """ + alias A.B + alias A.B.C + alias A.B.C.D, as: X + + A.B.foo() + A.B.C.foo() + A.B.C.D.woo() + C.D.woo() + """, + """ + alias A.B + alias A.B.C + alias A.B.C.D, as: X + + B.foo() + C.foo() + X.woo() + X.woo() + """ + ) + end + + test "ignores quotes" do + assert_style( + """ + alias A.B.C + + A.B.C + + quote do + A.B.C + end + """, + """ + alias A.B.C + + C + + quote do + A.B.C + end + """ + ) + end + + test "removes embedded duplicate aliases" do + assert_style( + """ + alias A.B + + def foo do + alias A.B + A.B.bar() + end + """, + """ + alias A.B - use OptionsMagic, my_opts: library_options + def foo do + B.bar() + end + """ + ) + end - @library_options library_options + test "doesn't rewrite the LHS of `alias X, as: Y` directives" do + # exact-match dedup: inner `alias` is the same module + same `as` as outer, so it's removed + assert_style( + """ + defmodule Outer do + @moduledoc false + alias Foo.Bar.Baz, as: BazSchema + + defmodule Inner do + @moduledoc false + alias Foo.Bar.Baz, as: BazSchema + + def x, do: BazSchema + end end + """, + """ + defmodule Outer do + @moduledoc false + + alias Foo.Bar.Baz, as: BazSchema + + defmodule Inner do + @moduledoc false + + def x, do: BazSchema + end + end + """ + ) + + # different `as`: inner alias stays put and its LHS is left alone + assert_style( + """ + defmodule Outer do + @moduledoc false + alias Foo.Bar.Baz, as: BazSchema + + defmodule Inner do + @moduledoc false + alias Foo.Bar.Baz, as: SomethingElse + + def x, do: SomethingElse + end + end + """, + """ + defmodule Outer do + @moduledoc false + + alias Foo.Bar.Baz, as: BazSchema + + defmodule Inner do + @moduledoc false + + alias Foo.Bar.Baz, as: SomethingElse + + def x, do: SomethingElse + end + end + """ + ) + end + + test "forces a single alias" do + assert_style( + """ + alias A.B.C.D.E, as: B + alias A.B.C.D.E, as: C + alias A.B.C.D.E + + B + C + E + """, + """ + alias A.B.C.D.E + alias A.B.C.D.E, as: B + alias A.B.C.D.E, as: C + + C + C + C """ ) end diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 50284537..570f5b91 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -11,7 +11,7 @@ defmodule Styler.Style.PipesTest do use Styler.StyleCase, async: true - describe "big picture" do + describe "big picture / readability" do test "unnests multiple steps" do assert_style("f(g(h(x))) |> j()", "x |> h() |> g() |> f() |> j()") end @@ -78,6 +78,16 @@ defmodule Styler.Style.PipesTest do """ ) end + + test "rewrites anonymous function invocations to use then" do + assert_style("a |> (& &1).()", "then(a, & &1)") + assert_style("a |> (& {&1, &2}).(b)", "(&{&1, &2}).(a, b)") + assert_style("a |> (& &1).() |> c", "a |> then(& &1) |> c()") + + assert_style("a |> (fn x, y -> {x, y} end).() |> c", "a |> then(fn x, y -> {x, y} end) |> c()") + assert_style("a |> (fn x -> x end).()", "then(a, fn x -> x end)") + assert_style("a |> (fn x -> x end).() |> c", "a |> then(fn x -> x end) |> c()") + end end describe "block pipe starts" do @@ -184,6 +194,7 @@ defmodule Styler.Style.PipesTest do x = case y do :ok -> :ok |> IO.puts() + :error -> :error end |> bar() |> baz() @@ -192,6 +203,7 @@ defmodule Styler.Style.PipesTest do case_result = case y do :ok -> IO.puts(:ok) + :error -> :error end x = @@ -281,16 +293,13 @@ defmodule Styler.Style.PipesTest do assert_style( """ case x do - x -> x + y -> z end |> foo() """, """ - case_result = - case x do - x -> x - end - + y = x + case_result = z foo(case_result) """ ) @@ -300,6 +309,7 @@ defmodule Styler.Style.PipesTest do def foo do case x do x -> x + y -> y end |> foo() end @@ -309,6 +319,7 @@ defmodule Styler.Style.PipesTest do case_result = case x do x -> x + y -> y end foo(case_result) @@ -455,8 +466,8 @@ defmodule Styler.Style.PipesTest do end test "writes brackets for unpiped kwl" do - assert_style("foo(kwl: :arg) |> bar()", "[kwl: :arg] |> foo() |> bar()") - assert_style("%{a: foo(a: :b, c: :d) |> bar()}", "%{a: [a: :b, c: :d] |> foo() |> bar()}") + assert_style("foo(kwl: arg) |> bar()", "[kwl: arg] |> foo() |> bar()") + assert_style("%{a: foo(a: b, c: :d) |> bar()}", "%{a: [a: b, c: :d] |> foo() |> bar()}") assert_style("%{a: foo([a: :b, c: :d]) |> bar()}", "%{a: [a: :b, c: :d] |> foo() |> bar()}") end @@ -500,17 +511,7 @@ defmodule Styler.Style.PipesTest do end end - describe "simple rewrites" do - test "rewrites anonymous function invocations to use then" do - assert_style("a |> (& &1).()", "then(a, & &1)") - assert_style("a |> (& {&1, &2}).(b)", "(&{&1, &2}).(a, b)") - assert_style("a |> (& &1).() |> c", "a |> then(& &1) |> c()") - - assert_style("a |> (fn x, y -> {x, y} end).() |> c", "a |> then(fn x, y -> {x, y} end) |> c()") - assert_style("a |> (fn x -> x end).()", "then(a, fn x -> x end)") - assert_style("a |> (fn x -> x end).() |> c", "a |> then(fn x -> x end) |> c()") - end - + describe "readability" do test "rewrites then/2 when the passed function is a named function reference" do assert_style "a |> then(&fun/1) |> c", "a |> fun() |> c()" assert_style "a |> then(&(&1 / 1)) |> c", "a |> Kernel./(1) |> c()" @@ -541,6 +542,66 @@ defmodule Styler.Style.PipesTest do test "adds parens to 1-arity pipes" do assert_style("a |> b |> c", "a |> b() |> c()") end + end + + describe "optimizations for stdlib functions" do + test "map/intersperse => map_intersperse" do + assert_style "a |> Enum.map(fun) |> Enum.intersperse(sep)", "Enum.map_intersperse(a, sep, fun)" + + assert_style( + """ + a + |> Enum.map(fun) + |> Enum.intersperse(sep) + |> foo() + """, + """ + a + |> Enum.map_intersperse(sep, fun) + |> foo() + """ + ) + end + + test "filter/first => find" do + assert_style "a |> Enum.filter(fun) |> List.first()", "Enum.find(a, fun)" + assert_style "a |> Enum.filter(fun) |> List.first(default)", "Enum.find(a, default, fun)" + + assert_style( + """ + a + |> Enum.filter(fun) + |> List.first() + |> foo() + """, + """ + a + |> Enum.find(fun) + |> foo() + """ + ) + + assert_style( + """ + a + |> Enum.filter(fun) + |> List.first(default) + |> foo() + """, + """ + a + |> Enum.find(default, fun) + |> foo() + """ + ) + end + + test "Enum.sort/Enum.reverse" do + assert_style("a |> Enum.sort(direction) |> Enum.reverse()") + assert_style("a |> Enum.sort(:asc) |> Enum.reverse()", "Enum.sort(a, :desc)") + assert_style("a |> Enum.sort(:desc) |> Enum.reverse()", "Enum.sort(a, :asc)") + assert_style("a |> Enum.sort() |> Enum.reverse()", "Enum.sort(a, :desc)") + end test "reverse/concat" do assert_style("a |> Enum.reverse() |> Enum.concat()") @@ -970,4 +1031,31 @@ defmodule Styler.Style.PipesTest do ) end end + + describe "Req pipes" do + @req2 for fun <- ~w(delete get head patch post put request run), bang <- ["", "!"], do: :"#{fun}#{bang}" + + test "X.merge |> Req.foo/1 -> Req.foo/2" do + for fun <- @req2, merger <- ~w(Req Keyword) do + assert_style "foo |> #{merger}.merge(opts) |> Req.#{fun}()", "Req.#{fun}(foo, opts)" + assert_style "a |> b |> #{merger}.merge(opts) |> Req.#{fun}()", "a |> b() |> Req.#{fun}(opts)" + end + end + + test "Req.new |> Req.foo/1 -> Req.foo/2" do + for fun <- @req2 do + assert_style "foo |> Req.new() |> Req.#{fun}()", "Req.#{fun}(foo)" + assert_style "a |> b |> Req.new() |> Req.#{fun}()", "a |> b() |> Req.#{fun}()" + assert_style "foo |> Req.new() |> Req.#{fun}(c)", "Req.#{fun}(foo, c)" + assert_style "a |> b |> Req.new() |> Req.#{fun}(c)", "a |> b() |> Req.#{fun}(c)" + end + end + + test "new |> merge |> foo" do + for fun <- @req2 do + assert_style "foo |> Req.new() |> Req.merge(bar) |> Req.#{fun}()", "Req.#{fun}(foo, bar)" + assert_style "a |> b() |> Req.new() |> Req.merge(bar) |> Req.#{fun}()", "a |> b() |> Req.#{fun}(bar)" + end + end + end end diff --git a/test/style/single_node_test.exs b/test/style/single_node_test.exs index 00ec66ec..8717fd16 100644 --- a/test/style/single_node_test.exs +++ b/test/style/single_node_test.exs @@ -11,19 +11,77 @@ defmodule Styler.Style.SingleNodeTest do use Styler.StyleCase, async: true + describe "assert/refute" do + test "negation" do + assert_style "assert x != nil", "assert x" + assert_style "assert !!x", "assert x" + assert_style "assert !x", "refute x" + assert_style "assert not x", "refute x" + assert_style "assert !is_nil(x)", "assert x" + assert_style "assert x not in y" + + assert_style "assert nil == nil", "assert nil == nil" + assert_style "assert nil != nil", "assert nil" + + assert_style "refute x != y", "assert x == y" + assert_style "refute x !== y", "assert x === y" + assert_style "refute x != nil", "assert x == nil" + assert_style "refute !x", "assert x" + assert_style "refute not x", "assert x" + assert_style "refute x not in y" + + assert_style "assert x == nil" + assert_style "assert is_nil(x)" + assert_style "refute x" + end + + test "Enum.member? -> in" do + assert_style "assert Enum.member?(enum, x)", "assert x in enum" + assert_style "refute Enum.member?(enum, x)", "refute x in enum" + assert_style "assert not Enum.member?(enum, x)", "refute x in enum" + assert_style "refute not Enum.member?(enum, x)", "assert x in enum" + end + + test "Enum.find -> any?" do + assert_style "assert Enum.find(x, y)", "assert Enum.any?(x, y)" + assert_style "refute Enum.find(x, y)", "refute Enum.any?(x, y)" + end + + test "Enum.any?(x, & &1 == y) -> y in x" do + assert_style "assert Enum.any?(x, y)" + assert_style "assert Enum.any?(x, &(&1.x == y))" + assert_style "assert Enum.any?(x, &(&1 != y))" + assert_style "assert Enum.any?(y, & &1 == x)", "assert x in y" + assert_style "assert Enum.any?(y, fn q -> q == x end)", "assert x in y" + assert_style "assert Enum.find(y, & &1 == x)", "assert x in y" + assert_style "assert Enum.find(y, fn q -> q == x end)", "assert x in y" + end + + test "boolean comparisons" do + assert_style "assert x < y" + assert_style "assert x <= y" + assert_style "assert x > y" + assert_style "assert x >= y" + assert_style "refute x < y", "assert x >= y" + assert_style "refute x <= y", "assert x > y" + assert_style "refute x > y", "assert x <= y" + assert_style "refute x >= y", "assert x < y" + end + end + test "string sigil rewrites" do assert_style ~s|""| - assert_style ~s|"\\""| - assert_style ~s|"\\"\\""| - assert_style ~s|"\\"\\"\\""| - assert_style ~s|"\\"\\"\\"\\""|, ~s|~s("""")| + assert_style ~S|"\""| + assert_style ~S|"\"\""| + assert_style ~S|"\"\"\""| + assert_style ~S|"\"\"\"\""|, ~s|~s("""")| # choose closing delimiter wisely, based on what has the least conflicts, in the styliest order - assert_style ~s/"\\"\\"\\"\\" )"/, ~s/~s{"""" )}/ - assert_style ~s/"\\"\\"\\"\\" })"/, ~s/~s|"""" })|/ - assert_style ~s/"\\"\\"\\"\\" |})"/, ~s/~s["""" |})]/ - assert_style ~s/"\\"\\"\\"\\" ]|})"/, ~s/~s'"""" ]|})'/ - assert_style ~s/"\\"\\"\\"\\" ']|})"/, ~s/~s<"""" ']|})>/ - assert_style ~s/"\\"\\"\\"\\" >']|})"/, ~s|~s/"""" >']\|})/| + assert_style ~S/"\"\"\"\" )"/, ~s/~s{"""" )}/ + assert_style ~S/"\"\"\"\" })"/, ~s/~s|"""" })|/ + assert_style ~S/"\"\"\"\" |})"/, ~s/~s["""" |})]/ + assert_style ~S/"\"\"\"\" ]|})"/, ~s/~s'"""" ]|})'/ + assert_style ~S/"\"\"\"\" ']|})"/, ~s/~s<"""" ']|})>/ + assert_style ~S/"\"\"\"\" >']|})"/, ~s|~s/"""" >']\|})/| assert_style ~s/"\\"\\"\\"\\" \/>']|})"/, ~s|~s("""" />']\|}\\))| end @@ -186,11 +244,13 @@ defmodule Styler.Style.SingleNodeTest do """ case foo do bar = _ -> :ok + _ -> :error end """, """ case foo do bar -> :ok + _ -> :error end """ ) @@ -199,11 +259,13 @@ defmodule Styler.Style.SingleNodeTest do """ case foo do _ = bar -> :ok + _ = second_clause_to_maintain_case -> :ok end """, """ case foo do bar -> :ok + second_clause_to_maintain_case -> :ok end """ ) @@ -334,10 +396,15 @@ defmodule Styler.Style.SingleNodeTest do assert_style "to_timeout(second: 60 * 60)", "to_timeout(hour: 1)" end - test "doesnt mess with" do + test "plurals and multiples oh my" do + assert_style "to_timeout(hours: 24 * 1, seconds: 60 * 4)", "to_timeout(day: 1, minute: 4)" + # nb: this'll raise an argument error after styling. + assert_style "to_timeout(minute: 60, hours: 3)", "to_timeout(hour: 1, hour: 3)" + end + + test "doesnt mess with non-integers" do assert_style "to_timeout(hour: n * m)" assert_style "to_timeout(whatever)" - assert_style "to_timeout(hour: 24 * 1, second: 60 * 4)" end end end diff --git a/test/style_test.exs b/test/style_test.exs index 43bc8b27..e7d074ac 100644 --- a/test/style_test.exs +++ b/test/style_test.exs @@ -1,3 +1,13 @@ +# Copyright 2024 Adobe. All rights reserved. +# This file is licensed to you under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. You may obtain a copy +# of the License at http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +# OF ANY KIND, either express or implied. See the License for the specific language +# governing permissions and limitations under the License. + defmodule Styler.StyleTest do use ExUnit.Case, async: true diff --git a/test/zipper_test.exs b/test/zipper_test.exs index 30d80d52..2359d481 100644 --- a/test/zipper_test.exs +++ b/test/zipper_test.exs @@ -63,14 +63,14 @@ defmodule StylerTest.ZipperTest do describe "down/1" do test "rips and tears the parent node" do - assert [1, 2] |> Zipper.zip() |> Zipper.down() == {1, {[], {[1, 2], nil}, [2]}} - assert {1, 2} |> Zipper.zip() |> Zipper.down() == {1, {[], {{1, 2}, nil}, [2]}} + assert [1, 2] |> Zipper.zip() |> Zipper.down() == {1, %{l: [], r: [2], ptree: {[1, 2], nil}}} + assert {1, 2} |> Zipper.zip() |> Zipper.down() == {1, %{l: [], r: [2], ptree: {{1, 2}, nil}}} assert {:foo, [], [1, 2]} |> Zipper.zip() |> Zipper.down() == - {1, {[], {{:foo, [], [1, 2]}, nil}, [2]}} + {1, %{l: [], r: [2], ptree: {{:foo, [], [1, 2]}, nil}}} assert {{:., [], [:a, :b]}, [], [1, 2]} |> Zipper.zip() |> Zipper.down() == - {{:., [], [:a, :b]}, {[], {{{:., [], [:a, :b]}, [], [1, 2]}, nil}, [1, 2]}} + {{:., [], [:a, :b]}, %{l: [], r: [1, 2], ptree: {{{:., [], [:a, :b]}, [], [1, 2]}, nil}}} end end @@ -471,8 +471,8 @@ defmodule StylerTest.ZipperTest do end test "builds a new root node made of a block" do - assert {42, {[:nope], {{:__block__, _, _}, nil}, []}} = 42 |> Zipper.zip() |> Zipper.insert_left(:nope) - assert {42, {[], {{:__block__, _, _}, nil}, [:nope]}} = 42 |> Zipper.zip() |> Zipper.insert_right(:nope) + assert {42, %{l: [:nope], ptree: {{:__block__, _, _}, nil}}} = 42 |> Zipper.zip() |> Zipper.insert_left(:nope) + assert {42, %{r: [:nope], ptree: {{:__block__, _, _}, nil}}} = 42 |> Zipper.zip() |> Zipper.insert_right(:nope) end end