Skip to content

fix(config): surface invalid config and honor configured SQL extensions - #2658

Open
nikolaevx wants to merge 33 commits into
mainfrom
config-early-behavior-fixes
Open

fix(config): surface invalid config and honor configured SQL extensions#2658
nikolaevx wants to merge 33 commits into
mainfrom
config-early-behavior-fixes

Conversation

@nikolaevx

Copy link
Copy Markdown
Collaborator

Summary

  • Honor configured sql_file_exts during path discovery.
  • Surface invalid configuration errors instead of silently falling back to defaults.
  • Add/adjust coverage for config loading and configured file discovery behavior.

Testing

  • cargo check -p sqruff-lib

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors config loading, templating, and execution around a new sqruff_lib::api layer (Engine/Workspace), so invalid configuration is surfaced instead of silently falling back, and path discovery honors configured SQL extensions and ignore rules more consistently across CLI/LSP/WASM.

Changes:

  • Introduce sqruff_lib::api (Engine, Workspace, structured diagnostics/reports, and SqruffError) and migrate CLI/LSP/WASM to use it.
  • Rework config loading to return structured errors (try_from_source, from_file, from_root) and add coverage for invalid config behavior.
  • Replace previous linter path discovery with an API-based implementation that supports configured file extensions and .sqruffignore.

Reviewed changes

