Prune imports no longer used by retained entities in the schema #2797

mpeyper · 2024-01-25T05:52:17Z

This is a WIP but wanted to get it up to get some feedback on the approach. This started with a discussion with @swankjesse after I discovered that imports are not necessarily being pruned from files when they are no longer needed by retained entities in the schema.

The existing logic appear to retain an imports when the file is being retained in the schema at all, regarless of whether the import is still being used by the file (and not by another file).

This change attempts to resolve the referenced types in the file and resolve them back to their import locations so that the list of actually required imports can be determined.

It's currently passing all existing test cases as well as the new test we added to replicate the issue, but I'm not sure if I'm capturing all ways a type can be referenced at the moment. Any insights into wierd edge cases that I should consider are very welcome.

I'll add some comments in the files where I'm not sure about things.

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt

mpeyper · 2024-01-25T05:55:23Z

wire-schema/src/jvmTest/kotlin/com/squareup/wire/schema/PrunerTest.kt

+      add(
+        "message.proto".toPath(),
+        """
+             |import 'footer.proto';


Previously this import was being retained even though AnotherMessage was being pruned, along with the need to keep importing it.

As discussed with @swankjesse, this test was good to replicate the issue, but I want to write more rubust cases around the different ways an import can be referenced before we merge this.

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt

mpeyper · 2024-01-25T06:07:36Z

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt

+    val typeLocations = referencedTypes.map { type -> type.location }
+
+    val extensionLocations = referencedTypes
+      .filterIsInstance<MessageType>()
+      .flatMap { type -> type.extensionFields }
+      .map { field -> field.location }


With the existing test suite, the locations that have been important to retain are the locations of the referenced types themselves (typeLocations), and the locations of any extensions used (extensionLocations).

Am I missing any other places I should be looking for locations?

I think that's cool

mpeyper · 2024-01-25T06:09:40Z

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt

+
+    val referencedImports = (typeLocations + extensionLocations).map { it.path }.toSet()
+
+    val retainedImports = imports.filter { referencedImports.contains(it) }


The logic here is that I want to ensure that I only remove existing imports and never add new ones. I had a case where google/protobuf/descriptor.proto was being inserted into the imports if I simply used referencedImports as the new retainedImports.

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt

mpeyper · 2024-01-25T06:19:12Z

...r-generator/src/test/java/com/squareup/wire/kotlin/grpcserver/FileDescriptorGeneratorTest.kt

@@ -46,7 +46,9 @@ internal class FileDescriptorGeneratorTest {
      |package test;
      |import "imported.proto";
      |
-      |message Test {}
+      |message Test {
+      |  optional Imported imported = 1;


A side effect of this change is that descriptor.dependencies.size was 0 without including a usage of the imported file because the unused import is getting pruned away. I'm not sure how much of an issue that is in reality though?

no problem I think

oldergod

🚀 LGTM

oldergod · 2024-01-25T10:22:41Z

...r-generator/src/test/java/com/squareup/wire/kotlin/grpcserver/FileDescriptorGeneratorTest.kt

@@ -46,7 +46,9 @@ internal class FileDescriptorGeneratorTest {
      |package test;
      |import "imported.proto";
      |
-      |message Test {}
+      |message Test {
+      |  optional Imported imported = 1;


no problem I think

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/Pruner.kt

oldergod · 2024-01-25T10:27:01Z

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt

+    val typeLocations = referencedTypes.map { type -> type.location }
+
+    val extensionLocations = referencedTypes
+      .filterIsInstance<MessageType>()
+      .flatMap { type -> type.extensionFields }
+      .map { field -> field.location }


I think that's cool

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt

swankjesse · 2024-01-25T14:36:20Z

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt

+
+    val referencedImports = (typeLocations + extensionLocations).map { it.path }.toSet()
+
+    val retainedImports = imports.filter { referencedImports.contains(it) }


wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/Pruner.kt

… API change

mpeyper · 2024-01-26T11:39:32Z

I've added a bunch of tests to test the various ways a type can be referenced and also to test the new import public pruning logic. I think I have all cases covered now and I think this is ok to merge now.

oldergod

I pushed a commit to satisfy spotless. Very good tests, thank you

swankjesse · 2024-01-26T14:50:23Z

wire-schema/src/jvmTest/kotlin/com/squareup/wire/schema/PrunerTest.kt

+  }
+
+  @Test
+  fun retainImportWhenUsedForNestedMessageField() {


These tests are repetitive but also very good

oldergod · 2024-01-26T14:56:02Z

🚀

mpeyper requested review from oldergod and swankjesse January 25, 2024 05:52

mpeyper commented Jan 25, 2024

View reviewed changes

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt Show resolved Hide resolved

mpeyper force-pushed the mpeyper/improve-import-pruning branch from b9b57e8 to afbcd98 Compare January 25, 2024 05:54

mpeyper commented Jan 25, 2024

View reviewed changes

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt Outdated Show resolved Hide resolved

mpeyper force-pushed the mpeyper/improve-import-pruning branch from afbcd98 to a0635fb Compare January 25, 2024 06:02

mpeyper commented Jan 25, 2024

View reviewed changes

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt Outdated Show resolved Hide resolved

mpeyper commented Jan 25, 2024

View reviewed changes

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt Show resolved Hide resolved

mpeyper force-pushed the mpeyper/improve-import-pruning branch from a0635fb to cf5b658 Compare January 25, 2024 06:17

mpeyper commented Jan 25, 2024

View reviewed changes

Prune imports no longer used by retained entities in the schema

f1729a6

mpeyper force-pushed the mpeyper/improve-import-pruning branch from cf5b658 to f1729a6 Compare January 25, 2024 08:18

oldergod approved these changes Jan 25, 2024

View reviewed changes

swankjesse reviewed Jan 25, 2024

View reviewed changes

wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/ProtoFile.kt Outdated Show resolved Hide resolved

swankjesse approved these changes Jan 25, 2024

View reviewed changes

mpeyper force-pushed the mpeyper/improve-import-pruning branch 2 times, most recently from 120072f to a8be91b Compare January 26, 2024 02:43

Prune import public imports, capture more types and remove breaking…

a4647cb

… API change

mpeyper force-pushed the mpeyper/improve-import-pruning branch from a8be91b to a4647cb Compare January 26, 2024 03:01

mpeyper added 2 commits January 26, 2024 21:53

Add import pruning tests for different referenced types

1971d4e

Fix import public pruning with test

b0b192f

mpeyper marked this pull request as ready for review January 26, 2024 11:37

mpeyper requested review from swankjesse and oldergod January 26, 2024 11:39

mpeyper and others added 2 commits January 26, 2024 22:43

Fix typos in import pruning tests

1c664eb

Applied spotless

602d3c7

oldergod approved these changes Jan 26, 2024

View reviewed changes

swankjesse reviewed Jan 26, 2024

View reviewed changes

swankjesse approved these changes Jan 26, 2024

View reviewed changes

oldergod merged commit dc0c8e1 into master Jan 26, 2024
7 checks passed

oldergod deleted the mpeyper/improve-import-pruning branch January 26, 2024 14:55

mpeyper mentioned this pull request Jan 27, 2024

Fix over pruning imports when only option types are being extended in a file #2800

Merged

abhisheksharma-dd mentioned this pull request Mar 12, 2024

Wire proto schema generation broken for protobuf messages containing a map with "protobuf.Any" type fields on versions >= 4.9.4 #2862

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prune imports no longer used by retained entities in the schema #2797

Prune imports no longer used by retained entities in the schema #2797

mpeyper commented Jan 25, 2024

mpeyper Jan 25, 2024 •

edited

mpeyper Jan 25, 2024 •

edited

oldergod Jan 25, 2024

mpeyper Jan 25, 2024 •

edited

swankjesse Jan 25, 2024

mpeyper Jan 25, 2024 •

edited

oldergod Jan 25, 2024

oldergod left a comment

oldergod Jan 25, 2024

oldergod Jan 25, 2024

swankjesse Jan 25, 2024

mpeyper commented Jan 26, 2024

oldergod left a comment

swankjesse Jan 26, 2024

oldergod commented Jan 26, 2024


		val referencedImports = (typeLocations + extensionLocations).map { it.path }.toSet()

		val retainedImports = imports.filter { referencedImports.contains(it) }

Prune imports no longer used by retained entities in the schema #2797

Prune imports no longer used by retained entities in the schema #2797

Conversation

mpeyper commented Jan 25, 2024

mpeyper Jan 25, 2024 • edited

Choose a reason for hiding this comment

mpeyper Jan 25, 2024 • edited

Choose a reason for hiding this comment

oldergod Jan 25, 2024

Choose a reason for hiding this comment

mpeyper Jan 25, 2024 • edited

Choose a reason for hiding this comment

swankjesse Jan 25, 2024

Choose a reason for hiding this comment

mpeyper Jan 25, 2024 • edited

Choose a reason for hiding this comment

oldergod Jan 25, 2024

Choose a reason for hiding this comment

oldergod left a comment

Choose a reason for hiding this comment

oldergod Jan 25, 2024

Choose a reason for hiding this comment

oldergod Jan 25, 2024

Choose a reason for hiding this comment

swankjesse Jan 25, 2024

Choose a reason for hiding this comment

mpeyper commented Jan 26, 2024

oldergod left a comment

Choose a reason for hiding this comment

swankjesse Jan 26, 2024

Choose a reason for hiding this comment

oldergod commented Jan 26, 2024

mpeyper Jan 25, 2024 •

edited

mpeyper Jan 25, 2024 •

edited

mpeyper Jan 25, 2024 •

edited

mpeyper Jan 25, 2024 •

edited