Skip to content

Commit

Permalink
Modify the error message that occurs when a requested target does not… (
Browse files Browse the repository at this point in the history
#20636)

… 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

Cherry-picked from ef98ef9
  • Loading branch information
fmeum authored Dec 21, 2023
1 parent 8f4b115 commit 8e2972b
Show file tree
Hide file tree
Showing 14 changed files with 300 additions and 77 deletions.
2 changes: 0 additions & 2 deletions src/main/java/com/google/devtools/build/lib/packages/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set",
"//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/split_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
"//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import static com.google.common.base.MoreObjects.firstNonNull;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.BiMap;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -80,7 +79,6 @@
import net.starlark.java.eval.Module;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.spelling.SpellChecker;
import net.starlark.java.syntax.Location;

/**
Expand Down Expand Up @@ -231,13 +229,6 @@ public enum ConfigSettingVisibilityPolicy {
*/
private RepositoryMapping repositoryMapping;

/**
* The repository mapping of the main repository. This is only used internally to obtain
* user-friendly apparent names from canonical repository names in error message arising from this
* package.
*/
private RepositoryMapping mainRepositoryMapping;

private ImmutableList<TargetPattern> registeredExecutionPlatforms;
private ImmutableList<TargetPattern> registeredToolchains;
private OptionalInt firstWorkspaceSuffixRegisteredToolchain;
Expand Down Expand Up @@ -405,7 +396,6 @@ private void finishInit(Builder builder) {
this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
this.firstWorkspaceSuffixRegisteredToolchain = builder.firstWorkspaceSuffixRegisteredToolchain;
this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping);
this.mainRepositoryMapping = Preconditions.checkNotNull(builder.mainRepositoryMapping);
ImmutableMap.Builder<RepositoryName, ImmutableMap<String, RepositoryName>>
repositoryMappingsBuilder = ImmutableMap.builder();
if (!builder.externalPackageRepositoryMappings.isEmpty() && !builder.isRepoRulePackage()) {
Expand Down Expand Up @@ -695,15 +685,7 @@ private String getAlternateTargetSuggestion(String targetName) {
+ getName()
+ "/BUILD?)";
} else {
String suggestedTarget = SpellChecker.suggest(targetName, targets.keySet());
String targetSuggestion =
suggestedTarget == null ? null : String.format("did you mean '%s'?", suggestedTarget);
String blazeQuerySuggestion =
String.format(
"Tip: use `query \"%s:*\"` to see all the targets in that package",
packageIdentifier.getDisplayForm(mainRepositoryMapping));
return String.format(
" (%s)", Joiner.on(" ").skipNulls().join(targetSuggestion, blazeQuerySuggestion));
return TargetSuggester.suggestTargets(targetName, targets.keySet());
}
}

Expand Down Expand Up @@ -894,11 +876,6 @@ default boolean precomputeTransitiveLoads() {
* workspace.
*/
private final RepositoryMapping repositoryMapping;
/**
* The repository mapping of the main repository. This is only used to resolve user-friendly
* apparent names from canonical repository names in error message arising from this package.
*/
private final RepositoryMapping mainRepositoryMapping;
/** Converts label literals to Label objects within this package. */
private final LabelConverter labelConverter;

Expand Down Expand Up @@ -1045,7 +1022,6 @@ public T intern(T sample) {
this.precomputeTransitiveLoads = packageSettings.precomputeTransitiveLoads();
this.noImplicitFileExport = noImplicitFileExport;
this.repositoryMapping = repositoryMapping;
this.mainRepositoryMapping = mainRepositoryMapping;
this.labelConverter = new LabelConverter(id, repositoryMapping);
if (pkg.getName().startsWith("javatests/")) {
mergePackageArgsFrom(PackageArgs.builder().setDefaultTestOnly(true));
Expand Down
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 + "?)";
}
}
4 changes: 3 additions & 1 deletion src/main/java/net/starlark/java/spelling/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,7 @@ java_library(
"//src/main/java/net/starlark/java:bazel",
"//src/main/java/net/starlark/java:starlark",
],
deps = ["//third_party:jsr305"],
deps = [
"//third_party:jsr305",
],
)
Original file line number Diff line number Diff line change
Expand Up @@ -657,8 +657,7 @@ public void testHelpfulErrorForWrongPackageLabels() throws Exception {
assertThat(result.hasError()).isTrue();
assertContainsEvent(
"no such target '//x:z': target 'z' not declared in package 'x' defined by"
+ " /workspace/x/BUILD (Tip: use `query \"//x:*\"` to see all the targets in that"
+ " package) and referenced by '//y:y'");
+ " /workspace/x/BUILD");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,7 @@ public void testFileThatsNotRegisteredYieldsUnknownTargetException() throws Exce
assertThrows(NoSuchTargetException.class, () -> pkg().getTarget("baz.txt"));
assertThat(e)
.hasMessageThat()
.isEqualTo(
"no such target '//pkg:baz.txt': target 'baz.txt' not declared in package 'pkg' "
+ "defined by /workspace/pkg/BUILD (did you mean 'bar.txt'? Tip: use `query "
+ "\"//pkg:*\"` to see all the targets in that package)");
.contains("no such target '//pkg:baz.txt': target 'baz.txt' not declared in package 'pkg'");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,7 @@ public void testCreationOfInputFiles() throws Exception {
NoSuchTargetException e = assertThrows(NoSuchTargetException.class, () -> pkg.getTarget("B"));
assertThat(e)
.hasMessageThat()
.isEqualTo(
"no such target '//foo:B': target 'B' not declared in package 'foo' defined by"
+ " /workspace/foo/BUILD (Tip: use `query \"//foo:*\"` to see all the targets in"
+ " that package)");
.contains("no such target '//foo:B': target 'B' not declared in package 'foo'");

// These are the only input files: BUILD, Z
Set<String> inputFiles = Sets.newTreeSet();
Expand Down Expand Up @@ -443,9 +440,9 @@ public void testHelpfulErrorForMissingExportsFiles() throws Exception {
assertThat(e)
.hasMessageThat()
.isEqualTo(
"no such target '//x:z.cc': target 'z.cc' not declared in package 'x' defined by"
+ " /workspace/x/BUILD (did you mean 'x.cc'? Tip: use `query \"//x:*\"` to see all"
+ " the targets in that package)");
"no such target '//x:z.cc': "
+ "target 'z.cc' not declared in package 'x' "
+ "defined by /workspace/x/BUILD (did you mean x.cc?)");

e = assertThrows(NoSuchTargetException.class, () -> pkg.getTarget("dir"));
assertThat(e)
Expand Down
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?)");
}
}
Loading

0 comments on commit 8e2972b

Please sign in to comment.