New Check: StrictModuleDirectiveScope

[stevebissett]

New Check: StrictModuleDirectiveScope

Summary

Add a new readability check that enforces module directives (alias, require, import, use) are defined at the module level rather than inside function bodies.

Motivation

Module directives that appear inside function bodies can make code harder to follow and obscure module dependencies. By requiring all directives to be declared at the module level, readers can quickly understand a module's dependencies by looking at the top of the file.

Current Situation

Elixir allows module directives to be scoped to function bodies, which can lead to code like this:

defmodule MyModule do
  def process_data(data) do
    alias MyApp.DataProcessor
    alias MyApp.Validator
    require Logger

    data
    |> Validator.validate()
    |> DataProcessor.process()
    |> then(fn result ->
      Logger.info("Processed: #{inspect(result)}")
      result
    end)
  end
end

While this is valid Elixir, it has several drawbacks:

1. Hidden dependencies: Module dependencies are not immediately visible
2. Reduced scanability: Readers must examine each function to understand imports
3. Inconsistency: Mixed patterns where some directives are at module level, others inline
4. Refactoring friction: Moving code between functions requires moving directives

Proposed Improvement

The check would encourage this style instead:

defmodule MyModule do
  alias MyApp.DataProcessor
  alias MyApp.Validator
  require Logger

  def process_data(data) do
    data
    |> Validator.validate()
    |> DataProcessor.process()
    |> then(fn result ->
      Logger.info("Processed: #{inspect(result)}")
      result
    end)
  end
end

Proposed Check Details

Name

Credo.Check.Readability.StrictModuleDirectiveScope

Configuration

• ID: EX5027
• Category: :readability
• Base Priority: :low
• Tags: [:controversial] (disabled by default)

Parameters

param_defaults: [
  # Which directives to check
  directives: [:alias, :require, :import, :use],

  # Allow directives in private functions (some teams prefer this)
  allow_in_private_functions: false,

  # Allow in test setup blocks (setup, describe, test macros)
  allow_in_test_macros: true,

  # Allow in quote blocks (macros that generate code for callers)
  allow_in_quote_blocks: true,

  # Function names to exclude from checking (regex patterns)
  exclude_functions: []
]

Detection Scope

The check should detect directives in:

1. Function bodies (def, defp)

   def foo do
     alias Bar  # ❌ Issue
   end

2. Macro bodies (defmacro, defmacrop)

   defmacro create_thing do
     require Logger  # ❌ Issue (unless in quote block)
   end

3. Nested control structures

   def foo(x) do
     if x > 10 do
       alias Bar  # ❌ Issue
       Bar.process(x)
     end
   end

4. Multi-clause functions

   def foo(:ok) do
     alias Success  # ❌ Issue
     Success.handle()
   end
   
   def foo(:error) do
     alias Failure  # ❌ Issue
     Failure.handle()
   end

5. Anonymous functions (controversial - could be excluded)

   fn ->
     alias Foo  # ❌ Issue (maybe?)
     Foo.bar()
   end

Smart Skip Logic

The check should NOT flag directives in:

1. Quote blocks (macros generating code for callers)

   defmacro my_macro do
     quote do
       alias MyModule  # ✅ OK - generates code for caller
       MyModule.do_something()
     end
   end

2. Test helper macros (when allow_in_test_macros: true)

   setup do
     alias MyApp.Factory  # ✅ OK in test setup
     {:ok, user: Factory.insert(:user)}
   end
   
   test "something" do
     alias MyApp.Helper  # ✅ OK in test macro
     assert Helper.works?()
   end

3. Private functions (when allow_in_private_functions: true)

   defp helper do
     alias Internal  # ✅ OK if configured to allow
     Internal.do_thing()
   end

4. Module-level scope

   defmodule MyModule do
     alias Foo  # ✅ OK - module level
   
     def bar, do: Foo.baz()
   end

Error Messages

"Alias Foo.Bar should be defined at module level, not inside function process_data/1"
"Import Logger should be defined at module level, not inside private function log_it/0"
"Require Protocol should be defined at module level, not inside function check/2"
"Use MyBehaviour should be defined at module level, not inside defmacro create/1"

Implementation Considerations

AST Traversal Strategy

The check needs to:

1. Track context (are we inside a function/macro?)
2. Detect all four directive types
3. Handle nested structures (if, case, with, cond, try, rescue)
4. Identify quote blocks to skip
5. Identify test macros to skip (optional)

Controversy & Trade-offs

Why mark as controversial?

Some teams and use cases legitimately prefer inline directives:

1. Namespace scoping: Limiting alias scope to where it's needed
2. Phoenix LiveView components: Inline aliases in render functions
3. Test organization: Grouping test-specific aliases with tests
4. Macro hygiene: Keeping macro-internal dependencies localized
5. Legacy codebases: Large refactoring burden

Counter-arguments for the check:

1. Consistency: Encourages predictable module structure
2. Discoverability: All dependencies visible at module top
3. Tooling: Editor jump-to-definition works better
4. Refactoring: Easier to move code between functions
5. Code review: Dependencies clear in PR diffs

Compatibility with Other Checks

• StrictModuleLayout: Complementary - enforces order of module-level directives
• AliasUsage: Complementary - enforces when to alias vs. full module names
• UnusedOperation: May conflict if directives are conditionally used

Examples

Basic Usage

.credo.exs

%{
  configs: [
    %{
      name: "default",
      checks: [
        # Disabled by default (controversial)
        {Credo.Check.Readability.ModuleDirectiveScope, false}
      ]
    }
  ]
}

Opt-in Configuration

{Credo.Check.Readability.StrictModuleDirectiveScope, []}

Relaxed Configuration (allow in private functions)

{Credo.Check.Readability.StrictModuleDirectiveScope, [
  allow_in_private_functions: true
]}

Strict Configuration (check all directives, no exceptions)

{Credo.Check.Readability.StrictModuleDirectiveScope, [
  directives: [:alias, :require, :import, :use],
  allow_in_private_functions: false,
  allow_in_test_macros: false,
  allow_in_quote_blocks: true  # Still allow in macros
]}

Exclude specific functions

{Credo.Check.Readability.StrictModuleDirectiveScope, [
  exclude_functions: [~r/^render/, ~r/_test$/]
]}

Related Checks

• Credo.Check.Readability.StrictModuleLayout - Enforces order of module-level parts
• Credo.Check.Design.AliasUsage - Enforces when to use alias vs. full names
• Credo.Check.Readability.ModuleDoc - Enforces module documentation

Questions for Maintainers

1. ID Assignment: Is EX5027 the correct next ID for readability checks?
2. Anonymous functions: Should we flag directives in fn -> alias Foo end or skip them?
3. Comprehensions: Should we check inside for comprehensions?
4. Default params: Are the proposed defaults reasonable?
5. Naming: Does StrictModuleDirectiveScope clearly complement StrictModuleLayout?

Implementation Checklist

• Implement check with all configuration options
• Comprehensive test suite covering:
  • All four directive types (alias, require, import, use)
  • Nested control structures (if, case, with, cond, try)
  • Multi-clause functions
  • Quote block detection and skipping
  • Test macro detection and skipping
  • Private function handling
  • Configuration parameter handling
  • Edge cases (anonymous functions, comprehensions)
• Documentation with examples
• Update CHANGELOG.md
• Ensure check is disabled by default (controversial tag)

Additional Context

This check was inspired by real-world experience maintaining a large Elixir codebase where inline directives made it difficult to understand module dependencies and refactor code. The goal is to provide teams who value this style with a tool to enforce it, while respecting that other teams may have valid reasons to use inline directives.

The controversial tag ensures this doesn't break existing codebases and allows teams to opt-in consciously.

[rrrene]

Hi, thx for opening this issue.

I am sometimes not getting back to people in a reasonable timeframe and sometimes that is due to not finding the right words to frame an argument or critique.

I am also wary that I might be misunderstood due to this being text and me not being a native speaker.

But I do not want to forget about this issue just because I want to find a more polite way to say:

I REALLY love the idea. Unfortunately, I really, really dislike the name, parameters and much of the considerations section.

To start a conversation: Why would you want to allow the very thing this check guards against (calls to directives or use) in private functions?

[stevebissett]

Hi @rrrene - Thank you for the response. I'm very welcome to open critique, so don't hold back.

This is working fine for me in a custom check, but myself and my team were surprised that this check doesn't exist, so I thought to share it.

I'd be happy to radically simplify the approach with your feedback.

You raise a great point about allow_in_private_functions - honestly, it does contradict the whole purpose of the check. I included it thinking some teams might want that flexibility, but I can see how it undermines the goal of having all dependencies visible at the module level.

Would you like me to strip this down to be simpler in the meantime?
Or perhaps you could paint out how you would want to approach this, potentially with naming suggestions. For the name, I was trying to match the pattern of StrictModuleLayout,

[leonqadirie]

For what it is worth, I just shipped a variation of this in (the highly experimental) AshCredo and only then got the idea to look into the issues here... DirectiveInFunctionBody.

Would be delighted to cull my check in favor of a more 'canonical' check in credo itself.
For such a check I'd suggest

• either an opt-in or opt-out list of modules which are flagged if their directives are not at the module level (probably opt-out, default empty)
• the directives to include with the default set to all 3.

I agree test/macro definitions are something to consider, haven't formed a clear opinion on it yet.

My check is definitely very Ash-opinionated, I tried to be non-intrusive by requiring to opt-into modules to check this for, with two common Ash modules set as the default. The machinery underneath is relatively generic, though.

[stevebissett]

Thank you for looking into this. We just stuck with a custom check in our repo and continued with that for now which has been working fine. I'll have a look at the newly shipped directive and see if it suits, but I don't think I'll get to that very soon.