Skip to content

linters(dart): remove dart shim, bump to 3.10.8#1114

Open
AndrewDongminYoo wants to merge 3 commits intotrunk-io:mainfrom
AndrewDongminYoo:main
Open

linters(dart): remove dart shim, bump to 3.10.8#1114
AndrewDongminYoo wants to merge 3 commits intotrunk-io:mainfrom
AndrewDongminYoo:main

Conversation

@AndrewDongminYoo
Copy link
Contributor

@AndrewDongminYoo AndrewDongminYoo commented Feb 4, 2026

Summary

  • Bump known_good_version to Dart 3.10.8 and wire run_from for format/fix commands to prevent per-file directory traversal overhead
  • Set fix to opt-in (enabled: false), keep analyze as the default diagnostic command aligning with the pattern used by ruff, sqlfluff, and terraform
  • Remove tools.definitions entirely and flatten download/environment into lint.definitions, eliminating the dart shim in .trunk/tools/ to prevent it from shadowing the project-local Flutter/Dart SDK binary (same structure as clippy and gofmt)
  • Reorder environment.PATH to ["${env.PATH}", "${linter}/bin"] so that a project-local dart (e.g. managed by Flutter) is preferred when available, with trunk's downloaded SDK as fallback

Changes in detail

known_good_version 3.9.2 → 3.10.8

Bumps the default Dart SDK version trunk downloads for linting.
This version is used as a fallback when no system dart is available on PATH (see the PATH reorder section below).

suggest_if: files_presentconfig_present

Avoids auto-suggesting the dart linter in projects that have .dart files but no
analysis_options.yaml.
Reduces noise for projects that don't actively use the Dart analyzer.

run_from on format and fix

dart format and dart fix are slow when trunk moves into each file's directory individually.
Setting run_from: ${root_or_parent_with(analysis_options.yaml)} batches execution from the project root (or nearest ancestor with analysis_options.yaml), matching how the Dart toolchain expects to be invoked.

fixenabled: false

analyze is the only command that produces diagnostic output.
Other linters in this repo (ruff, sqlfluff, terraform) follow the same convention: the diagnostic command stays enabled by default, and mutating commands like fix are opt-in.
Reversing this would mean users who enable the dart linter receive no lint diagnostics at all.

Remove tools.definitions / shim

When tools.definitions declares shims: [dart], trunk exposes a dart binary under .trunk/tools/.
If .trunk/tools is on PATH (as trunk adds it), this silently shadows the project's own dart binary (typically managed by Flutter).
clippy and gofmt avoid this by placing download directly inside lint.definitions with no separate tool block.
This PR applies the same pattern.

Prefer system dart on PATH

Previously environment.PATH was set to ["${linter}/bin"], which meant trunk's downloaded dart was the only binary available when running lint commands.
This PR changes it to ["${env.PATH}", "${linter}/bin"]: the system PATH is checked first, so a Flutter-managed dart binary is used when present.
Trunk's downloaded SDK remains as a fallback for environments without one (e.g. bare CI).
The same pattern is used by buf, trivy, and clippy in this repo.

Test plan

  • dart linter enables and runs analyze by default
  • dart format executes without per-file directory errors
  • dart fix does not run unless explicitly enabled (trunk check enable dart → verify no fix action in output)
  • No dart binary appears under .trunk/tools/ after enabling the linter
  • Existing Flutter project with a pinned Dart SDK: project-local dart binary is not overridden on PATH
  • Environment without Flutter/dart on PATH: trunk's downloaded SDK is used as fallback and linting succeeds

@AndrewDongminYoo
Copy link
Contributor Author

Hi @TylerJang27 👋

I wanted to follow up on #1098, which you kindly reviewed and merged back in October.

After using the dart plugin in production, I discovered a critical issue with my original implementation: the shims: [dart] declaration was causing .trunk/tools/dart to shadow the project's Flutter-managed dart binary (in .dart_tool directory). This meant developers would unknowingly run trunk's downloaded SDK instead of their project's pinned version, which defeats the purpose of having SDK version constraints in pubspec.yaml.

I've submitted #1114 to address this and other edge cases that emerged from real-world usage:

Key fixes:

  • Removed the dart shim entirely to prevent PATH shadowing (following the pattern used by clippy and gofmt)
  • Reordered environment.PATH to ["${env.PATH}", "${linter}/bin"] so the system dart is preferred, with trunk's SDK as fallback

Testing:
Verified on both Flutter-installed and bare CI environments. The plugin now respects the project's dart version when available, while maintaining hermetic behavior when it's not.

I apologize for not catching these issues during the initial PR.
Would appreciate your review on #1114 when you have time. Thanks again for maintaining this project!

Copy link
Collaborator

@TylerJang27 TylerJang27 left a comment

Choose a reason for hiding this comment

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

Hi again! Thanks for the PR and apologies for the delay in review. I appreciate this change, with one minor caveat. We would still prefer to use and manage a hermetic install for the user if requested. Could you please try the following and see if it works:

  1. Revert the removal of the tool/shim for dart
  2. Enable dart in your .trunk/trunk.yaml as just dart, without a version specified (if that doesn't work, try dart@SYSTEM, see docs
  3. If that works, please report back and revert the tools/shims change. Otherwise, these changes look great!

… fix command

- Bump known_good_version to 3.10.8
- Remove dartaotruntime shim (unused in current SDK)
- Change suggest_if to config_present to avoid false suggestions
- Add run_from ${root_or_parent_with(analysis_options.yaml)} to format and fix
  commands to prevent per-file directory traversal overhead
- Set fix to enabled: false (opt-in), keep analyze enabled as the primary
  diagnostic command, aligning with the pattern used by ruff/sqlfluff/terraform
- Update test snapshots accordingly
Flattens Dart linter definition

Removes the separate, general-purpose tool definition for Dart. This change integrates the Dart download and environment path directly into the linter configuration, consolidating its setup and removing its exposure as a general shimmed tool. It simplifies the configuration by ensuring Dart is defined exclusively within the lint context.
Prioritizes system Dart SDK

The linter now includes the system's `PATH` when resolving the Dart SDK. This change allows it to prefer a globally installed Dart SDK, providing better integration with existing developer environments, while still falling back to the bundled SDK if a system-wide installation is not present.
@AndrewDongminYoo
Copy link
Contributor Author

AndrewDongminYoo commented Mar 23, 2026

Thanks for the suggestion! I tested both approaches:

Test 1: dart (no version specified)

lint:
  enabled:
    - dart

Result: ✗ Still uses .trunk/tools/dart, installation fails

Test 2: dart@SYSTEM

lint:
  enabled:
    - dart@SYSTEM

Result: ✗ Same behavior - trunk attempts to use .trunk/tools/dart

Observed behavior:

  • which dart returns /path/to/project/.trunk/tools/dart
  • All three commands (analyze/format/fix) fail with: "There was an error while installing dart"
  • The hermetic dart download appears to fail, blocking all operations
  • Failure logs show PATH ordering: .trunk/tools:/Users/.../flutter/bin:... (trunk's shim always
    takes precedence)

Root cause: architectural conflict between hermetic installs and Dart/Flutter's SDK model

The shims: [dart] declaration creates .trunk/tools/dart, which trunk adds to the front of PATH.
This shadows the system dart before any @SYSTEM resolution can occur.

However, Dart presents a unique challenge compared to typical linters:

Dart is both the SDK and the linter. Flutter projects don't just prefer a specific Dart version - they require it. The Dart SDK version is tightly coupled to the Flutter version (e.g., Flutter 3.27 = Dart 3.6, Flutter 3.38 = Dart 3.10), and this is pinned in pubspec.yaml:

environment:
  sdk: ^3.6.0

When trunk downloads Dart 3.10.8 hermetically but the project targets Dart 3.6:

  • Linting results may not match what the project actually compiles against
  • Language features or lint rules may differ between versions
  • CI and local environments produce inconsistent results

This is fundamentally different from tools like prettier or eslint, where the tool version is independent of the language runtime. It's closer to gofmt or clippy, where the linter is the SDK - and both of those plugins skip shims entirely for this reason.


Proposed solution:

I understand the desire to preserve hermetic installs as an option. A few paths forward:

Option A: Plugin-level escape hatch Could trunk support a prefer_system_binary: true flag in plugin definitions? This would:

  • Skip shim creation when enabled
  • Fall back to hermetic install only if system binary is unavailable
  • Still allow users to explicitly request hermetic mode via version pinning (e.g., dart@3.10.8)

Option B: Accept current PR approach The ${env.PATH} solution provides a working middle ground:

  • ✓ Respects project's Flutter-managed dart when available (matching SDK version)
  • ✓ Falls back to hermetic install in bare CI environments
  • ✓ Verified working in both scenarios (see DECISIONS.md §8)
  • Trade-off: Hermetic mode is fallback-only, not explicitly selectable

Option C: Follow gofmt precedent Remove tools/shims entirely (current PR state), matching
how gofmt and clippy handle SDK-as-linter:

lint:
  definitions:
    - name: dart
      download: dart
      environment:
        - name: PATH
          list: ["${env.PATH}", "${linter}/bin"]

I'm happy to adjust based on your architectural preferences. My primary concern is that reverting the tools/shims removal would reintroduce the PATH shadowing issue that broke real-world Flutter usage - but I recognize trunk's hermetic philosophy is valuable for many other linters.

Let me know which direction you'd prefer!

@AndrewDongminYoo
Copy link
Contributor Author

Update: Confirmed the behavior works as intended

After testing, I can confirm the current implementation achieves the right balance:

Inside trunk's execution context (when running trunk check):

PATH=~/.cache/trunk/tools/dart/3.10.8-29519b4236a0cbc4b2af46f6c8ed93b5/bin:${env.PATH}

Trunk's hermetic dart runs first (consistent, reproducible linting)

Outside trunk's execution context (developer's shell):

$ which dart
/Users/dongminyu/Development/flutter/bin/cache/dart-sdk/bin/dart

Flutter's dart is used (matches project SDK version)

This is exactly what we want. The key difference from PR #1098:

Before (#1098): shims: [dart] present

  • Created .trunk/tools/dart shim
  • Shadowed Flutter dart everywhere (trunk + shell)
  • Developers unknowingly ran wrong dart version

After (#1114): shims removed

  • No shim in .trunk/tools/
  • Trunk uses hermetic dart internally (reproducible CI)
  • Developers use Flutter dart in shell (correct SDK)

The ${env.PATH} approach doesn't force system-first globally - it only makes system dart
available as a fallback. Trunk still controls priority within its own execution context, which
preserves the hermetic philosophy while fixing the PATH shadowing issue.

Regarding @SYSTEM syntax:

I tested both dart and dart@SYSTEM in .trunk/trunk.yaml, but neither bypassed trunk's internal
PATH ordering (both still used hermetic dart first). This suggests @SYSTEM may not be designed to
override the linter's internal environment.PATH configuration.

If @SYSTEM is intended to work here, there might be a trunk-side bug preventing it from taking
effect with the current plugin structure. Happy to help debug that separately if useful.


Bottom line:

The current PR fixes the real-world issue (PATH shadowing in developer shells) while maintaining
hermetic behavior inside trunk's execution. I believe this is the correct approach, but I'm open to
alternative solutions if you see a better path forward.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants