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

Modify the error message that occurs when a requested target does not… #20636

Merged
Show file tree
Hide file tree
Changes from all 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
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
Loading