feat(Buildifier): Allow enabling /disabling buildifier functionality #629
Open
cbandera wants to merge 8 commits into
Open
feat(Buildifier): Allow enabling /disabling buildifier functionality #629cbandera wants to merge 8 commits into
cbandera wants to merge 8 commits into
Conversation
cbandera
requested review from
cameron-martin,
coeuvre,
jfirebaugh and
vogelsgesang
as code owners
- Add BuildifierFeature class extending BaseExtensionFeature - Add bazel.enableBuildifier configuration setting (default: true) - Move buildifier diagnostics and formatting provider registration into feature - Extract showBuildifierDownloadPrompt for async user notifications - Remove duplicate buildifier availability checks from extension.ts - Maintain backward compatibility with existing buildifier functionality This change encapsulates buildifier support as a toggleable feature following the same patterns as CodeLensFeature and WorkspaceTreeFeature.
This has multiple positive effects: - buildifier can be used on any Starlark file even if not bazel workspace is present - buildifier can be used on standalone files outside your current workspace - buildifier will work robustly for multi-workspace setups Closes bazel-contrib#620
cbandera
force-pushed
the
feature/490-buildifier-feature
branch
from
6d823ef to
3fd9ddf
Compare
cbandera
commented
May 11, 2026
cbandera
changed the title
cameron-martin
requested changes
May 17, 2026
|
|
||
| enable(context: vscode.ExtensionContext): boolean { | ||
| // Precondition: buildifier executable available | ||
| if (!checkBuildifierIsAvailable()) { |
Collaborator
There was a problem hiding this comment.
The checkBuildifierIsAvailable function is async so this condition will always evaluate to false (since a promise is truthy). Is it viable for this function to be async?
This would have been caught if we had the no-misused-promises eslint option enabled. I'll put up a PR to enable this.
Collaborator
Author
There was a problem hiding this comment.
Thanks. I will need to think about the best design for this a little longer
Collaborator
Author
There was a problem hiding this comment.
Made the enable() function async, which however also made it necessary to avoid race conditions with multiple config changes while a enable call is still running.
cameron-martin
added a commit
that referenced
this pull request
May 18, 2026
This rule would have caught an issue with #629 if it were enabled, particularly the `checksConditionals` part. A few modifications had to be made to accommodate the `checksVoidReturn` part of the rule.
checkBuildifierIsAvailable() is async but was called without await, so the truthy Promise always passed the guard — buildifier was never actually checked. Fix by making enable() return Promise<boolean> across the base class and all subclasses, and propagating async through onConfigurationChanged and the create() factory.
…andling
- Serialize onConfigurationChanged calls via a pending-promise chain to
prevent concurrent enable() invocations and lost disable-while-enabling
- Catch exceptions thrown by enable() in doConfigurationChange so OS-level
errors (EACCES, spawn failure) are logged instead of silently swallowed
- Wrap executeBuildifier in try/catch in checkBuildifierIsAvailable to
prevent process errors from propagating uncaught to the void call site
- Thread the validated buildifierPath through to BuildifierDiagnosticsManager
and BuildifierFormatProvider to eliminate the TOCTOU gap between the
availability check and runtime path resolution
- Remove misleading async keyword from sync-only enable() in CodeLensFeature
and WorkspaceTreeFeature; use explicit Promise.resolve() instead
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change encapsulates buildifier support as a toggleable feature following the same patterns as CodeLensFeature and WorkspaceTreeFeature.
Related Issue
Related to #490
Closes #193
Related to #402
Closes #449
Closes #620
Testing
Checklist
npm run test)