Copilot reviewed 68 out of 69 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
crates/lsp/src/lib.rs Switch LSP to Engine, surface startup/config errors, add ignore handling and LSP-focused tests.
crates/lsp/Cargo.toml Add ignore dependency for .sqruffignore parsing.
crates/lsp/BUILD.bazel Include ignore in WASM crate list.
crates/lib/tests/templaters.rs Update templater tests for new TemplaterInput/Output and config parsing API.
crates/lib/tests/rules.rs Update rule tests for new Mode/ParseErrors and config parsing changes.
crates/lib/src/utils/reflow/respace.rs Update test linter construction for new ParseErrors and config parsing API.
crates/lib/src/utils/reflow/reindent.rs Update test linter invocation for new Mode/ParseErrors.
crates/lib/src/tests.rs Update tests for new ParseErrors linter constructor signature.
crates/lib/src/templaters/types.rs Remove deprecated TemplaterKind::templater() static mapping.
crates/lib/src/templaters/raw.rs Update templater interface to typed inputs/outputs and SourceId.
crates/lib/src/templaters/python.rs Update python templater interface to typed inputs/outputs and SourceId naming.
crates/lib/src/templaters/python_shared.rs Update config parsing usage in tests.
crates/lib/src/templaters/placeholder.rs Update placeholder templater interface to typed inputs/outputs and SourceId.
crates/lib/src/templaters/jinja.rs Update jinja templater interface to typed inputs/outputs and SourceId.
crates/lib/src/templaters/dbt.rs Update dbt templater integration to support “skipped” outputs from Python batch processing.
crates/lib/src/templaters.rs Introduce TemplaterRuntime, typed templater I/O, and simplify templater enumeration via TemplaterKind.
crates/lib/src/rules/aliasing/al05.rs Update rule tests for new config parsing and lint modes.
crates/lib/src/lib.rs Expose new api module; remove legacy Formatter trait.
crates/lib/src/core/test_functions.rs Update helper linter construction for new ParseErrors.
crates/lib/src/core/rules/noqa.rs Update tests to use try_from_source and new Mode/ParseErrors.
crates/lib/src/core/linter/linted_file.rs Make path private and add source() accessor for Engine diagnostics conversion.
crates/lib/src/core/linter/core.rs Refactor linter templating/rendering for typed templater outputs; remove legacy path discovery/lint_paths.
crates/lib/src/core/linter/common.rs Replace batch-render result with RenderedSource (rendered vs skipped).
crates/lib/src/core/config.rs Convert config loading/parsing to Result-based APIs; improve error surfacing for invalid config values.
crates/lib/src/api/workspace.rs New Workspace API for ignore + path discovery + applying fixes.
crates/lib/src/api/source.rs New Source/SourceId types for consistent identity handling.
crates/lib/src/api/report.rs New RunReport/FileReport/SkipReason reporting types.
crates/lib/src/api/options.rs New Mode, ParseErrors, EngineOptions, and RunRequest.
crates/lib/src/api/error.rs New SqruffError error type with structured variants and messages.
crates/lib/src/api/engine.rs New Engine API to check/fix/run sources, reload config, and produce FileReports.
crates/lib/src/api/diagnostic.rs New LintDiagnostic with canonical byte-range mapping and tests.
crates/lib/src/api.rs Export the new API surface.
crates/lib/Cargo.toml Add ignore and thiserror dependencies for ignore/config errors and structured errors.
crates/lib/BUILD.bazel Include ignore and thiserror in WASM crate list.
crates/lib/benches/fix.rs Update benchmark usage for new Mode/ParseErrors.
crates/lib/benches/depth_map.rs Update benchmark linter construction for new ParseErrors.
crates/lib-wasm/src/lib.rs Migrate wasm linter to Engine for formatting and to typed diagnostics; improve error handling.
crates/lib-core/src/errors.rs Preserve source_slice when converting parse/lex errors to SQLBaseError.
crates/cli/tests/ui.rs Normalize Windows path separators in UI output comparisons.
crates/cli/tests/ui_json.rs Normalize Windows path escaping/separators for JSON UI output comparisons.
crates/cli/tests/ui_github.rs Normalize Windows path separators in GitHub annotation output comparisons.
crates/cli/tests/path_discovery.rs Add CLI-level tests for ignore pruning, explicit ignored files, and missing-path errors.
crates/cli/tests/json/test_fail_whitespace_before_comma.stdout Update expected JSON ranges to use non-empty end positions.
crates/cli/tests/json/hql_file.stdout Update expected JSON ranges to use non-empty end positions.
crates/cli/tests/ignore_data_directory.rs Update ignore traversal test to use Workspace discovery API.
crates/cli/tests/fix_return_code.rs Add coverage to assert fixes write only when needed and don’t rewrite unchanged files.
crates/cli/tests/fix_parse_errors.rs Add coverage ensuring parse errors don’t rewrite files.
crates/cli/Cargo.toml Register new path_discovery harness-less test.
crates/cli-python/tests/json/test_fail_whitespace_before_comma.stdout Update expected JSON ranges to match new diagnostic end positions.
crates/cli-python/tests/json/hql_file.stdout Update expected JSON ranges to match new diagnostic end positions.
crates/cli-python/python/sqruff/templaters/dbt_templater.py Extend batch API to return skip reasons separately from errors.
crates/cli-python/python/sqruff/templaters/dbt_templater_test.py Update tests for new (templated_file, error, skip_reason) tuple shape.
crates/cli-python/python/sqruff/templaters/dbt_templater_benchmark.py Update benchmark loop for new tuple shape.
crates/cli-lib/src/reporters/json.rs New JSON reporter driven by RunReport instead of legacy formatter callbacks.
crates/cli-lib/src/reporters/human.rs New human reporter emitting diagnostics/skips from RunReport.
crates/cli-lib/src/reporters/github.rs New GitHub annotation reporter emitting from LintDiagnostic.
crates/cli-lib/src/reporters.rs Reporter abstraction + display_source_id helper.
crates/cli-lib/src/lib.rs Switch CLI driver to ParseErrors enum and new LSP error handling; remove legacy linter/formatter creation.
crates/cli-lib/src/formatters/json.rs Remove legacy JSON formatter (replaced by reporter).
crates/cli-lib/src/formatters/json_types.rs Update JSON diagnostic types to convert from LintDiagnostic.
crates/cli-lib/src/formatters/github_annotation_native_formatter.rs Remove legacy GitHub annotation formatter (replaced by reporter).
crates/cli-lib/src/formatters.rs Refactor stream formatter to format LintDiagnostic and SkipReason; add rule-name lookup by code.
crates/cli-lib/src/docs.rs Update docs generation to iterate TemplaterKind and instantiate via TemplaterRuntime.
crates/cli-lib/src/commands.rs Restrict parsing_errors visibility and adjust CLI plumbing.
crates/cli-lib/src/commands_templaters.rs Print templater names from TemplaterKind strings.
crates/cli-lib/src/commands_parse.rs Update parse command to use new ParseErrors and linter ctor signature.
crates/cli-lib/src/commands_lint.rs Rewrite lint command around Engine + Workspace + reporters; honor configured extensions in discovery.
crates/cli-lib/src/commands_fix.rs Rewrite fix command to reuse lint runner with Mode::Fix and apply fixes via Workspace.
Cargo.lock Lockfile updates for new deps (ignore, thiserror).
Comments suppressed due to low confidence (2)

crates/lsp/src/lib.rs:274

  • When reloading config on save, set_config() failure is always logged as "Invalid templater". Since engine.reload_config() can fail for multiple reasons, log the actual error to make troubleshooting possible.
    crates/lsp/src/lib.rs:274
  • When reloading config on save, set_config() failure is always logged as "Invalid templater". Since engine.reload_config() can fail for multiple reasons, log the actual error to make troubleshooting possible.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/lib/src/core/linter/core.rs
