add for checker for command ids order in a command set and union types #90

wekesa360 · 2023-11-27T21:12:25Z

The check_command_ids_order.py file accepts an argument which is the path to the directory with the command set python files, and checks the order of listed command sets' IDs, if they are in ascending order, exits with 1 if not.

For the check_union_types.py also accepts an argument which is the directory containing the command set files, checks if there are Tagged unions, in this case, the event_request's tag: _SetCommand_out_modKind is compared to the key of
cases: __SetCommand_outArray_element_modifier mapping

michalgr · 2023-11-28T11:33:34Z

projects/jdwp/tools/check_union_types.py

+    parser.add_argument(
+        "command_sets_dir", type=str, help="Directory containing command set files"
+    )


As we discussed during our meeting, let's filter by the name of the command set, not the name of a directory. Could you please make the filtering optional so that we can easily run this tool to guarantee all of the union fields are correct ?

michalgr · 2023-11-28T11:37:43Z

projects/jdwp/tools/check_union_types.py

+    command_sets_dir: str = args.command_sets_dir
+    for file in os.listdir(command_sets_dir):
+        if file.endswith(".py"):
+            module_name: str = os.path.splitext(file)[0]
+            module_name_title: str = (
+                module_name.replace("_", " ").title().replace(" ", "")
+            )
+            module = __import__(
+                f"projects.jdwp.defs.command_sets.{module_name}", fromlist=[module_name]
+            )
+
+            command_set: CommandSet = getattr(module, module_name_title)
+            if isinstance(command_set, CommandSet):
+                check_tagged_union_mappings(command_set)


As we discussed, could you add a list containing all command sets to command_sets/__init__.py ? In that case this code would look like:

for command_set in command_sets.ALL: if arg.command_set: if arg.command_set != command_set.name: continue check_command_set(command_set)

michalgr · 2023-11-28T11:50:05Z

projects/jdwp/tools/check_union_types.py

+def check_tagged_union_mappings(command_set: CommandSet) -> None:
+    for command in command_set.commands:
+        if command.out is None:
+            continue
+
+        for field in command.out.fields:
+            mod_kind_value = None
+
+            if isinstance(field.type, Array):
+                for element in field.type.element_type.fields:
+                    if isinstance(element.type, UnionTag):
+                        mod_kind_value = element.type.value
+                    if isinstance(element.type, TaggedUnion):
+                        union_tag: UnionTag = element.type.tag
+                        union_cases: dict = element.type.cases
+                        if union_tag.type.value != mod_kind_value:
+                            print(
+                                f"Error in command '{command.name}': Invalid union tag for field '{field.name}'."
+                            )
+                            exit(1)
+                        for case_value, case_type in union_cases.items():
+                            expected_case_value = expected_case_value_for_modifier_kind(
+                                case_value
+                            )
+                            if expected_case_value != case_type:
+                                print(
+                                    f"Error in command '{command.name}': Invalid case type for field '{field.name}'."
+                                )
+                                exit(1)


It makes sense to break this into smaller functions that invoke one another, something like:

def check_command_set(command_set: CommandSet) -> None: for command in command_set.commands: check_command(command) def check_command(command: Command) -> None: if command.out: check_struct(command.out) if command.reply: check_struct(command.reply) def check_struct(struct: Struct) -> None: for field in struct.fields: check_type(type) def check_type(type: Type) -> None: match type: case Struct(): check_struct(type) case Array(): check_struct(type.element_type) case TaggedUnion(): check_tagged_union(type) def check_tagged_union(union: TaggedUnion): for enum_value in union.tag.type.value: if enum_value not in union.caseses: # handle error for value in union.cases.values(): check_struct(value)

michalgr · 2023-11-28T11:51:38Z

projects/jdwp/tools/check_union_types.py

+def expected_case_value_for_modifier_kind(modifier_kind: ModifierKind) -> Struct:
+    if modifier_kind == ModifierKind.COUNT:
+        return CountModifier
+    elif modifier_kind == ModifierKind.CONDITIONAL:
+        return ConditionalModifier
+    elif modifier_kind == ModifierKind.THREAD_ONLY:
+        return ThreadOnlyModifier
+    elif modifier_kind == ModifierKind.CLASS_ONLY:
+        return ClassOnlyModifier
+    elif modifier_kind == ModifierKind.CLASS_MATCH:
+        return ClassMatchModifier
+    elif modifier_kind == ModifierKind.CLASS_EXCLUDE:
+        return ClassExcludeModifier
+    elif modifier_kind == ModifierKind.STEP:
+        return StepModifier
+    elif modifier_kind == ModifierKind.LOCATION_ONLY:
+        return LocationOnlyModifier
+    elif modifier_kind == ModifierKind.EXCEPTION_ONLY:
+        return ExceptionOnlyModifier
+    elif modifier_kind == ModifierKind.FIELD_ONLY:
+        return FieldOnlyModifier
+    elif modifier_kind == ModifierKind.INSTANCE_ONLY:
+        return InstanceOnlyModifier
+    elif modifier_kind == ModifierKind.SOURCE_NAME_MATCH:
+        return SourceNameMatchModifier
+    else:
+        raise ValueError(f"Unknown modifierKind: {modifier_kind}")
+


Let's not verify actual structs corresponding to modifier_kind, this is hard to generalize.

projects/jdwp/tools/check_union_types.py

michalgr · 2023-11-28T11:53:20Z

