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.

Sign up for GitHub

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

Jump to bottom

Apply map_kind to existing rules as well as new ones #1425

Merged
merged 7 commits into from
Feb 10, 2023
Merged

Apply map_kind to existing rules as well as new ones #1425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
594e394
Add tests for a bunch of map_kind edge cases
illicitonion Dec 9, 2022
0b1f447
Apply map_kind to existing rules as well as new ones
illicitonion Jan 24, 2023
8dee9e5
Handle transitive map_kind directives
illicitonion Jan 24, 2023
49697e5
Simplify function
illicitonion Jan 25, 2023
94ee030
Update cmd/gazelle/fix-update.go
illicitonion Jan 31, 2023
8241316
Update cmd/gazelle/fix-update.go
illicitonion Jan 31, 2023
c8c2e77
Tidy up error message to be less repetative
illicitonion Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
64 changes: 60 additions & 4 deletions cmd/gazelle/fix-update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ func runFixUpdate(wd string, cmd command, args []string) (err error) {
// Visit all directories in the repository.
var visits []visitRecord
uc := getUpdateConfig(c)
var errorsFromWalk []error
walk.Walk(c, cexts, uc.dirs, uc.walkMode, func(dir, rel string, c *config.Config, update bool, f *rule.File, subdirs, regularFiles, genFiles []string) {
// If this file is ignored or if Gazelle was not asked to update this
// directory, just index the build file and move on.
Expand Down Expand Up @@ -322,11 +323,22 @@ func runFixUpdate(wd string, cmd command, args []string) (err error) {
mappedKinds []config.MappedKind
mappedKindInfo = make(map[string]rule.KindInfo)
)
for _, r := range gen {
if repl, ok := c.KindMap[r.Kind()]; ok {
// We apply map_kind to all rules, including pre-existing ones.
var allRules []*rule.Rule
allRules = append(allRules, gen...)
linzhp marked this conversation as resolved.
Show resolved Hide resolved
if f != nil {
allRules = append(allRules, f.Rules...)
}
Comment on lines +327 to +331
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: does the append ordering matter here?

I.e. if in foo/BUILD I were to map a -> b and in foo/bar/BUILD I were to map b -> c, what would be generated in foo/bar/tee/BUILD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting question! I think ordering of rules doesn't matter here, but there is an interesting edge-case nonetheless:

For each rule, we look up in a pre-generated config for the map_kind directive, which was generated in the ordinary way for config. So the config object for foo/bar/tee/BUILD will contain: a -> b and b -> c but doesn't resolve a transitive a -> c. Given this is a static pre-generated config, and the mappings aren't affected by the order the rules are consulted, I don't think there's an ordering problem here.

This means for things we generate, if we generate an a we'll end up with a b, and if we generate a b we'll end up with a c.

For things which already existed, if there was an a we'll end up with a b, and if there was a b we'll end up with a c. Which means that if you have an a, and run gazelle twice, you'll end up with different results after each generation.

I think if we wanted to fix this, we'd probably add a loop around the if repl, ok := c.KindMap[r.Kind()]; ok { to keep mapping until we reach an unmapped rule, or until we detect a loop.

I've pushed 0c06eb2 which does this transitive mapping, and which errors if a loop was encountered. I can imagine situations where people may have loops in their map_kind directives today, so this may be a breaking change to some.

for _, r := range allRules {
repl, err := lookupMapKindReplacement(c.KindMap, r.Kind())
if err != nil {
errorsFromWalk = append(errorsFromWalk, fmt.Errorf("looking up mapped kind: %w", err))
continue
}
if repl != nil {
mappedKindInfo[repl.KindName] = kinds[r.Kind()]
mappedKinds = append(mappedKinds, repl)
mrslv.MappedKind(rel, repl)
mappedKinds = append(mappedKinds, *repl)
mrslv.MappedKind(rel, *repl)
r.SetKind(repl.KindName)
}
}
Expand Down Expand Up @@ -366,6 +378,19 @@ func runFixUpdate(wd string, cmd command, args []string) (err error) {
}
}

if len(errorsFromWalk) == 1 {
return errorsFromWalk[0]
}

if len(errorsFromWalk) > 1 {
var additionalErrors []string
for _, error := range errorsFromWalk[1:] {
additionalErrors = append(additionalErrors, error.Error())
}

return fmt.Errorf("encountered multiple errors: %w, %v", errorsFromWalk[0], strings.Join(additionalErrors, ", "))
}

// Finish building the index for dependency resolution.
ruleIndex.Finish()

Expand Down Expand Up @@ -411,6 +436,37 @@ func runFixUpdate(wd string, cmd command, args []string) (err error) {
return exit
}

// lookupMapKindReplacement finds a mapped replacement for rule kind `kind`, resolving transitively.
// i.e. if go_library is mapped to custom_go_library, and custom_go_library is mapped to other_go_library,
// looking up go_library will return other_go_library.
// It returns an error on a loop, and may return nil if no remapping should be performed.
func lookupMapKindReplacement(kindMap map[string]config.MappedKind, kind string) (*config.MappedKind, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func lookupMapKindReplacement(kindMap map[string]config.MappedKind, kind string) (*config.MappedKind, error) {
func lookupMapKindReplacement(kindMap map[string]config.MappedKind, kind string) (string, error) {

To simplify the interface, you can return the replacement kind here, because the caller of this function can get the result of info from config.MappedKind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure on this one - having a fully typed *config.MappedKind as a return value feels better documenting as to what the function is doing, and the caller is going to look up the output in c.KindMap again anyway, so it feels like returning a *config.MappedKind avoids potential errors such as a different KindMap being passed to the function and being used by the caller?

var mapped *config.MappedKind
seenKinds := make(map[string]struct{})
seenKindPath := []string{kind}
for {
replacement, ok := kindMap[kind]
if !ok {
break
}

seenKindPath = append(seenKindPath, replacement.KindName)
if _, alreadySeen := seenKinds[replacement.KindName]; alreadySeen {
return nil, fmt.Errorf("found loop of map_kind replacements: %s", strings.Join(seenKindPath, " -> "))
}

seenKinds[replacement.KindName] = struct{}{}
mapped = &replacement
if kind == replacement.KindName {
break
}

kind = replacement.KindName
}

return mapped, nil
}

func newFixUpdateConfiguration(wd string, cmd command, args []string, cexts []config.Configurer) (*config.Config, error) {
c := config.New()
c.WorkDir = wd
Expand Down