Comment thread crates/lib/src/core/config.rs
Comment thread crates/lsp/src/lib.rs Outdated
Comment thread crates/lsp/src/lib.rs Outdated
Comment thread crates/lib/src/core/linter/core.rs
Comment thread crates/lib/src/core/config.rs
Comment thread crates/lsp/src/lib.rs Outdated
Comment thread crates/lsp/src/lib.rs Outdated
@nikolaevx
nikolaevx force-pushed the config-early-behavior-fixes branch from 8609cf0 to 4ed9f7b Compare June 1, 2026 08:56
@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Benchmark for cf69afa

Click to view benchmark
Test Base PR %
DepthMap::from_parent 59.7±0.63µs 65.8±1.77µs +10.22%
fix_complex_query 12.2±0.15ms 12.3±0.06ms +0.82%
fix_superlong 191.9±7.21ms 178.8±6.47ms -6.83%
parse_complex_query 4.1±0.11µs 4.1±0.14µs 0.00%
parse_expression_recursion 6.9±0.10µs 6.9±0.07µs 0.00%
parse_simple_query 1064.3±34.29ns 1051.7±30.60ns -1.18%

@codspeed-hq

codspeed-hq Bot commented Jun 1, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 6 untouched benchmarks
⏩ 6 skipped benchmarks1


Comparing config-early-behavior-fixes (ef4f743) with main (1960ca2)

Open in CodSpeed

Footnotes

  1. 6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@nikolaevx

Copy link
Copy Markdown
Collaborator Author

@codspeedbot fix this regression

@codspeed-hq

codspeed-hq Bot commented Jun 1, 2026

Copy link
Copy Markdown

@codspeedbot fix this regression

Performance regression fixed ✅

I investigated the -15.43% regression reported by CodSpeed on from_parent and fix_complex_query and traced it to unnecessary heap allocations introduced in the refactored rendering pipeline.

Root cause

The new render_stringrender_sourcerender_sourcesprocess_templater_singleprocess_templater_batch call chain was creating multiple intermediate allocations for what is fundamentally a single-source operation:

  • 3 redundant SourceId clones (each containing a heap-allocated String)
  • Premature into_owned() on normalise_newlines (forcing a String copy even when no replacement occurred)
  • Multiple Vec allocations for intermediate collections (NormalizedSource, TemplaterInput, RenderedSource)
  • Double String allocation in the raw templater's process_single (source_id_name() returns an owned String, then .to_string() cloned it again)

These setup-phase allocations left the heap in a more fragmented state, causing the benchmark loops' allocations and deallocations to become significantly more expensive — visible in the flamegraph as malloc_consolidate and atomic operations that were absent in the base.

Fix

Optimized render_string and render_source to use direct paths that call the templater without going through the multi-source pipeline, eliminating the unnecessary clones and intermediate collections. The batch path (render_sources) remains unchanged for the Engine API.

Results

CodSpeed simulation comparison against main shows all benchmarks unchanged — the regression is fully resolved:

Benchmark Before fix After fix
from_parent -16.55% ❌ ✅ Unchanged
fix_complex_query -14.3% ❌ ✅ Unchanged
fix_superlong ✅ Unchanged
Summary
  • Warming up the performance wizard...
  • Investigating regression — Eliminated 15.4% performance regression by reducing heap allocations in the rendering pipeline. CodSpeed simulation confirms no regression vs main.
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

@codspeed-hq
codspeed-hq Bot force-pushed the config-early-behavior-fixes branch from b92315d to c6d3541 Compare June 1, 2026 09:59
@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Benchmark for 31fde4f

Click to view benchmark
Test Base PR %
DepthMap::from_parent 61.2±1.30µs 61.5±2.06µs +0.49%
fix_complex_query 12.7±0.12ms 12.7±0.22ms 0.00%
fix_superlong 179.6±8.15ms 199.1±10.17ms +10.86%
parse_complex_query 4.6±0.06µs 4.3±0.03µs -6.52%
parse_expression_recursion 7.8±0.17µs 7.6±0.06µs -2.56%
parse_simple_query 1089.4±25.20ns 1063.2±24.59ns -2.40%

@nikolaevx

Copy link
Copy Markdown
Collaborator Author

@codspeedbot fix this regression

@codspeed-hq

codspeed-hq Bot commented Jun 1, 2026

Copy link
Copy Markdown

@codspeedbot fix this regression

No real regression detected ✅

I investigated the +10.86% fix_superlong result from the wall-time benchmark and compared it against the CodSpeed simulation data.

CodSpeed simulation results (noise-free)

Benchmark main PR Change
fix_superlong 495.5 ms 498.8 ms +0.67% — Unchanged
fix_complex_query 36.2 ms 36.5 ms Unchanged
from_parent 166.1 µs 166.5 µs Unchanged