projects/jdwp/tools/check_command_ids_order.py

+    parser = ArgumentParser(description="Check command IDs in command sets")
+    parser.add_argument("--verbose", action="store_true", help="Enable verbose output")
+    parser.add_argument(
+        "command_sets_dir", type=str, help="Directory containing command set files"
+    )
+    args = parser.parse_args()
+
+    command_sets_dir = os.path.abspath(args.command_sets_dir)
+    command_set_files = [f for f in os.listdir(command_sets_dir) if f.endswith(".py")]
+
+    for file in command_set_files:
+        try:
+            module_name = os.path.splitext(file)[0]
+            module_name_title = module_name.replace("_", " ").title().replace(" ", "")
+            module = __import__(
+                f"projects.jdwp.defs.command_sets.{module_name}", fromlist=[module_name]
+            )
+
+            if args.verbose:
+                print(f"Checking {module_name_title}...")
+
+            if any(isinstance(obj, CommandSet) for obj in vars(module).values()):
+                command_set = getattr(module, module_name_title)
+                check_command_ids(command_set)


Could you change this in a similar way to the other tool ?

michalgr · 2023-11-28T11:54:08Z

projects/jdwp/tools/check_command_ids_order.py

+        except Exception as e:
+            handle_file_error(file, e)


The other tool does not use exceptions to propagate errors, any reason why one does and the other does not ? Can we have both of them report errors in the same way ?

projects/jdwp/tools/check_command_ids_order.py

michalgr

Looks great ! We need one more thing, projects/jdwp/tools/BUCK file that will allow us to run new tools via BUCK :) Here is an example of how to do that: https://github.com/facebookexperimental/ExtendedAndroidTools/blob/main/projects/jdwp/codegen/BUCK#L3

michalgr · 2023-11-30T09:18:44Z

projects/jdwp/defs/command_sets/__init__.py

@@ -0,0 +1,9 @@
+from projects.jdwp.defs.command_sets.virtual_machine import VirtualMachine


Could you add the copyright here ?

I have added the line

michalgr · 2023-12-01T13:22:45Z

projects/jdwp/tools/BUCK

+    ],
+    main_module = "projects.jdwp.tools.check_command_ids_order",
+
+)


Could you add a new line here ?

michalgr · 2023-12-01T13:39:07Z

projects/jdwp/tools/BUCK

+python_binary(
+    name = "check-uniontypes",
+    deps = [
+        "//projects/jdwp/defs:defs"
+    ],
+    main_module = "projects.jdwp.tools.check_union_types",
+
+)
+
+python_binary(
+    name = "cmd-order-check",
+    deps = [


You need to define a python_library target containing all the *.py files in this directory and depending on //projects/jdwp/defs:defs and then both python_binaries should depend on that library only. It should look similar to this with one difference: we'll have two python_binaries instead of one and we don't care about the visibility attribute.

[jdwp] fix BUCK and union types checker

michalgr · 2023-12-07T18:08:45Z

projects/jdwp/tools/check_command_ids_order.py

+    except Exception as e:
+        print(f"Error checking command IDs in {command_set.name}: {e}")
+        exit(1)


what are we catching here ?

michalgr · 2023-12-07T18:11:28Z

projects/jdwp/tools/check_union_types.py

+    tagged_union: UnionTag = union.tag.type
+    for enum_value in tagged_union.value:
+        try:
+            union.cases[enum_value]


cases is not a dictionary any more, it's a sequence of key-values: https://github.com/facebookexperimental/ExtendedAndroidTools/blob/main/projects/jdwp/defs/schema.py#L88. The check needs to look a bit different now.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 27, 2023

michalgr requested changes Nov 28, 2023

View reviewed changes

wekesa360 force-pushed the jdwp_cli_tools branch from 4d9526d to 948a98b Compare November 29, 2023 16:30

michalgr reviewed Nov 30, 2023

View reviewed changes

wekesa360 force-pushed the jdwp_cli_tools branch from 948a98b to 6a0552e Compare November 30, 2023 17:22

michalgr reviewed Dec 1, 2023

View reviewed changes

wekesa360 force-pushed the jdwp_cli_tools branch from 6a0552e to f666a98 Compare December 7, 2023 16:19

[jdwp] add for checker for cmd ids orders and union types

aac1e28

[jdwp] fix BUCK and union types checker

wekesa360 force-pushed the jdwp_cli_tools branch from f666a98 to aac1e28 Compare December 7, 2023 16:21

michalgr requested changes Dec 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add for checker for command ids order in a command set and union types #90

add for checker for command ids order in a command set and union types #90

wekesa360 commented Nov 27, 2023

michalgr Nov 28, 2023

michalgr Nov 28, 2023

michalgr Nov 28, 2023

michalgr Nov 28, 2023

michalgr Nov 28, 2023

michalgr Nov 28, 2023

michalgr left a comment

michalgr Nov 30, 2023

wekesa360 Nov 30, 2023

michalgr Dec 1, 2023

michalgr Dec 1, 2023

michalgr Dec 7, 2023

michalgr Dec 7, 2023

		@@ -0,0 +1,9 @@
		from projects.jdwp.defs.command_sets.virtual_machine import VirtualMachine

add for checker for command ids order in a command set and union types #90

Are you sure you want to change the base?

add for checker for command ids order in a command set and union types #90

Conversation

wekesa360 commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment