Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix ordered map race #1701

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion args/args.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@ package args
import (
"strings"

"github.com/go-task/task/v3/internal/omap"
"github.com/go-task/task/v3/taskfile/ast"
)

// Parse parses command line argument: tasks and global variables
func Parse(args ...string) ([]*ast.Call, *ast.Vars) {
calls := []*ast.Call{}
globals := &ast.Vars{}
globals := &ast.Vars{
OrderedMap: omap.New[string, ast.Var](),
}

for _, arg := range args {
if !strings.Contains(arg, "=") {
Expand Down
13 changes: 12 additions & 1 deletion cmd/task/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/go-task/task/v3/internal/experiments"
"github.com/go-task/task/v3/internal/flags"
"github.com/go-task/task/v3/internal/logger"
"github.com/go-task/task/v3/internal/omap"
"github.com/go-task/task/v3/internal/sort"
ver "github.com/go-task/task/v3/internal/version"
"github.com/go-task/task/v3/taskfile/ast"
Expand Down Expand Up @@ -175,7 +176,17 @@ func run() error {

// If there are no calls, run the default task instead
if len(calls) == 0 {
calls = append(calls, &ast.Call{Task: "default"})
calls = append(calls, &ast.Call{Task: "default", Vars: ast.NewVars()})
} else {
for _, call := range calls {
if call.Vars == nil {
call.Vars = ast.NewVars()
}
}
}

if globals.OrderedMap == nil {
globals.OrderedMap = omap.New[string, ast.Var]()
}

globals.Set("CLI_ARGS", ast.Var{Value: cliArgs})
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
// GetEnviron the all return all environment variables encapsulated on a
// ast.Vars
func GetEnviron() *ast.Vars {
m := &ast.Vars{}
m := ast.NewVars()
for _, e := range os.Environ() {
keyVal := strings.SplitN(e, "=", 2)
key, val := keyVal[0], keyVal[1]
Expand Down
125 changes: 89 additions & 36 deletions internal/omap/orderedmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,43 +4,49 @@ import (
"cmp"
"fmt"
"slices"
"sync"

"gopkg.in/yaml.v3"

"github.com/go-task/task/v3/internal/deepcopy"
"github.com/go-task/task/v3/internal/exp"
)

// An OrderedMap is a wrapper around a regular map that maintains an ordered
// list of the map's keys. This allows you to run deterministic and ordered
// operations on the map such as printing/serializing/iterating.
type OrderedMap[K cmp.Ordered, V any] struct {
s []K
m map[K]V
type OrderedMap[K cmp.Ordered, V any] interface {
Get(key K) V
Set(key K, value V)
Len() int
Exists(key K) bool
Range(fn func(key K, value V) error) error
Values() []V
Keys() []K
Sort()
SortFunc(less func(i, j K) int)
DeepCopy() OrderedMap[K, V]
Merge(om OrderedMap[K, V])
yaml.Unmarshaler
}

// New will create a new OrderedMap of the given type and return it.
func New[K cmp.Ordered, V any]() OrderedMap[K, V] {
return OrderedMap[K, V]{
s: make([]K, 0),
m: make(map[K]V),
}
return newOrderedMap[K, V]()
}

// FromMap will create a new OrderedMap from the given map. Since Golang maps
// are unordered, the order of the created OrderedMap will be random.
func FromMap[K cmp.Ordered, V any](m map[K]V) OrderedMap[K, V] {
om := New[K, V]()
om.m = m
om.s = exp.Keys(m)
om := newOrderedMap[K, V]()
for k, v := range m {
om.Set(k, v)
}
return om
}

func FromMapWithOrder[K cmp.Ordered, V any](m map[K]V, order []K) OrderedMap[K, V] {
om := New[K, V]()
if len(m) != len(order) {
panic("length of map and order must be equal")
}

om := newOrderedMap[K, V]()
om.m = m
om.s = order
for key := range om.m {
Expand All @@ -51,26 +57,47 @@ func FromMapWithOrder[K cmp.Ordered, V any](m map[K]V, order []K) OrderedMap[K,
return om
}

// An OrderedMap is a wrapper around a regular map that maintains an ordered
// list of the map's keys. This allows you to run deterministic and ordered
// operations on the map such as printing/serializing/iterating.
type orderedMap[K cmp.Ordered, V any] struct {
mutex sync.RWMutex
s []K
m map[K]V
}

func newOrderedMap[K cmp.Ordered, V any]() *orderedMap[K, V] {
return &orderedMap[K, V]{
s: make([]K, 0),
m: make(map[K]V),
}
}

// Len will return the number of items in the map.
func (om *OrderedMap[K, V]) Len() int {
return len(om.s)
func (om *orderedMap[K, V]) Len() (l int) {
om.mutex.RLock()
l = len(om.s)
om.mutex.RUnlock()

return
}

// Set will set the value for a given key.
func (om *OrderedMap[K, V]) Set(key K, value V) {
if om.m == nil {
om.m = make(map[K]V)
}
func (om *orderedMap[K, V]) Set(key K, value V) {
om.mutex.Lock()
if _, ok := om.m[key]; !ok {
om.s = append(om.s, key)
}
om.m[key] = value
om.mutex.Unlock()
}

// Get will return the value for a given key.
// If the key does not exist, it will return the zero value of the value type.
func (om *OrderedMap[K, V]) Get(key K) V {
func (om *orderedMap[K, V]) Get(key K) V {
om.mutex.RLock()
value, ok := om.m[key]
om.mutex.RUnlock()
if !ok {
var zero V
return zero
Expand All @@ -79,62 +106,88 @@ func (om *OrderedMap[K, V]) Get(key K) V {
}

// Exists will return whether or not the given key exists.
func (om *OrderedMap[K, V]) Exists(key K) bool {
func (om *orderedMap[K, V]) Exists(key K) bool {
om.mutex.RLock()
_, ok := om.m[key]
om.mutex.RUnlock()
return ok
}

// Sort will sort the map.
func (om *OrderedMap[K, V]) Sort() {
func (om *orderedMap[K, V]) Sort() {
om.mutex.Lock()
slices.Sort(om.s)
om.mutex.Unlock()
}

// SortFunc will sort the map using the given function.
func (om *OrderedMap[K, V]) SortFunc(less func(i, j K) int) {
func (om *orderedMap[K, V]) SortFunc(less func(i, j K) int) {
om.mutex.Lock()
slices.SortFunc(om.s, less)
om.mutex.Unlock()
}

// Keys will return a slice of the map's keys in order.
func (om *OrderedMap[K, V]) Keys() []K {
return om.s
func (om *orderedMap[K, V]) Keys() []K {
om.mutex.RLock()
keys := deepcopy.Slice(om.s)
om.mutex.RUnlock()

return keys
}

// Values will return a slice of the map's values in order.
func (om *OrderedMap[K, V]) Values() []V {
func (om *orderedMap[K, V]) Values() []V {
var values []V

om.mutex.RLock()
for _, key := range om.s {
values = append(values, om.m[key])
values = append(values, om.Get(key))
trim21 marked this conversation as resolved.
Show resolved Hide resolved
}
om.mutex.RUnlock()

return values
}

// Range will iterate over the map and call the given function for each key/value.
func (om *OrderedMap[K, V]) Range(fn func(key K, value V) error) error {
for _, key := range om.s {
if err := fn(key, om.m[key]); err != nil {
func (om *orderedMap[K, V]) Range(fn func(key K, value V) error) error {
keys := om.Keys()
for _, key := range keys {
if err := fn(key, om.Get(key)); err != nil {
return err
}
}
return nil
}

// Merge merges the given Vars into the caller one
func (om *OrderedMap[K, V]) Merge(other OrderedMap[K, V]) {
func (om *orderedMap[K, V]) Merge(other OrderedMap[K, V]) {
// nolint: errcheck
other.Range(func(key K, value V) error {
om.Set(key, value)
return nil
})
}

func (om *OrderedMap[K, V]) DeepCopy() OrderedMap[K, V] {
return OrderedMap[K, V]{
func (om *orderedMap[K, V]) DeepCopy() OrderedMap[K, V] {
om.mutex.RLock()
o := orderedMap[K, V]{
s: deepcopy.Slice(om.s),
m: deepcopy.Map(om.m),
}
om.mutex.RUnlock()

return &o
}

func (om *OrderedMap[K, V]) UnmarshalYAML(node *yaml.Node) error {
func (om *orderedMap[K, V]) UnmarshalYAML(node *yaml.Node) error {
if om == nil {
*om = orderedMap[K, V]{
m: make(map[K]V),
s: make([]K, 0),
}
}

switch node.Kind {
// Even numbers contain the keys
// Odd numbers contain the values
Expand Down
16 changes: 8 additions & 8 deletions internal/omap/orderedmap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (

func TestFromMap(t *testing.T) {
m := map[int]string{3: "three", 1: "one", 2: "two"}
om := FromMap(m)
om := FromMap(m).(*orderedMap[int, string])
assert.Len(t, om.m, 3)
assert.Len(t, om.s, 3)
assert.ElementsMatch(t, []int{1, 2, 3}, om.s)
Expand All @@ -29,7 +29,7 @@ func TestSetGetExists(t *testing.T) {
}

func TestSort(t *testing.T) {
om := New[int, string]()
om := New[int, string]().(*orderedMap[int, string])
om.Set(3, "three")
om.Set(1, "one")
om.Set(2, "two")
Expand All @@ -38,7 +38,7 @@ func TestSort(t *testing.T) {
}

func TestSortFunc(t *testing.T) {
om := New[int, string]()
om := New[int, string]().(*orderedMap[int, string])
om.Set(3, "three")
om.Set(1, "one")
om.Set(2, "two")
Expand All @@ -49,7 +49,7 @@ func TestSortFunc(t *testing.T) {
}

func TestKeysValues(t *testing.T) {
om := New[int, string]()
om := New[int, string]().(*orderedMap[int, string])
om.Set(3, "three")
om.Set(1, "one")
om.Set(2, "two")
Expand All @@ -58,7 +58,7 @@ func TestKeysValues(t *testing.T) {
}

func Range(t *testing.T) {
om := New[int, string]()
om := New[int, string]().(*orderedMap[int, string])
om.Set(3, "three")
om.Set(1, "one")
om.Set(2, "two")
Expand All @@ -81,7 +81,7 @@ func Range(t *testing.T) {
}

func TestOrderedMapMerge(t *testing.T) {
om1 := New[string, int]()
om1 := New[string, int]().(*orderedMap[string, int])
om1.Set("a", 1)
om1.Set("b", 2)

Expand Down Expand Up @@ -109,8 +109,8 @@ func TestUnmarshalYAML(t *testing.T) {
1: one
2: two
`
var om OrderedMap[int, string]
err := yaml.Unmarshal([]byte(yamlString), &om)
om := New[int, string]()
err := yaml.Unmarshal([]byte(yamlString), om)
trim21 marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(t, err)

expectedKeys := []int{3, 1, 2}
Expand Down
3 changes: 2 additions & 1 deletion internal/summary/summary_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/stretchr/testify/assert"

"github.com/go-task/task/v3/internal/logger"
"github.com/go-task/task/v3/internal/omap"
"github.com/go-task/task/v3/internal/summary"
"github.com/go-task/task/v3/taskfile/ast"
)
Expand Down Expand Up @@ -156,7 +157,7 @@ func TestPrintAllWithSpaces(t *testing.T) {
t2 := &ast.Task{Task: "t2"}
t3 := &ast.Task{Task: "t3"}

tasks := ast.Tasks{}
tasks := ast.Tasks{OrderedMap: omap.New[string, *ast.Task]()}
tasks.Set("t1", t1)
tasks.Set("t2", t2)
tasks.Set("t3", t3)
Expand Down
4 changes: 2 additions & 2 deletions internal/templater/templater.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,11 @@ func ReplaceVarsWithExtra(vars *ast.Vars, cache *Cache, extra map[string]any) *a
return nil
}

var newVars ast.Vars
newVars := ast.NewVars()
_ = vars.Range(func(k string, v ast.Var) error {
newVars.Set(k, ReplaceVarWithExtra(v, cache, extra))
return nil
})

return &newVars
return newVars
}
2 changes: 1 addition & 1 deletion task.go
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ func (e *Executor) GetTask(call *ast.Call) (*ast.Task, error) {
case 0: // Carry on
case 1:
if call.Vars == nil {
call.Vars = &ast.Vars{}
call.Vars = ast.NewVars()
}
call.Vars.Set("MATCH", ast.Var{Value: matchingTasks[0].Wildcards})
return matchingTasks[0].Task, nil
Expand Down
Loading
Loading