Skip to content

Commit

Permalink
Allow using regal fix as a formatter
Browse files Browse the repository at this point in the history
Also:
- Removed the code that would send a response back for formatter
  options that `opa fmt` can't handle. This was a good idea, but since
  there isn't any obvious way for users to fix that, it hurts more than
  it helps
- Removed the fmt fix from the default fixer options as formatting is
  done anyway by the rego-v1 fixer

Fixes #839

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert committed Aug 5, 2024
1 parent b3a79c0 commit 9082db2
Show file tree
Hide file tree
Showing 8 changed files with 115 additions and 88 deletions.
5 changes: 5 additions & 0 deletions docs/fixing.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,8 @@ Currently, the following rules are automatically fixable:
Need to fix individual violations? Checkout the editors Regal supports
[here](/regal/editor-support).
:::
## Community
If you'd like to discuss Regal development or just talk about Regal in general, please join us in the `#regal`
channel in the Styra Community [Slack](https://communityinviter.com/apps/styracommunity/#)!
9 changes: 7 additions & 2 deletions docs/language-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,18 @@ user-defined functions too.

### Formatting

Regal uses the `opa fmt` formatter for formatting Rego. This is made available as a command in editors, but also via
a [code action](#code-actions) when unformatted files are encountered.
By default, Regal uses the `opa fmt` formatter for formatting Rego. This is made available as a command in editors,
but also via a [code action](#code-actions) when unformatted files are encountered.

<img
src={require('./assets/lsp/format.png').default}
alt="Screenshot of diagnostics as displayed in Zed"/>

Two other formatters are also available — `opa fmt --rego-v1` and `regal fix`. See the docs on
[Fixing Violations](fixing.md) for more information about the `fix` command. Which formatter to use
can be set via the `formatter` configuration option, which can be passed to Regal via the client (see
the documentation for your client for how to do that).

### Code completions

Code completions, or suggestions, is likely one of the most useful features of the Regal language server. And best of
Expand Down
28 changes: 0 additions & 28 deletions internal/lsp/format.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,31 +60,3 @@ func ComputeEdits(before, after string) []types.TextEdit {

return edits
}

// The opa fmt command does not allow configuration options, so we will have
// to ignore them if provided by the client. We can however log the warnings
// so that the caller has some chance to be made aware of why their options
// aren't applied.
func validateFormattingOptions(opts types.FormattingOptions) []string {
warnings := make([]string, 0)

if opts.InsertSpaces {
warnings = append(warnings, "opa fmt: only tabs supported for indentation")
}

if !opts.TrimTrailingWhitespace {
warnings = append(warnings, "opa fmt: trailing whitespace always trimmed")
}

if !opts.InsertFinalNewline {
warnings = append(warnings, "opa fmt: final newline always inserted")
}

if !opts.TrimFinalNewlines {
warnings = append(warnings, "opa fmt: final newlines always trimmed")
}

// opts.TabSize ignored as we don't support using spaces

return warnings
}
114 changes: 77 additions & 37 deletions internal/lsp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ type LanguageServer struct {
workspaceRootURI string
clientIdentifier clients.Identifier

clientInitializationOptions types.InitializationOptions

cache *cache.Cache
regoStore storage.Store

Expand Down Expand Up @@ -352,7 +354,7 @@ func (l *LanguageServer) StartConfigWorker(ctx context.Context) {
}
}

// when a file is 'unignored', we move it's contents to the
// when a file is 'unignored', we move its contents to the
// standard file list if missing
for k, v := range l.cache.GetAllIgnoredFiles() {
if l.ignoreURI(k) {
Expand All @@ -364,7 +366,7 @@ func (l *LanguageServer) StartConfigWorker(ctx context.Context) {
if !ok {
l.cache.SetFileContents(k, v)

// updating the parse here will enable things like go-to defn
// updating the parse here will enable things like go-to definition
// to start working right away without the need for a file content
// update to run updateParse.
_, err = updateParse(ctx, l.cache, l.regoStore, k)
Expand Down Expand Up @@ -529,9 +531,9 @@ func (l *LanguageServer) StartWorkspaceStateWorker(ctx context.Context) {
continue
}

for _, uri := range changedOrNewURIs {
for _, cnURI := range changedOrNewURIs {
l.diagnosticRequestFile <- fileUpdateEvent{
URI: uri,
URI: cnURI,
Reason: "internal/workspaceStateWorker/changedOrNewFile",
}
}
Expand Down Expand Up @@ -1296,10 +1298,6 @@ func (l *LanguageServer) handleTextDocumentFormatting(
return nil, fmt.Errorf("failed to unmarshal params: %w", err)
}

if warnings := validateFormattingOptions(params.Options); len(warnings) > 0 {
l.logError(fmt.Errorf("formatting params validation warnings: %v", warnings))
}

var oldContent string

var ok bool
Expand All @@ -1315,43 +1313,81 @@ func (l *LanguageServer) handleTextDocumentFormatting(
return nil, fmt.Errorf("failed to get file contents for uri %q", params.TextDocument.URI)
}

// set up an in-memory file provider to pass to the fixer for this one file
memfp := fileprovider.NewInMemoryFileProvider(map[string][]byte{
params.TextDocument.URI: []byte(oldContent),
})
formatter := "opa-fmt"

input, err := memfp.ToInput()
if err != nil {
return nil, fmt.Errorf("failed to create fixer input: %w", err)
if l.clientInitializationOptions.Formatter != nil {
formatter = *l.clientInitializationOptions.Formatter
}

li := linter.NewLinter().
WithUserConfig(*l.loadedConfig).
WithInputModules(&input)
var newContent []byte

switch formatter {
case "opa-fmt", "opa-fmt-rego-v1":
opts := format.Opts{}
if formatter == "opa-fmt-rego-v1" {
opts.RegoVersion = ast.RegoV0CompatV1
}

f := &fixes.Fmt{OPAFmtOpts: opts}
p := uri.ToPath(l.clientIdentifier, params.TextDocument.URI)

fixResults, err := f.Fix(&fixes.FixCandidate{Filename: filepath.Base(p), Contents: []byte(oldContent)}, nil)
if err != nil {
l.logError(fmt.Errorf("failed to format file: %w", err))

// return "null" as per the spec
return nil, nil
}

if len(fixResults) == 0 {
return []types.TextEdit{}, nil
}

newContent = fixResults[0].Contents
case "regal-fix":
// set up an in-memory file provider to pass to the fixer for this one file
memfp := fileprovider.NewInMemoryFileProvider(map[string][]byte{
params.TextDocument.URI: []byte(oldContent),
})

input, err := memfp.ToInput()
if err != nil {
return nil, fmt.Errorf("failed to create fixer input: %w", err)
}

f := fixer.NewFixer()
f.RegisterFixes(fixes.NewDefaultFixes()...)
f.RegisterMandatoryFixes(
&fixes.Fmt{
NameOverride: "use-rego-v1",
OPAFmtOpts: format.Opts{
RegoVersion: ast.RegoV0CompatV1,
f := fixer.NewFixer()
f.RegisterFixes(fixes.NewDefaultFixes()...)
f.RegisterMandatoryFixes(
&fixes.Fmt{
NameOverride: "use-rego-v1",
OPAFmtOpts: format.Opts{
RegoVersion: ast.RegoV0CompatV1,
},
},
},
)
)

fixReport, err := f.Fix(ctx, &li, memfp)
if err != nil {
return nil, fmt.Errorf("failed to format: %w", err)
}
li := linter.NewLinter().
WithInputModules(&input)

if fixReport.TotalFixes() == 0 {
return []types.TextEdit{}, nil
}
if l.loadedConfig != nil {
li = li.WithUserConfig(*l.loadedConfig)
}

newContent, err := memfp.GetFile(params.TextDocument.URI)
if err != nil {
return nil, fmt.Errorf("failed to get formatted contents: %w", err)
fixReport, err := f.Fix(ctx, &li, memfp)
if err != nil {
return nil, fmt.Errorf("failed to format: %w", err)
}

if fixReport.TotalFixes() == 0 {
return []types.TextEdit{}, nil
}

newContent, err = memfp.GetFile(params.TextDocument.URI)
if err != nil {
return nil, fmt.Errorf("failed to get formatted contents: %w", err)
}
default:
return nil, fmt.Errorf("unrecognized formatter %q", formatter)
}

return ComputeEdits(oldContent, string(newContent)), nil
Expand Down Expand Up @@ -1507,6 +1543,10 @@ func (l *LanguageServer) handleInitialize(
)
}

if params.InitializationOptions != nil {
l.clientInitializationOptions = *params.InitializationOptions
}

regoFilter := types.FileOperationFilter{
Scheme: "file",
Pattern: types.FileOperationPattern{
Expand Down
21 changes: 13 additions & 8 deletions internal/lsp/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,20 @@ type FileEvent struct {
URI string `json:"uri"`
}

type InitializationOptions struct {
Formatter *string `json:"formatter,omitempty"`
}

type InitializeParams struct {
ProcessID int `json:"processId"`
ClientInfo Client `json:"clientInfo"`
Locale string `json:"locale"`
RootPath string `json:"rootPath"`
RootURI string `json:"rootUri"`
Capabilities ClientCapabilities `json:"capabilities"`
Trace string `json:"trace"`
WorkspaceFolders []WorkspaceFolder `json:"workspaceFolders"`
ProcessID int `json:"processId"`
ClientInfo Client `json:"clientInfo"`
Locale string `json:"locale"`
RootPath string `json:"rootPath"`
RootURI string `json:"rootUri"`
Capabilities ClientCapabilities `json:"capabilities"`
Trace string `json:"trace"`
WorkspaceFolders []WorkspaceFolder `json:"workspaceFolders"`
InitializationOptions *InitializationOptions `json:"initializationOptions,omitempty"`
}

type WorkspaceFolder struct {
Expand Down
21 changes: 11 additions & 10 deletions pkg/fixer/fixer.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,15 @@ import (
"github.com/styrainc/regal/pkg/linter"
)

// NewFixer instantiates a Fixer.
func NewFixer() *Fixer {
return &Fixer{}
return &Fixer{
registeredFixes: make(map[string]any),
registeredMandatoryFixes: make(map[string]any),
}
}

// Fixer must be instantiated via NewFixer.
type Fixer struct {
registeredFixes map[string]any
registeredMandatoryFixes map[string]any
Expand All @@ -24,25 +29,21 @@ type Fixer struct {
// RegisterFixes sets the fixes that will be fixed if there are related linter
// violations that can be fixed by fixes.
func (f *Fixer) RegisterFixes(fixes ...fixes.Fix) {
if f.registeredFixes == nil {
f.registeredFixes = make(map[string]any)
}

for _, fix := range fixes {
f.registeredFixes[fix.Name()] = fix
if _, mandatory := f.registeredMandatoryFixes[fix.Name()]; !mandatory {
f.registeredFixes[fix.Name()] = fix
}
}
}

// RegisterMandatoryFixes sets fixes which will be run before other registered
// fixes, against all files which are not ignored, regardless of linter
// violations.
func (f *Fixer) RegisterMandatoryFixes(fixes ...fixes.Fix) {
if f.registeredMandatoryFixes == nil {
f.registeredMandatoryFixes = make(map[string]any)
}

for _, fix := range fixes {
f.registeredMandatoryFixes[fix.Name()] = fix

delete(f.registeredFixes, fix.Name())
}
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/fixer/fixer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ deny = true

expectedFileFixedViolations := map[string][]string{
// use-assignment-operator is not expected since use-rego-v1 also addresses this in this example
"main.rego": {"no-whitespace-comment", "opa-fmt", "use-rego-v1"},
"main.rego": {"no-whitespace-comment", "use-rego-v1"},
}
expectedFileContents := map[string][]byte{
"main.rego": []byte(`package test
Expand All @@ -64,7 +64,7 @@ deny := true
`),
}

if got, exp := fixReport.TotalFixes(), 3; got != exp {
if got, exp := fixReport.TotalFixes(), 2; got != exp {
t.Fatalf("expected %d fixed files, got %d", exp, got)
}

Expand Down
1 change: 0 additions & 1 deletion pkg/fixer/fixes/fixes.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
// When a new fix is added, it should be added to this list.
func NewDefaultFixes() []Fix {
return []Fix{
&Fmt{},
&Fmt{
NameOverride: "use-rego-v1",
OPAFmtOpts: format.Opts{
Expand Down

0 comments on commit 9082db2

Please # to comment.