Skip to content

Commit

Permalink
Make QuerySummary a member variable in BlazeQueryParser.
Browse files Browse the repository at this point in the history 
The parser is already single use, to making the summary a member simplifies it's implementation avoiding the need to pass the query around to all methods. This also unlocks further code improvements required to implemet c++ support.

Also add a simple factory class to ensure that the single use semantics of the parser are clear for all callers.

PiperOrigin-RevId: 568487464
  • Loading branch information
@copybara-github
Googler authored and copybara-github committed Sep 27, 2023
1 parent 092b862 commit 11bf3fe
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 49 deletions.
50 changes: 35 additions & 15 deletions querysync/java/com/google/idea/blaze/qsync/BlazeQueryParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,24 @@
*/
public class BlazeQueryParser {

/**
* Factory class to allow the creation of the the parser to be decoupled from the injection of its
* dependencies.
*/
public static class Factory {
private final Context<?> context;
private final ImmutableSet<String> alwaysBuildRuleKinds;

public Factory(Context<?> context, ImmutableSet<String> alwaysBuildRuleKinds) {
this.context = context;
this.alwaysBuildRuleKinds = alwaysBuildRuleKinds;
}

public BlazeQueryParser newParser(QuerySummary querySummary) {
return new BlazeQueryParser(querySummary, context, alwaysBuildRuleKinds);
}
}

// Rules that will need to be built, whether or not the target is included in the
// project.
public static final ImmutableSet<String> ALWAYS_BUILD_RULE_KINDS =
Expand Down Expand Up @@ -85,6 +103,8 @@ public class BlazeQueryParser {
private final Context<?> context;
private final SetView<String> alwaysBuildRuleKinds;

private final QuerySummary query;

private final BuildGraphData.Builder graphBuilder = BuildGraphData.builder();
private final PackageSet.Builder packages = new PackageSet.Builder();
private final Map<Label, Set<Label>> ruleDeps = Maps.newHashMap();
Expand All @@ -96,16 +116,18 @@ public class BlazeQueryParser {
// An aggregation of all the dependencies of java rules
private final Set<Label> javaDeps = new HashSet<>();

public BlazeQueryParser(Context<?> context, ImmutableSet<String> handledRuleKinds) {
public BlazeQueryParser(
QuerySummary query, Context<?> context, ImmutableSet<String> handledRuleKinds) {
this.context = context;
this.alwaysBuildRuleKinds = Sets.difference(ALWAYS_BUILD_RULE_KINDS, handledRuleKinds);
this.query = query;
}

private static boolean isJavaRule(String ruleClass) {
return JAVA_RULE_TYPES.contains(ruleClass) || ANDROID_RULE_TYPES.contains(ruleClass);
}

public BuildGraphData parse(QuerySummary query) {
public BuildGraphData parse() {
context.output(PrintOutput.log("Analyzing project structure..."));

long now = System.nanoTime();
Expand Down Expand Up @@ -139,7 +161,7 @@ public BuildGraphData parse(QuerySummary query) {
graphBuilder.targetMapBuilder().put(ruleEntry.getKey(), buildTarget.build());

if (isJavaRule(ruleClass)) {
visitJavaRule(query, ruleEntry.getKey(), ruleEntry.getValue());
visitJavaRule(ruleEntry.getKey(), ruleEntry.getValue());
}
if (alwaysBuildRuleKinds.contains(ruleClass)) {
projectTargetsToBuild.add(ruleEntry.getKey());
Expand Down Expand Up @@ -178,9 +200,9 @@ public BuildGraphData parse(QuerySummary query) {
return graph;
}

private void visitJavaRule(QuerySummary query, Label label, Query.Rule rule) {
private void visitJavaRule(Label label, Query.Rule rule) {
graphBuilder.allTargetsBuilder().add(label);
ImmutableSet<Label> thisSources = sourcesWithExpandedFileGroups(rule, query);
ImmutableSet<Label> thisSources = sourcesWithExpandedFileGroups(rule);
Set<Label> thisDeps = Sets.newHashSet(toLabelList(rule.getDepsList()));
ruleDeps.computeIfAbsent(label, x -> Sets.newHashSet()).addAll(thisDeps);

Expand Down Expand Up @@ -223,36 +245,34 @@ private ImmutableMultimap<Label, Label> calculateReverseDeps() {
}

/** Returns a set of all sources for a rule, expanding any in-project {@code filegroup} rules */
private ImmutableSet<Label> sourcesWithExpandedFileGroups(Rule rule, QuerySummary query) {
private ImmutableSet<Label> sourcesWithExpandedFileGroups(Rule rule) {
ImmutableSet<Label> rawLabels =
ImmutableSet.<Label>builder()
.addAll(toLabelList(rule.getSourcesList()))
.addAll(toLabelList(rule.getResourceFilesList()))
.build();
return rawLabels.stream()
.flatMap(l -> expandFileGroups(l, query).stream())
.collect(toImmutableSet());
return rawLabels.stream().flatMap(l -> expandFileGroups(l).stream()).collect(toImmutableSet());
}

private ImmutableSet<Label> expandFileGroups(Label label, QuerySummary summary) {
if (!isFileGroup(label, summary)) {
private ImmutableSet<Label> expandFileGroups(Label label) {
if (!isFileGroup(label)) {
return ImmutableSet.of(label);
}
Set<Label> visited = Sets.newHashSet();
ImmutableSet.Builder<Label> result = ImmutableSet.builder();

for (String source : requireNonNull(summary.getRulesMap().get(label)).getSourcesList()) {
for (String source : requireNonNull(query.getRulesMap().get(label)).getSourcesList()) {
Label asLabel = Label.of(source);
if (visited.add(asLabel)) {
result.addAll(expandFileGroups(asLabel, summary));
result.addAll(expandFileGroups(asLabel));
}
}

return result.build();
}

private boolean isFileGroup(Label label, QuerySummary summary) {
Rule rule = summary.getRulesMap().get(label);
private boolean isFileGroup(Label label) {
Rule rule = query.getRulesMap().get(label);
if (rule == null) {
return false;
}
Expand Down
6 changes: 3 additions & 3 deletions querysync/java/com/google/idea/blaze/qsync/FullProjectUpdate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class FullProjectUpdate implements RefreshOperation {

private final Context<?> context;
private final Path effectiveWorkspaceRoot;
private final BlazeQueryParser queryParser;
private final BlazeQueryParser.Factory queryParserFactory;
private final ProjectDefinition projectDefinition;
private final GraphToProjectConverter graphToProjectConverter;

Expand All @@ -60,7 +60,7 @@ public FullProjectUpdate(
this.result =
PostQuerySyncData.builder().setProjectDefinition(definition).setVcsState(vcsState);
this.projectDefinition = definition;
this.queryParser = new BlazeQueryParser(context, handledRuleKinds);
this.queryParserFactory = new BlazeQueryParser.Factory(context, handledRuleKinds);
this.graphToProjectConverter =
new GraphToProjectConverter(
packageReader, effectiveWorkspaceRoot, context, projectDefinition, executor);
Expand All @@ -80,7 +80,7 @@ public void setQueryOutput(QuerySummary output) {
@Override
public BlazeProjectSnapshot createBlazeProject() throws BuildException {
PostQuerySyncData newData = result.build();
BuildGraphData graph = queryParser.parse(newData.querySummary());
BuildGraphData graph = queryParserFactory.newParser(newData.querySummary()).parse();
ProjectProto.Project project = graphToProjectConverter.createProject(graph);
return BlazeProjectSnapshot.builder().queryData(newData).graph(graph).project(project).build();
}
Expand Down
6 changes: 3 additions & 3 deletions querysync/java/com/google/idea/blaze/qsync/PartialProjectRefresh.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class PartialProjectRefresh implements RefreshOperation {

private final Path effectiveWorkspaceRoot;
private final PostQuerySyncData previousState;
private final BlazeQueryParser queryParser;
private final BlazeQueryParser.Factory queryParserFactory;
private final GraphToProjectConverter graphToProjectConverter;
private final PostQuerySyncData.Builder newState;
@VisibleForTesting final ImmutableSet<Path> modifiedPackages;
Expand All @@ -72,7 +72,7 @@ class PartialProjectRefresh implements RefreshOperation {
.setProjectDefinition(previousState.projectDefinition());
this.modifiedPackages = modifiedPackages;
this.deletedPackages = deletedPackages;
this.queryParser = new BlazeQueryParser(context, handledRuleKinds);
this.queryParserFactory = new BlazeQueryParser.Factory(context, handledRuleKinds);
this.graphToProjectConverter =
new GraphToProjectConverter(
packageReader,
Expand Down Expand Up @@ -111,7 +111,7 @@ public BlazeProjectSnapshot createBlazeProject() throws BuildException {
Preconditions.checkNotNull(partialQuery, "queryOutput");
QuerySummary effectiveQuery = applyDelta();
PostQuerySyncData postQuerySyncData = newState.setQuerySummary(effectiveQuery).build();
BuildGraphData graph = queryParser.parse(effectiveQuery);
BuildGraphData graph = queryParserFactory.newParser(effectiveQuery).parse();
ProjectProto.Project project = graphToProjectConverter.createProject(graph);
return BlazeProjectSnapshot.builder()
.queryData(postQuerySyncData)
Expand Down
2 changes: 1 addition & 1 deletion querysync/java/com/google/idea/blaze/qsync/util/ProjectSpecBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ private int run() throws IOException, BuildException {
.orElseThrow()
.getSyncData();
BuildGraphData buildGraph =
new BlazeQueryParser(context, ImmutableSet.of()).parse(snapshot.querySummary());
new BlazeQueryParser(snapshot.querySummary(), context, ImmutableSet.of()).parse();
GraphToProjectConverter converter =
new GraphToProjectConverter(
packageReader, workspaceRoot, context, snapshot.projectDefinition(), executor);
Expand Down
14 changes: 10 additions & 4 deletions querysync/javatests/com/google/idea/blaze/qsync/GraphToProjectConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -638,8 +638,11 @@ public void testConvertProject_buildGraphWithSingleImportRoot() throws Exception
newDirectExecutorService());

BuildGraphData buildGraphData =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.JAVA_LIBRARY_NO_DEPS_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.JAVA_LIBRARY_NO_DEPS_QUERY),
NOOP_CONTEXT,
ImmutableSet.of())
.parse();
ProjectProto.Project project = converter.createProject(buildGraphData);

// Sanity check
Expand Down Expand Up @@ -681,8 +684,11 @@ public void testTestSources() throws Exception {
ImmutableSet.of("querysync/javatests/*")),
newDirectExecutorService());
BuildGraphData buildGraphData =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.JAVA_LIBRARY_NO_DEPS_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.JAVA_LIBRARY_NO_DEPS_QUERY),
NOOP_CONTEXT,
ImmutableSet.of())
.parse();
ProjectProto.Project project = converter.createProject(buildGraphData);

assertThat(project.getModulesCount()).isEqualTo(1);
Expand Down
4 changes: 2 additions & 2 deletions querysync/javatests/com/google/idea/blaze/qsync/TestDataSyncRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public BlazeProjectSnapshot sync(TestData testProject) throws IOException, Build
.setVcsState(Optional.empty())
.build();
BuildGraphData buildGraphData =
new BlazeQueryParser(context, ImmutableSet.of()).parse(querySummary);
new BlazeQueryParser(querySummary, context, ImmutableSet.of()).parse();
GraphToProjectConverter converter =
new GraphToProjectConverter(
packageReader,
Expand All @@ -72,7 +72,7 @@ public BlazeProjectSnapshot sync(TestData testProject) throws IOException, Build
Project project = converter.createProject(buildGraphData);
return BlazeProjectSnapshot.builder()
.queryData(pqsd)
.graph(new BlazeQueryParser(context, ImmutableSet.of()).parse(querySummary))
.graph(new BlazeQueryParser(querySummary, context, ImmutableSet.of()).parse())
.project(project)
.build();
}
Expand Down
64 changes: 44 additions & 20 deletions querysync/javatests/com/google/idea/blaze/qsync/project/BuildGraphTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,11 @@ public class BuildGraphTest {
@Test
public void testJavaLibraryNoDeps() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.JAVA_LIBRARY_NO_DEPS_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.JAVA_LIBRARY_NO_DEPS_QUERY),
NOOP_CONTEXT,
ImmutableSet.of())
.parse();
assertThat(graph.allTargets().toLabelSet())
.containsExactly(Label.of("//" + TESTDATA_ROOT + "/nodeps:nodeps"));
assertThat(graph.getAllSourceFiles())
Expand All @@ -59,8 +62,11 @@ public void testJavaLibraryNoDeps() throws Exception {
@Test
public void testJavaLibraryExternalDep() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.JAVA_LIBRARY_EXTERNAL_DEP_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.JAVA_LIBRARY_EXTERNAL_DEP_QUERY),
NOOP_CONTEXT,
ImmutableSet.of())
.parse();
assertThat(
graph.getFileDependencies(
TESTDATA_ROOT.resolve("externaldep/TestClassExternalDep.java")))
Expand All @@ -70,8 +76,11 @@ public void testJavaLibraryExternalDep() throws Exception {
@Test
public void testJavaLibraryInternalDep() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.JAVA_LIBRARY_INTERNAL_DEP_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.JAVA_LIBRARY_INTERNAL_DEP_QUERY),
NOOP_CONTEXT,
ImmutableSet.of())
.parse();
// Sanity check:
assertThat(graph.getAllSourceFiles())
.contains(TESTDATA_ROOT.resolve("nodeps/TestClassNoDeps.java"));
Expand All @@ -84,8 +93,11 @@ public void testJavaLibraryInternalDep() throws Exception {
@Test
public void testJavaLibraryTransientDep() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.JAVA_LIBRARY_TRANSITIVE_DEP_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.JAVA_LIBRARY_TRANSITIVE_DEP_QUERY),
NOOP_CONTEXT,
ImmutableSet.of())
.parse();
// Sanity check:
assertThat(graph.getAllSourceFiles())
.contains(TESTDATA_ROOT.resolve("externaldep/TestClassExternalDep.java"));
Expand All @@ -98,8 +110,11 @@ public void testJavaLibraryTransientDep() throws Exception {
@Test
public void testJavaLibraryProtoDep() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.JAVA_LIBRARY_PROTO_DEP_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.JAVA_LIBRARY_PROTO_DEP_QUERY),
NOOP_CONTEXT,
ImmutableSet.of())
.parse();
assertThat(graph.getFileDependencies(TESTDATA_ROOT.resolve("protodep/TestClassProtoDep.java")))
.containsExactly(
Label.of("//" + TESTDATA_ROOT + "/protodep:proto_java_proto_lite"),
Expand All @@ -109,8 +124,11 @@ public void testJavaLibraryProtoDep() throws Exception {
@Test
public void testJavaLibraryMultiTargets() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.JAVA_LIBRARY_MULTI_TARGETS));
new BlazeQueryParser(
getQuerySummary(TestData.JAVA_LIBRARY_MULTI_TARGETS),
NOOP_CONTEXT,
ImmutableSet.of())
.parse();
assertThat(graph.allTargets().toLabelSet())
.containsExactly(
Label.of("//" + TESTDATA_ROOT + "/multitarget:nodeps"),
Expand All @@ -132,8 +150,9 @@ public void testJavaLibraryMultiTargets() throws Exception {
@Test
public void testJavaLibaryExportingExternalTargets() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.JAVA_EXPORTED_DEP_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.JAVA_EXPORTED_DEP_QUERY), NOOP_CONTEXT, ImmutableSet.of())
.parse();
Path sourceFile = TESTDATA_ROOT.resolve("exports/TestClassUsingExport.java");
assertThat(graph.getJavaSourceFiles()).containsExactly(sourceFile);
assertThat(graph.getFileDependencies(sourceFile))
Expand All @@ -143,8 +162,9 @@ public void testJavaLibaryExportingExternalTargets() throws Exception {
@Test
public void testAndroidLibrary() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.ANDROID_LIB_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.ANDROID_LIB_QUERY), NOOP_CONTEXT, ImmutableSet.of())
.parse();
assertThat(graph.getAllSourceFiles())
.containsExactly(
TESTDATA_ROOT.resolve("android/TestAndroidClass.java"),
Expand All @@ -169,8 +189,11 @@ public void testAndroidLibrary() throws Exception {
@Test
public void testProjectAndroidLibrariesWithAidlSource_areProjectDeps() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.ANDROID_AIDL_SOURCE_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.ANDROID_AIDL_SOURCE_QUERY),
NOOP_CONTEXT,
ImmutableSet.of())
.parse();
assertThat(graph.getAllSourceFiles())
.containsExactly(
TESTDATA_ROOT.resolve("aidl/TestAndroidAidlClass.java"),
Expand All @@ -195,8 +218,9 @@ public void testProjectAndroidLibrariesWithAidlSource_areProjectDeps() throws Ex
@Test
public void testFileGroupSource() throws Exception {
BuildGraphData graph =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of())
.parse(getQuerySummary(TestData.FILEGROUP_QUERY));
new BlazeQueryParser(
getQuerySummary(TestData.FILEGROUP_QUERY), NOOP_CONTEXT, ImmutableSet.of())
.parse();
Path sourceFile = TESTDATA_ROOT.resolve("filegroup/TestFileGroupSource.java");
Path subgroupSourceFile = TESTDATA_ROOT.resolve("filegroup/TestSubFileGroupSource.java");
assertThat(graph.projectDeps())
Expand Down
2 changes: 1 addition & 1 deletion querysync/javatests/com/google/idea/blaze/qsync/testdata/ProjectProtos.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public static Project forTestProject(TestData project) throws IOException, Build
newDirectExecutorService());

BuildGraphData buildGraphData =
new BlazeQueryParser(NOOP_CONTEXT, ImmutableSet.of()).parse(getQuerySummary(project));
new BlazeQueryParser(getQuerySummary(project), NOOP_CONTEXT, ImmutableSet.of()).parse();
return converter.createProject(buildGraphData);
}
}

0 comments on commit 11bf3fe

Please sign in to comment.