Skip to content

Commit

Permalink
[7.4.0] Show "did you mean" suggestion for unknown repo names (#23360)
Browse files Browse the repository at this point in the history
If `@boo` is parsed in a context in which only `@foo` is visible, the
resulting `RepositoryName` object will show as follows in error
messages:

```
@@[unknown repo 'boo' requested from @@ (did you mean 'foo'?)]
```

Closes #23184.

PiperOrigin-RevId: 660678414
Change-Id: Iecb5b1d211ff9e1f04ff0f2189376a98b7b261a9

Closes #23189
Closes #23239
  • Loading branch information
fmeum authored Aug 21, 2024
1 parent 9409213 commit e3a47d9
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 10 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/cmdline/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ java_library(
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/main/java/net/starlark/java/annot",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/spelling",
"//src/main/protobuf:failure_details_java_proto",
"//third_party:auto_value",
"//third_party:caffeine",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Map.Entry;
import java.util.Optional;
import javax.annotation.Nullable;
import net.starlark.java.spelling.SpellChecker;

/**
* Stores the mapping from apparent repo name to canonical repo name, from the viewpoint of an
Expand Down Expand Up @@ -128,7 +129,8 @@ public RepositoryName get(String preMappingName) {
if (ownerRepo() == null) {
return RepositoryName.createUnvalidated(preMappingName);
} else {
return RepositoryName.createUnvalidated(preMappingName).toNonVisible(ownerRepo());
return RepositoryName.createUnvalidated(preMappingName)
.toNonVisible(ownerRepo(), SpellChecker.didYouMean(preMappingName, entries().keySet()));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Throwables;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
Expand Down Expand Up @@ -132,13 +133,22 @@ public static Pair<RepositoryName, PathFragment> fromPathFragment(
*/
private final RepositoryName ownerRepoIfNotVisible;

private RepositoryName(String name, RepositoryName ownerRepoIfNotVisible) {
/**
* If ownerRepoIfNotVisible is not null, this field stores the suffix to be appended to the error
*/
@Nullable private final String didYouMeanSuffix;

private RepositoryName(
String name,
@Nullable RepositoryName ownerRepoIfNotVisible,
@Nullable String didYouMeanSuffix) {
this.name = name;
this.ownerRepoIfNotVisible = ownerRepoIfNotVisible;
this.didYouMeanSuffix = didYouMeanSuffix;
}

private RepositoryName(String name) {
this(name, null);
this(name, /* ownerRepoIfNotVisible= */ null, /* didYouMeanSuffix= */ null);
}

/**
Expand Down Expand Up @@ -192,10 +202,16 @@ public String getMarkerFileName() {
* actually not visible from the owner repository and should fail in {@code
* RepositoryDelegatorFunction} when fetching with this {@link RepositoryName} instance.
*/
public RepositoryName toNonVisible(RepositoryName ownerRepo) {
public RepositoryName toNonVisible(RepositoryName ownerRepo, String didYouMeanSuffix) {
Preconditions.checkNotNull(ownerRepo);
Preconditions.checkArgument(ownerRepo.isVisible());
return new RepositoryName(name, ownerRepo);
Preconditions.checkNotNull(didYouMeanSuffix);
return new RepositoryName(name, ownerRepo, didYouMeanSuffix);
}

@VisibleForTesting
public RepositoryName toNonVisible(RepositoryName ownerRepo) {
return toNonVisible(ownerRepo, "");
}

public boolean isVisible() {
Expand Down Expand Up @@ -239,7 +255,9 @@ public boolean isMain() {
// TODO(bazel-team): Rename to "getCanonicalForm".
public String getNameWithAt() {
if (!isVisible()) {
return String.format("@@[unknown repo '%s' requested from %s]", name, ownerRepoIfNotVisible);
return String.format(
"@@[unknown repo '%s' requested from %s%s]",
name, ownerRepoIfNotVisible, didYouMeanSuffix);
}
return "@@" + name;
}
Expand Down Expand Up @@ -339,16 +357,17 @@ public boolean equals(Object object) {
if (this == object) {
return true;
}
if (!(object instanceof RepositoryName)) {
if (!(object instanceof RepositoryName other)) {
return false;
}
RepositoryName other = (RepositoryName) object;
return OsPathPolicy.getFilePathOs().equals(name, other.name)
&& Objects.equals(ownerRepoIfNotVisible, other.ownerRepoIfNotVisible);
&& Objects.equals(ownerRepoIfNotVisible, other.ownerRepoIfNotVisible)
&& Objects.equals(didYouMeanSuffix, other.didYouMeanSuffix);
}

@Override
public int hashCode() {
return Objects.hash(OsPathPolicy.getFilePathOs().hash(name), ownerRepoIfNotVisible);
return Objects.hash(
OsPathPolicy.getFilePathOs().hash(name), ownerRepoIfNotVisible, didYouMeanSuffix);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,13 @@ public void composeWith() throws Exception {
assertThat(mapping.get("C")).isEqualTo(RepositoryName.create("C_mapped"));
assertThat(mapping.get("D")).isEqualTo(RepositoryName.create("D"));
}

@Test
public void unknownRepoDidYouMean() throws LabelSyntaxException {
RepositoryMapping mapping =
RepositoryMapping.create(
ImmutableMap.of("foo", RepositoryName.create("foo_internal")), RepositoryName.MAIN);
assertThat(mapping.get("boo").getNameWithAt())
.isEqualTo("@@[unknown repo 'boo' requested from @@ (did you mean 'foo'?)]");
}
}

0 comments on commit e3a47d9

Please sign in to comment.