All 6 benchmarks are classified as unchanged by CodSpeed simulation.

Why the wall-time benchmark fluctuates

The wall-time numbers for fix_superlong tell the story:

  • First CI run (cf69afa): -6.83% — PR appeared faster
  • Second CI run (31fde4f): +10.86% — PR appeared slower

A ~17% swing between two runs on nearly identical code is a clear indicator of environmental noise (CPU throttling, scheduling jitter, memory pressure on shared CI runners). The ±8–10ms error bars on a ~190ms measurement already imply ~5% noise, so a 10% swing is within expected variance.

Conclusion

No code change is needed — the fix_superlong regression is wall-time noise, not a real performance change. The CodSpeed simulation (which isolates measurements from environmental factors) confirms all benchmarks are unchanged relative to main.

Summary
  • Warming up the performance wizard...
  • Investigating regression — Replying with analysis results
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

@nikolaevx

Copy link
Copy Markdown
Collaborator Author

@copilot FIX CI

Copilot AI commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

@copilot FIX CI

Fixed in 0ce5fdd. I investigated the failing PR Checks run, identified the Bazel Lint failure (clippy::doc_lazy_continuation in crates/lib/src/templaters.rs), and applied the minimal doc comment fix. Local validation: cargo clippy -p sqruff-lib -- -D warnings now passes.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Benchmark for 6758894

Click to view benchmark
Test Base PR %
DepthMap::from_parent 58.4±0.58µs 59.6±0.72µs +2.05%
fix_complex_query 11.9±0.10ms 12.0±0.03ms +0.84%
fix_superlong 155.1±5.78ms 164.4±6.58ms +6.00%
parse_complex_query 4.1±0.06µs 4.1±0.04µs 0.00%
parse_expression_recursion 6.9±0.05µs 6.9±0.11µs 0.00%
parse_simple_query 1046.2±19.50ns 1050.9±17.01ns +0.45%

@nikolaevx

Copy link
Copy Markdown
Collaborator Author

@copilot fix CI

Copilot AI commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

@copilot fix CI

Fixed the Bazel Lint (clippy) failure — collapsible_if in crates/lsp/src/lib.rs:440. Collapsed the nested if let into a single chained condition.

@nikolaevx

Copy link
Copy Markdown
Collaborator Author

@copilot make clippy happy

Copilot AI commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

@copilot make clippy happy

Fixed clippy::default_constructed_unit_structs in crates/cli-lib/src/reporters.rs. All crates now pass cargo clippy --all -- -D warnings.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Benchmark for 16724d2

Click to view benchmark
Test Base PR %
DepthMap::from_parent 62.4±1.13µs 62.1±8.72µs -0.48%
fix_complex_query 12.9±0.06ms 12.8±0.05ms -0.78%
fix_superlong 190.1±8.47ms 186.4±6.11ms -1.95%
parse_complex_query 4.3±0.32µs 4.3±0.10µs 0.00%
parse_expression_recursion 7.3±0.15µs 7.4±0.09µs +1.37%
parse_simple_query 1065.6±14.21ns 1064.9±13.46ns -0.07%

@nikolaevx

Copy link
Copy Markdown
Collaborator Author

@copilot fix CI

Copilot AI commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

@copilot fix CI

Fixed in the latest commit. Three issues resolved:

  1. cargo_machete: Removed unused dependencies (walkdir from lib, line-index from lib-wasm, ignore from lsp)
  2. cargo_test (ui_with_dbt): Restored the old == [filename] SKIP: reason format in OutputStreamFormatter::dispatch_file_skip, preserved relative paths in discover_paths output by stripping the working_dir prefix when the input path was relative, and updated the expected file count from 8→9
  3. All crates pass cargo clippy --all -- -D warnings and existing tests pass

nikolaevx added 28 commits July 8, 2026 08:48
@nikolaevx
nikolaevx force-pushed the config-early-behavior-fixes branch from 5ec8c77 to 08afe70 Compare July 8, 2026 15:50
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Benchmark for cf677fc

Click to view benchmark
Test Base PR %
DepthMap::from_parent 58.7±0.83µs 57.7±0.59µs -1.70%
fix_complex_query 12.2±0.10ms 12.0±0.14ms -1.64%
fix_superlong 142.9±12.50ms 135.7±13.14ms -5.04%
parse_complex_query 4.1±0.11µs 4.1±0.04µs 0.00%
parse_expression_recursion 7.0±0.05µs 7.1±0.09µs +1.43%
parse_simple_query 1054.0±14.40ns 1072.7±26.43ns +1.77%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants