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

[ffigen] Refactor AST filtering #1681

Merged
merged 13 commits into from
Oct 28, 2024
Merged

[ffigen] Refactor AST filtering #1681

merged 13 commits into from
Oct 28, 2024

Conversation

liamappelbe
Copy link
Contributor

@liamappelbe liamappelbe commented Oct 25, 2024

Currently all the filtering happens during parsing. This is complicated, error prone, and is blocking certain bugs from being fixed. The main goal of this PR is to instead parse everything, then filter the AST afterwards using the new visitor infra.

There are several new visitors:

  1. ApplyConfigFiltersVisitation uses the config filters to construct a set, included, containing all the Bindings that are included by the filters. The logic is copied from the old includer.dart file.
  2. FindByValueCompoundsVisitation and ClearOpaqueCompoundMembersVisitation replicate the old behavior where the config.structDependencies and config.unionDependencies flags could change structs and unions to be opaque, if they weren't explicitly included by the config filters, and they were never referred to by value anywhere in the bindings (only by pointer).
  3. FindTransitiveDepsVisitation just constructs a set, transitives, of all Bindings reachable from the included set.
  4. ListBindingsVisitation is now in charge of combining the included and transitives sets to figure out which Bindings should be included in the final list of bindings.
  5. MarkBindingsVisitation just sets the generateBindings flag for everything in the final binding set. Needs to be separate from ListBindingsVisitation because it needs to iterate the whole tree so that it can set generateBindings to false for omitted Bindings (ListBindingsVisitation doesn't iterate the whole AST).

Other changes:

  • AST transformation has been moved out of Library and into parser.dart. Library is now a pretty dumb class that just holds some Bindings and doesn't transform them at all. This is much cleaner, but means that a few of the tests had to be updated, because they were constructing Library objects directly from synthetic Bindings, and relying on the automatic transformations.
  • Removed the ubiquitous and confusing ignoreFilter flag from all the sub_parsers. It was used to control the behavior of transitive deps, but now that's much more clearly implemented in the visitors.
  • Deleted header_parser/includer.dart. All the inclusion logic is now in the visitors.
  • A lot more AST nodes are now individually visitable (see the new methods in Visitation).
  • Added a top level visit function for the common case where you just want to run a visitation on a single list of nodes.
  • Split Constant into UnnamedEnumConstant and MacroConstant so that the visitors can distinguish them (they have different config filtering rules).
  • Make Binding implement the config Declaration type, allowing us to pass bindings directly to the config filter functions.
  • Promote Binding.generateBindings to a mutable bool field. This field is filled in by MarkBindingsVisitation, and is used so that bindings that have been filtered out but still appear in the AST codegen sensibly. Currently this only affects Typealias, but I'll use it for stubbing ObjC interfaces in a follow up. Previously we were using this field to skip binding generation inside the toBindingString method, but now that behavior is handled by just omitting filtered bindings from the finalBindings list in _transformBindings.
  • Had to add NSOrderedCollectionDifference to package:objective_c.

#1259

Copy link

github-actions bot commented Oct 25, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️
File Coverage
pkgs/ffigen/example/libclang-example/generated_bindings.dart 💔 Not covered
pkgs/ffigen/example/objective_c/avf_audio_bindings.dart 💔 Not covered
pkgs/ffigen/example/swift/swift_api_bindings.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/binding.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/compound.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/constant.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/enum_class.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/func.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/global.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/library.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_interface.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_nullable.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_protocol.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/pointer.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/struct.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/typealias.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/union.dart 💔 Not covered
pkgs/ffigen/lib/src/config_provider/config_types.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/compounddecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/enumdecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/functiondecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/macro_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcinterfacedecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcprotocoldecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/typedefdecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/unnamed_enumdecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/var_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/translation_unit_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/type_extractor/extractor.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/apply_config_filters.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/find_transitive_deps.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/list_bindings.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/opaque_compounds.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/visitor.dart 💔 Not covered
pkgs/objective_c/lib/objective_c.dart 💔 Not covered
pkgs/objective_c/lib/src/c_bindings_generated.dart 💔 Not covered
pkgs/objective_c/lib/src/objective_c_bindings_generated.dart 💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ⚠️
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_sort_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_enum_int_mimic_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/objective_c/lib/src/ns_input_stream.dart
pkgs/swift2objc/lib/src/config.dart
pkgs/swift2objc/lib/src/generate_wrapper.dart
pkgs/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swift2objc/lib/src/generator/generator.dart
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_initializer_declaration.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_variable_declaration.dart
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart
pkgs/swift2objc/lib/src/transformer/_core/utils.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_globals.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_variable.dart
pkgs/swift2objc/test/unit/parse_initializer_param_test.dart

This check can be disabled by tagging the PR with skip-license-check.

@coveralls
Copy link

coveralls commented Oct 28, 2024

Coverage Status

coverage: 91.61% (+0.5%) from 91.126%
when pulling 8007e59 on filterast
into fb39b6f on main.

@liamappelbe liamappelbe changed the title WIP: [ffigen] Refactor AST filtering [ffigen] Refactor AST filtering Oct 28, 2024
@liamappelbe liamappelbe marked this pull request as ready for review October 28, 2024 05:44
Copy link
Member

@HosseinYousefi HosseinYousefi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@liamappelbe liamappelbe merged commit 8a8603b into main Oct 28, 2024
20 checks passed
@liamappelbe liamappelbe deleted the filterast branch October 28, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants