-
Notifications
You must be signed in to change notification settings - Fork 4k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Modify the error message that occurs when a requested target does not…
… exist. First, we removed the suggestion to "use `query`" as it was misleading to some users. Second, we fine-tuned the suggested target behaviors. The existing method retrieves only the closest target within a reasonable distance. We updated this to return potentially more targets, with a preference for those that are particularly close. Performance-wise, this should be not diverge much from the status quo. The existing behavior already checks edit-distance for each target in the package against the requested target. This adds minimal behavior on top of that to identify the most likely candidate(s). RELNOTES: None. PiperOrigin-RevId: 591302621 Change-Id: I9bd272b68def2f9a75db01061287697d23936bca
- Loading branch information
1 parent
cad0f29
commit ef98ef9
Showing
14 changed files
with
300 additions
and
78 deletions.
There are no files selected for viewing
This file contains 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
This file contains 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
136 changes: 136 additions & 0 deletions
136
src/main/java/com/google/devtools/build/lib/packages/TargetSuggester.java
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| // Copyright 2023 The Bazel Authors. All rights reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| package com.google.devtools.build.lib.packages; | ||
|
|
||
| import com.google.common.annotations.VisibleForTesting; | ||
| import com.google.common.base.Joiner; | ||
| import com.google.common.collect.ImmutableList; | ||
| import com.google.common.collect.ImmutableListMultimap; | ||
| import com.google.common.collect.Iterables; | ||
| import java.util.Locale; | ||
| import java.util.Set; | ||
| import net.starlark.java.spelling.SpellChecker; | ||
|
|
||
| /** | ||
| * Some static utility functions for determining suggested targets when a user requests a | ||
| * non-existent target. | ||
| */ | ||
| public final class TargetSuggester { | ||
| private static final int MAX_SUGGESTED_TARGETS_SIZE = 10; | ||
|
|
||
| private static final int MAX_SUGGESTION_EDIT_DISTANCE = 5; | ||
|
|
||
| private TargetSuggester() {} | ||
|
|
||
| /** | ||
| * Given a nonexistent target and the targets in its package, suggest what the user may have | ||
| * intended based on lexicographic closeness to the possibilities. | ||
| * | ||
| * <p>This will be pretty printed in the following form No suggested targets -> "". Suggested | ||
| * target "a" -> "a". Suggested targets "a", "b", "c" -> "a, b, or c". | ||
| */ | ||
| static String suggestTargets(String input, Set<String> words) { | ||
| ImmutableList<String> suggestedTargets = suggestedTargets(input, words); | ||
| return prettyPrintTargets(suggestedTargets); | ||
| } | ||
|
|
||
| /** | ||
| * Given a requested target and a Set of targets in the same package, return a list of strings | ||
| * closest based on edit distance. | ||
| * | ||
| * <p>If any strings are identical minus capitalization changes, they will be returned. If any | ||
| * other strings are exactly 1 character off, they will be returned. Otherwise, the 10 nearest | ||
| * (within a small edit distance) will be returned. | ||
| */ | ||
| @VisibleForTesting | ||
| static ImmutableList<String> suggestedTargets(String input, Set<String> words) { | ||
|
|
||
| final String lowerCaseInput = input.toLowerCase(Locale.US); | ||
|
|
||
| // Add words based on edit distance | ||
| ImmutableListMultimap.Builder<Integer, String> editDistancesBuilder = | ||
| ImmutableListMultimap.builder(); | ||
|
|
||
| int maxEditDistance = Math.min(MAX_SUGGESTION_EDIT_DISTANCE, (input.length() + 1) / 2); | ||
| for (String word : words) { | ||
| String lowerCaseWord = word.toLowerCase(Locale.US); | ||
|
|
||
| int editDistance = SpellChecker.editDistance(lowerCaseInput, lowerCaseWord, maxEditDistance); | ||
|
|
||
| if (editDistance >= 0) { | ||
| editDistancesBuilder.put(editDistance, word); | ||
| } | ||
| } | ||
| ImmutableListMultimap<Integer, String> editDistanceToWords = editDistancesBuilder.build(); | ||
|
|
||
| ImmutableList<String> zeroEditDistanceWords = editDistanceToWords.get(0); | ||
| ImmutableList<String> oneEditDistanceWords = editDistanceToWords.get(1); | ||
|
|
||
| if (editDistanceToWords.isEmpty()) { | ||
| return ImmutableList.of(); | ||
| } else if (!zeroEditDistanceWords.isEmpty()) { | ||
| int sublistLength = Math.min(zeroEditDistanceWords.size(), MAX_SUGGESTED_TARGETS_SIZE); | ||
| return ImmutableList.copyOf(zeroEditDistanceWords.subList(0, sublistLength)); | ||
| } else if (!oneEditDistanceWords.isEmpty()) { | ||
| int sublistLength = Math.min(oneEditDistanceWords.size(), MAX_SUGGESTED_TARGETS_SIZE); | ||
| return ImmutableList.copyOf(oneEditDistanceWords.subList(0, sublistLength)); | ||
| } else { | ||
| return getSuggestedTargets(editDistanceToWords, maxEditDistance); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Given a map of edit distance values to words that are that distance from the requested target, | ||
| * returns up to MAX_SUGGESTED_TARGETS_SIZE targets that are at least edit distance 2 but no more | ||
| * than the given max away. | ||
| */ | ||
| private static ImmutableList<String> getSuggestedTargets( | ||
| ImmutableListMultimap<Integer, String> editDistanceToWords, int maxEditDistance) { | ||
| // iterate through until MAX is achieved | ||
| int total = 0; | ||
| ImmutableList.Builder<String> suggestedTargets = ImmutableList.builder(); | ||
| for (int editDistance = 2; | ||
| editDistance < maxEditDistance && total < MAX_SUGGESTED_TARGETS_SIZE; | ||
| editDistance++) { | ||
|
|
||
| ImmutableList<String> values = editDistanceToWords.get(editDistance); | ||
| int addAmount = Math.min(values.size(), MAX_SUGGESTED_TARGETS_SIZE - total); | ||
| suggestedTargets.addAll(values.subList(0, addAmount)); | ||
| total += addAmount; | ||
| } | ||
|
|
||
| return suggestedTargets.build(); | ||
| } | ||
|
|
||
| /** | ||
| * Create a pretty-printable String for a list. Joiner doesn't currently support multiple | ||
| * separators so this is a custom roll for now. Returns a comma-delimited list with " or " before | ||
| * the last element. | ||
| */ | ||
| @VisibleForTesting | ||
| public static String prettyPrintTargets(ImmutableList<String> targets) { | ||
| String targetString; | ||
| if (targets.isEmpty()) { | ||
| return ""; | ||
| } else if (targets.size() == 1) { | ||
| targetString = targets.get(0); | ||
| } else { | ||
|
|
||
| String firstPart = Joiner.on(", ").join(targets.subList(0, targets.size() - 1)); | ||
|
|
||
| targetString = Joiner.on(", or ").join(firstPart, Iterables.getLast(targets)); | ||
| } | ||
| return " (did you mean " + targetString + "?)"; | ||
| } | ||
| } |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
100 changes: 100 additions & 0 deletions
100
src/test/java/com/google/devtools/build/lib/packages/TargetSuggesterTest.java
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| // Copyright 2023 The Bazel Authors. All rights reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| package com.google.devtools.build.lib.packages; | ||
|
|
||
| import static com.google.common.truth.Truth.assertThat; | ||
|
|
||
| import com.google.common.collect.ImmutableList; | ||
| import java.util.HashSet; | ||
| import java.util.Set; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.junit.runners.JUnit4; | ||
|
|
||
| @RunWith(JUnit4.class) | ||
| public class TargetSuggesterTest { | ||
|
|
||
| @Test | ||
| public void testRangeDoesntSuggestTarget() { | ||
| String requestedTarget = "range"; | ||
| Set<String> packageTargets = new HashSet<>(); | ||
| packageTargets.add("target"); | ||
|
|
||
| ImmutableList<String> suggestedTargets = | ||
| TargetSuggester.suggestedTargets(requestedTarget, packageTargets); | ||
| assertThat(suggestedTargets).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testMisspelledTargetRetrievesProperSuggestion() { | ||
| String misspelledTarget = "AnrdiodTest"; | ||
|
|
||
| Set<String> packageTargets = new HashSet<>(); | ||
| packageTargets.add("AndroidTest"); | ||
| packageTargets.add("AndroidTest_deploy"); | ||
| packageTargets.add("AndroidTest_java"); | ||
|
|
||
| ImmutableList<String> suggestedTargets = | ||
| TargetSuggester.suggestedTargets(misspelledTarget, packageTargets); | ||
| assertThat(suggestedTargets).containsExactly("AndroidTest"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testRetrieveMultipleTargets() { | ||
| String misspelledTarget = "pixel_2_test"; | ||
|
|
||
| Set<String> packageTargets = new HashSet<>(); | ||
| packageTargets.add("pixel_5_test"); | ||
| packageTargets.add("pixel_6_test"); | ||
| packageTargets.add("android_2_test"); | ||
|
|
||
| ImmutableList<String> suggestedTargets = | ||
| TargetSuggester.suggestedTargets(misspelledTarget, packageTargets); | ||
| assertThat(suggestedTargets).containsExactly("pixel_5_test", "pixel_6_test"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testOnlyClosestTargetIsReturned() { | ||
| String misspelledTarget = "Pixel_5_test"; | ||
|
|
||
| Set<String> packageTargets = new HashSet<>(); | ||
| packageTargets.add("pixel_5_test"); | ||
| packageTargets.add("pixel_6_test"); | ||
| packageTargets.add("android_2_test"); | ||
|
|
||
| ImmutableList<String> suggestedTargets = | ||
| TargetSuggester.suggestedTargets(misspelledTarget, packageTargets); | ||
| assertThat(suggestedTargets).containsExactly("pixel_5_test"); | ||
| } | ||
|
|
||
| @Test | ||
| public void prettyPrintEmpty() { | ||
| String empty = TargetSuggester.prettyPrintTargets(ImmutableList.of()); | ||
| assertThat(empty).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| public void prettyPrintSingleTarget_returnsSingleTarget() { | ||
| ImmutableList<String> targets = ImmutableList.of("pixel_5_test"); | ||
| String targetString = TargetSuggester.prettyPrintTargets(targets); | ||
| assertThat(targetString).isEqualTo(" (did you mean pixel_5_test?)"); | ||
| } | ||
|
|
||
| @Test | ||
| public void prettyPrintMultipleTargets_returnsMultipleTargets() { | ||
| ImmutableList<String> targets = ImmutableList.of("pixel_5_test", "pixel_6_test"); | ||
| String targetString = TargetSuggester.prettyPrintTargets(targets); | ||
| assertThat(targetString).isEqualTo(" (did you mean pixel_5_test, or pixel_6_test?)"); | ||
| } | ||
| } |
Oops, something went wrong.
ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarkDacek I like the new extended list of suggestions, but could you explain on more detail why you removed the
querysuggestion? It's very useful for external repositories, so maybe we would want to restrict it to those.Cc @Wyverald
ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmeum Internally there was a bit of confusion for internal users.
Part of the rationale was wondering how likely it would that the
querycommand suggestion would be needed if we gave better suggestions. Are users typing inbazel build //foo:nonsensejust to get a copy-pasteable query command?ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better suggestions are very helpful if there really is a typo in the target name. The query command is most helpful if there is a typo in the package, as the output of the
querycommand usually gives the user an idea of what package they ended up referring to. They can then interactively discover the right one by making small changes to thequerycommand.This is also shown if there is a typo in a BUILD file, which I would see as the most common and useful case for this.
@MarkDacek How about the following: If the new logic finds any reasonably similar target names, just suggest those. If no such suggestion is found, instead show the query command.
ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some more context, the original motivation for the
query ...thing is there was an instance of the existing edit-distance logic missing a confusingly-named generated target, so we added the "tip" to list all targets. Alas, folks have become habituated to copy-pasting the suggested commands bazel spits out, so they were copy/pastingquery ...then got confused when it didn't work, which motivated this reconsideration.To address this we could have threaded through and prepended
productName, but there's a surprising amount of added complexity/boilerplate and memory considerations, it misses when there's a wrapper or something else runningbazel, and it doesn't do much for fully IDE-based workflows or CI failures, where one doesn't have a terminal open. Thus, we decided to retry the original thing of taking a better guess at what the user meant.ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the missing
productNamedefinitely makes this warning error less useful. In Bazel, other references tobazelare emitted in fix-up commands (e.g. Java strict deps violations), so there is precedent for these kind of commands relying onbazelbeing the way to run Bazel.@michajlo I do like the approach to just guess harder. What do you think of keeping in the fallback to a
querysuggestion if that doesn't work?ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think let's let this bake for a bit and see how it goes? We got a surprising number of complaints about the query thing not being copy-paste-able :/.
ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me - I am just a bit worried that the usefulness of this will differ between bazel and blaze.
@iancha1992 Could we cherry-pick this into 7.1.0 to testdrive the new behavior early?
ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmeum the branch for release-7.1.0 has not been created yet. Lemme ask if I can create the branch tomorrow and then I'll cherry-pick this. Thanks for your patience.
ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to cherry-pick this, but there was a conflict with:
src/main/java/com/google/devtools/build/lib/packages/Package.java.@fmeum @Wyverald @meteorcloudy @brandjon could you please take a look?
cc: @bazelbuild/triage
ef98ef9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iancha1992 I submitted #20636