linters(dart): remove dart shim, bump to 3.10.8#1114
linters(dart): remove dart shim, bump to 3.10.8#1114AndrewDongminYoo wants to merge 3 commits intotrunk-io:mainfrom
Conversation
|
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 I've submitted #1114 to address this and other edge cases that emerged from real-world usage: Key fixes:
Testing: I apologize for not catching these issues during the initial PR. |
TylerJang27
left a comment
There was a problem hiding this comment.
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:
- Revert the removal of the tool/shim for dart
- Enable dart in your
.trunk/trunk.yamlas justdart, without a version specified (if that doesn't work, trydart@SYSTEM, see docs - 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.
|
Thanks for the suggestion! I tested both approaches: Test 1: lint:
enabled:
- dartResult: ✗ Still uses Test 2: lint:
enabled:
- dart@SYSTEMResult: ✗ Same behavior - trunk attempts to use Observed behavior:
Root cause: architectural conflict between hermetic installs and Dart/Flutter's SDK model The 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 environment:
sdk: ^3.6.0When trunk downloads Dart 3.10.8 hermetically but the project targets Dart 3.6:
This is fundamentally different from tools like 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
Option B: Accept current PR approach The
Option C: Follow 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! |
|
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 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/dartFlutter's dart is used (matches project SDK version) This is exactly what we want. The key difference from PR #1098: Before (#1098):
After (#1114):
The Regarding I tested both If Bottom line: The current PR fixes the real-world issue (PATH shadowing in developer shells) while maintaining |
Summary
known_good_versionto Dart 3.10.8 and wirerun_fromforformat/fixcommands to prevent per-file directory traversal overheadfixto opt-in (enabled: false), keepanalyzeas the default diagnostic command aligning with the pattern used byruff,sqlfluff, andterraformtools.definitionsentirely and flattendownload/environmentintolint.definitions, eliminating thedartshim in.trunk/tools/to prevent it from shadowing the project-local Flutter/Dart SDK binary (same structure asclippyandgofmt)environment.PATHto["${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 fallbackChanges in detail
known_good_version3.9.2 → 3.10.8Bumps 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_present→config_presentAvoids auto-suggesting the dart linter in projects that have
.dartfiles but noanalysis_options.yaml.Reduces noise for projects that don't actively use the Dart analyzer.
run_fromonformatandfixdart formatanddart fixare 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 withanalysis_options.yaml), matching how the Dart toolchain expects to be invoked.fix→enabled: falseanalyzeis 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 likefixare opt-in.Reversing this would mean users who enable the dart linter receive no lint diagnostics at all.
Remove
tools.definitions/ shimWhen
tools.definitionsdeclaresshims: [dart], trunk exposes adartbinary under.trunk/tools/.If
.trunk/toolsis on PATH (as trunk adds it), this silently shadows the project's own dart binary (typically managed by Flutter).clippyandgofmtavoid this by placingdownloaddirectly insidelint.definitionswith no separate tool block.This PR applies the same pattern.
Prefer system dart on PATH
Previously
environment.PATHwas 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, andclippyin this repo.Test plan
dartlinter enables and runsanalyzeby defaultdart formatexecutes without per-file directory errorsdart fixdoes not run unless explicitly enabled (trunk check enable dart→ verify nofixaction in output)dartbinary appears under.trunk/tools/after enabling the linterdartbinary is not overridden on PATH